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

Add support for more than one isolated process. #779

Merged
merged 15 commits into from Sep 16, 2020
Merged

Conversation

v-kumar
Copy link
Contributor

@v-kumar v-kumar commented Sep 13, 2020

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

  • Feature branch is up-to-date with master (if not - rebase it).
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • Update Readme.md when cli options are changed

Copy link
Owner

@grosser grosser left a 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 😞

lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved
lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved
lib/parallel_tests/grouper.rb Outdated Show resolved Hide resolved
@grosser
Copy link
Owner

grosser commented Sep 13, 2020

FYI windows test failures are most likely not caused by your changes

@v-kumar
Copy link
Contributor Author

v-kumar commented Sep 13, 2020

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 😞

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>
CHANGELOG.md Outdated Show resolved Hide resolved
lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved
lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved
lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved
lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved
lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved
Comment on lines 264 to 266
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
Copy link
Owner

@grosser grosser Sep 14, 2020

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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...

  1. leave it as is.
  2. Ignore the bounds check.
  3. 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.

Copy link
Contributor Author

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.

Copy link
Owner

Choose a reason for hiding this comment

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

  1. fail with a nice error when doing the group logic ? :)

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is failing like this:

image

or

image

Specs pass locally.

Any ideas?

@grosser
Copy link
Owner

grosser commented Sep 16, 2020

tests seem to be failing now

@grosser
Copy link
Owner

grosser commented Sep 16, 2020

test output shows Number of isolated processes must be less than total the number of processes
so it looks like a raise or abort escaped ?

https://github.com/grosser/parallel_tests/pull/779/checks?check_run_id=1124628079

@v-kumar
Copy link
Contributor Author

v-kumar commented Sep 16, 2020

test output shows Number of isolated processes must be less than total the number of processes
so it looks like a raise or abort escaped ?

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.

@grosser grosser merged commit d95ea27 into grosser:master Sep 16, 2020
@grosser
Copy link
Owner

grosser commented Sep 16, 2020

3.3.0 🎉

@v-kumar
Copy link
Contributor Author

v-kumar commented Sep 16, 2020

Thank 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.

Support multiple isolated processes
2 participants