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 option to set parallel test worker count to the physical core count of the machine #34735

Merged
merged 2 commits into from Dec 18, 2018

Conversation

Projects
None yet
4 participants
@bogdanvlviv
Copy link
Contributor

commented Dec 17, 2018

Also, use the physical core count of the machine as
the default number of workers, and generate the test_helper.rb file
with parallelize(workers: :number_of_processors)

Closes #34734

@@ -12,6 +12,7 @@
require "active_support/testing/time_helpers"
require "active_support/testing/file_fixtures"
require "active_support/testing/parallelization"
require "concurrent"

This comment has been minimized.

Copy link
@kaspth

kaspth Dec 17, 2018

Member

Do we need to require the whole thing here?

This comment has been minimized.

Copy link
@bogdanvlviv

bogdanvlviv Dec 17, 2018

Author Contributor

Seems like a good idea just to require "concurrent/utility/processor_counter"

Add option to set parallel test worker count to the physical core cou…
…nt of the machine

Also, use the physical core count of the machine as
the default number of workers, and  generate the `test_helper.rb` file
with `parallelize(workers: :number_of_processors)`

Closes #34734

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:issue-34734 branch from 07cd59b to 9658872 Dec 17, 2018

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

I guess tests failed because the physical core count of the Travis' server is equal to 1.
I could reproduce those failures on my VM only by running those tests with PARALLEL_WORKERS=1.
I think we should change parallelize(workers: :number_of_processors) to parallelize(workers: 2) for those tests by the similar approach we do there.

Ensure that we always test parallel testing
Since #34734 we decided to use the physical core count of the machine as
the default number of workers in the parallel testing, we need to
ensure that some tests use at least 2 workers because we could
run those tests on VM that has only 1 physical core.
It also fixes tests failures on the CI since Travis server we are using
has only one physical core.
See https://travis-ci.org/rails/rails/jobs/469281088#L2352

@dhh dhh merged commit e9f6ce6 into rails:master Dec 18, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dhh

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

💪❤️

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:issue-34734 branch Dec 18, 2018

<%- else -%>
parallelize(workers: 2)
parallelize(workers: :number_of_processors)

This comment has been minimized.

Copy link
@matthewd

matthewd Dec 19, 2018

Member

If we're making the default an "it just works" value like this, does it make sense to still include it here? If most people don't need to override it, maybe this line can just be a plain parallelize?

This comment has been minimized.

Copy link
@bogdanvlviv

bogdanvlviv Dec 19, 2018

Author Contributor

I thought that it would be better to keep this in this form since it would be easier for users to figure out that their tests use the actual core count as a number of workes. I suppose we should either simplify this to parallelize and parallelize(with: :threads)(a few lines above), or extend it to parallelize(workers: :number_of_processors, with: :processes) to emphasize that users' tests use "fork approach".

@dhh

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Dec 19, 2018

Make test_helper.rb file more verbose about parallel testing configur…
…ation

Despite `parallelize` method without arguments makes the same things
as `parallelize(workers: :number_of_processors, with: :processes)`
we can set extended form of it by default in `test/test_helper.rb`
for newly generated rails apps.
It can be useful for users, especially for new rails users since it
would explain about itself very well.

See discussion rails#34735 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.