Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: exec command with failFast should fail immediately #665

Merged
merged 12 commits into from
Mar 21, 2024

Conversation

MohiuddinM
Copy link
Contributor

Description

The expected behavior of failFast option is to fail the whole exec command as soon as a single fail occurs, and it works as expected if concurrency=1. But if concurrency > 1, it diverges from the expected behavior and instead keeps running until all pooled jobs have finished executing.

This PR fixes this divergence, and the exec command will fail immediately on a single fail irrespective of concurrency value.

Type of Change

  • feat -- New feature (non-breaking change which adds functionality)
  • 🛠️ fix -- Bug fix (non-breaking change which fixes an issue)
  • ! -- Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 refactor -- Code refactor
  • ci -- Build configuration change
  • 📝 docs -- Documentation
  • 🗑️ chore -- Chore

@MohiuddinM
Copy link
Contributor Author

Note to maintainers: only the last commit is related, others are showing because I didn't fork specifically for this fix, and instead merged this into my fork.

Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, the change makes.
Could you add a test, so that we can avoid regression in the future?

packages/melos/lib/src/commands/exec.dart Outdated Show resolved Hide resolved
packages/melos/lib/src/commands/exec.dart Outdated Show resolved Hide resolved
@MohiuddinM
Copy link
Contributor Author

MohiuddinM commented Mar 17, 2024

@spydon I have:

  • Added additional logger to report canceled tasks separately from failed jobs:
            '$ melos exec\n'
            '  └> exit 1\n'
            '     └> FAILED (in 1 packages)\n'
            '        └> c (with exit code 1)\n'
            '     └> CANCELED (in 2 packages)\n'
            '        └> a (due to failFast)\n'
            '        └> b (due to failFast)\n'
  • Added tests
  • Refactored earlier implementation to remove exit (to make code testable)

@MohiuddinM
Copy link
Contributor Author

MohiuddinM commented Mar 17, 2024

Tests are flaky I guess because all 3 are being run at the same time, and some times "a" fails first, and "b", "c" are cancelled. Otherwise, some other fails. I need to find a way to add delay before "exit 1" so it becomes more predictable.

   Expected: String ignoring Ansii '$ melos exec\n'
              '  └> exit 1\n'
              '     └> RUNNING (in 3 packages)\n'
              '\n'
              '--------------------------------------------------------------------------------\n'
              '--------------------------------------------------------------------------------\n'
              '\n'
              '$ melos exec\n'
              '  └> exit 1\n'
              '     └> FAILED (in 1 packages)\n'
              '        └> c (with exit code 1)\n'
              '     └> CANCELED (in 2 packages)\n'
              '        └> a (due to failFast)\n'
              '        └> b (due to failFast)\n'
              ''
    Actual: '$ melos exec\n'
              '  └> exit 1\n'
              '     └> RUNNING (in 3 packages)\n'
              '\n'
              '--------------------------------------------------------------------------------\n'
              '--------------------------------------------------------------------------------\n'
              '\n'
              '$ melos exec\n'
              '  └> exit 1\n'
              '     └> FAILED (in 1 packages)\n'
              '        └> b (with exit code 1)\n'
              '     └> CANCELED (in 2 packages)\n'
              '        └> a (due to failFast)\n'
              '        └> c (due to failFast)\n'

@MohiuddinM
Copy link
Contributor Author

@spydon I found a solution to the random failure order by adding delays (see: 1ab3177).

The test failing now is unrelated I don't know why, but it passes on my PC.

@spydon spydon changed the title fix: exec command with failFast option set, fails immediately instead of waiting for pooled tasks to complete fix: exec command with failFast should fail immediately Mar 17, 2024
Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@spydon spydon merged commit a5ff6da into invertase:main Mar 21, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants