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
Add support for more than one isolated process. #779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall this looks correct ... it's getting kinda funky with all the levels of options, but I feel your pain :D
... ideally of course make your tests not need isolation, but I bet you tried that 😞
FYI windows test failures are most likely not caused by your changes |
I wish. When feature specs get spread out in multiple groups, each groups need an extra process (Puma server) to run the application. In containerized environments, this ends up needing more CPU/ram resources. We consistently that, for N cpus - N or even N-1/N-2 groups where feature specs are not single perform much worse compared to isolating the feature specs alone few groups. |
Co-authored-by: Michael Grosser <michael@grosser.it>
…validations and move them to the end
lib/parallel_tests/cli.rb
Outdated
if options[:isolate_count] && options[:isolate_count] >= ParallelTests.determine_number_of_processes(options[:count]) | ||
abort 'Number of isolated processes must be less than total the number of processes' | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this triggers determine_number_of_processes twice when count was not given and duplicates the count ||= determine_number_of_processes
logic
so this check should move to where count is set or count default setting should move here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried moving num_processes determination to parse_options. Lot of specs fail - as they don't expect :count
option. That does not feel right either - as parse_option should be just that.
I prefer validation bounds check for all options inside parse_options. This keeps the code in appropriate places at the cost of calling determine_number_of_processes twice.
But I did move the validation for this inside the main run
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that did not work as the specs fail as CLI specs call only the parse_options method.
I would leave it up to you.. Three ways to fix this...
- leave it as is.
- Ignore the bounds check.
- Do not fail on isolate_count being out of bounds. Instead coerce the isolate_count to (n-1) in the in_even_groups_by_size by size method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grosser ☝️ In case you have not seen the above reply yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- fail with a nice error when doing the group logic ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done... Please do check the whole PR again with fresh set of eyes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Michael Grosser <michael@grosser.it>
tests seem to be failing now |
test output shows https://github.com/grosser/parallel_tests/pull/779/checks?check_run_id=1124628079 |
Thanks for pointing that out. Abort is a bad idea from within a method. Good for CLI though. I made it raise an error instead of abort. Also updated Readme.cmd. |
3.3.0 🎉 |
Thank you. |
We isolate our rspec feature specs from others as the setup for features specs require more complexity. However, the isolated features take significantly much longer time than the rest of the individual parallel tests.
I would like to have multiple isolated processes.. Say, if n=6, I would like to reserve 2 for isolated and 4 for the rest of them.
fixes #780
Checklist
master
(if not - rebase it).code introduces user-observable changes.