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

Support fast failure through interrupts #12

Closed
wants to merge 2 commits into from

Conversation

stackmystack
Copy link
Contributor

This patch refactors the __run method in order to support fast failure through interrupts.

The fail_fast plugin is fairly simple and popular, and I need it, or at least I need an equivalent for it, for my projects using minitest-paralll_fork.

The provided patches does enable it, and it's not the prettiest of implementations, but given the fact that forks are involved, I don't see (directly) how to do better.

@jeremyevans
Copy link
Owner

I'm OK with the idea, but this will need covering tests before it can be accepted. Is that something you are willing to work on?

@jeremyevans
Copy link
Owner

Obviously it needs to pass existing tests as well, which CI indicates it does not.

@stackmystack
Copy link
Contributor Author

I'm OK with the idea, but this will need covering tests before it can be accepted. Is that something you are willing to work on?

Sure.

Obviously it needs to pass existing tests as well, which CI indicates it does not.

I ran them locally only (ruby 3.2), and they passed. I will check the issue with older rubies.

@stackmystack stackmystack force-pushed the fail-fast branch 2 times, most recently from d04d145 to d1cac6d Compare October 31, 2023 15:15
@stackmystack
Copy link
Contributor Author

Hello @jeremyevans,

I added tests for the new feature, ran them on my private branch and passing. I think it's ready for review.

PS: just a curious question; do you really need to support ruby < 2.5 ? I just found out that Array#none? is not compatible with later versions, and had to !array.inclue?(value), which is borderline offensive … just curious …

@jeremyevans
Copy link
Owner

I started reviewing this today. One big change it needs that I started working on is the fail fast part is basically broken in the sense that only one fork fails fast. All other forks run to completion. The included tests didn't catch this because they didn't even use the parallel processing, since only a single test suite was run in the fail fast case. This should probably have a development dependency on minitest-fail_fast, instead of having it copied locally. Also, I'd like to restructure this so all of the extra complexity is opt-in, due to the invasiveness of the changes and the fact that most users do not need the fail fast feature. I hope to have more time to work on this later this week.

@jeremyevans
Copy link
Owner

I solved this a different way, by refactoring minitest/parallel_fork and adding a minitest/parallel_fork/fail_fast file that handles the fast failing. Can you try that out and see if it works for you?

@stackmystack
Copy link
Contributor Author

Thanks Jeremy. I just got the time to test it and it works great :)

Cheers.

@stackmystack
Copy link
Contributor Author

On a side note, when can we expect a release?

@jeremyevans
Copy link
Owner

I'll try to put out a release this week.

@jeremyevans
Copy link
Owner

I just released minitest-parallel_fork 2.0.0 with the changes.

@stackmystack
Copy link
Contributor Author

Thank you Jeremy, that's very generous of you.

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