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
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,8 @@

## Unreleased

- Added support for multiple isolated processes.

### Breaking Changes

- None
Expand Down
1 change: 1 addition & 0 deletions Readme.md
Expand Up @@ -206,6 +206,7 @@ Options are:
-m, --multiply-processes [FLOAT] use given number as a multiplier of processes to run
-s, --single [PATTERN] Run all matching files in the same process
-i, --isolate Do not run any other tests in the group used by --single(-s)
--isolate-n How many processes to reserve for isolated tests. Default is 1.
--only-group INT[, INT]
-e, --exec [COMMAND] execute this code parallel and with ENV['TEST_ENV_NUMBER']
-o, --test-options '[OPTIONS]' execute test commands with those options
Expand Down
10 changes: 10 additions & 0 deletions lib/parallel_tests/cli.rb
Expand Up @@ -192,6 +192,12 @@ def parse_options!(argv)
options[:isolate] = true
end

opts.on("--isolate-n [PROCESSES]",
Integer,
"Number of processes to use for isolated singles, default: 1. Setting any positive value turns on 'isolate'.") do |n|
v-kumar marked this conversation as resolved.
Show resolved Hide resolved
options[:isolate_count] = n
end

opts.on("--only-group INT[, INT]", Array) { |groups| options[:only_group] = groups.map(&:to_i) }

opts.on("-e", "--exec [COMMAND]", "execute this code parallel and with ENV['TEST_ENV_NUMBER']") { |path| options[:execute] = path }
Expand Down Expand Up @@ -255,6 +261,10 @@ def parse_options!(argv)
raise "--group-by #{allowed.join(" or ")} is required for --only-group"
end

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?


options
end

Expand Down
33 changes: 28 additions & 5 deletions lib/parallel_tests/grouper.rb
Expand Up @@ -15,19 +15,42 @@ def in_even_groups_by_size(items, num_groups, options= {})
groups = Array.new(num_groups) { {:items => [], :size => 0} }

# add all files that should run in a single process to one group
(options[:single_process] || []).each do |pattern|
matched, items = items.partition { |item, _size| item =~ pattern }
matched.each { |item, size| add_to_group(groups.first, item, size) }
single_process_patterns = options[:single_process] || []

single_items, items = items.partition do |item, _size|
single_process_patterns.any? { |pattern| item =~ pattern }
end

groups_to_fill = (options[:isolate] ? groups[1..-1] : groups)
group_features_by_size(items_to_group(items), groups_to_fill)
isolate_count = isolate_count(options)

if isolate_count >= 1
# add all files that should run in a multiple isolated processes to their own groups
group_features_by_size(items_to_group(single_items), groups[0..(isolate_count - 1)])
# group the non-isolated by size
group_features_by_size(items_to_group(items), groups[isolate_count..-1])
else
# add all files that should run in a single non-isolated process to first group
single_items.each { |item, size| add_to_group(groups.first, item, size) }

# group all by size
group_features_by_size(items_to_group(items), groups)
end

groups.map! { |g| g[:items].sort }
end

private

def isolate_count(options)
if options[:isolate_count] && options[:isolate_count] > 1
options[:isolate_count]
elsif options[:isolate]
1
else
0
end
end

def largest_first(files)
files.sort_by{|_item, size| size }.reverse
end
Expand Down
4 changes: 2 additions & 2 deletions spec/fixtures/rails51/Gemfile.lock
@@ -1,7 +1,7 @@
PATH
remote: ../../..
specs:
parallel_tests (2.32.0)
parallel_tests (3.2.0)
parallel

GEM
Expand Down Expand Up @@ -65,7 +65,7 @@ GEM
nio4r (2.3.1)
nokogiri (1.8.5)
mini_portile2 (~> 2.3.0)
parallel (1.19.1)
parallel (1.19.2)
rack (2.0.5)
rack-test (1.1.0)
rack (>= 1.0, < 3)
Expand Down
4 changes: 2 additions & 2 deletions spec/fixtures/rails52/Gemfile.lock
@@ -1,7 +1,7 @@
PATH
remote: ../../..
specs:
parallel_tests (2.32.0)
parallel_tests (3.2.0)
parallel

GEM
Expand Down Expand Up @@ -72,7 +72,7 @@ GEM
nio4r (2.3.1)
nokogiri (1.8.5)
mini_portile2 (~> 2.3.0)
parallel (1.19.1)
parallel (1.19.2)
rack (2.0.5)
rack-test (1.1.0)
rack (>= 1.0, < 3)
Expand Down
26 changes: 26 additions & 0 deletions spec/parallel_tests/cli_spec.rb
Expand Up @@ -107,6 +107,32 @@ def call(*args)
end
end

context "single and isolate" do
it "single_process should be an array of patterns" do
expect(call(["test", "--single", '1'])).to eq(defaults.merge(single_process: [/1/]))
end

it "single_process should be an array of patterns" do
expect(call(["test", "--single", '1', "--single", '2'])).to eq(defaults.merge(single_process: [/1/, /2/]))
end

it "isolate should set isolate_count defaults" do
expect(call(["test", "--single", '1', "--isolate"])).to eq(defaults.merge(single_process: [/1/], isolate: true))
end

it "isolate_n should set isolate_count and turn on isolate" do
expect(call(["test", "-n", "3", "--single", '1', "--isolate-n", "2"])).to eq(
defaults.merge(count: 3, single_process: [/1/], isolate_count: 2)
)
end

it "isolate_n must be within limits" do
expect(subject).to receive(:abort).with("Number of isolated processes must be less than total the number of processes")

call(["test", "-n", "3", "--single", '1', "--isolate-n", "3"])
end
end

context "when the -- option separator is used" do
it "interprets arguments as files/directories" do
expect(call(%w(-- test))).to eq( files: %w(test))
Expand Down
4 changes: 4 additions & 0 deletions spec/parallel_tests/grouper_spec.rb
Expand Up @@ -54,6 +54,10 @@ def call(num_groups, options={})
expect(call(2, :single_process => [/1|2|3|4/])).to eq([["1", "2", "3", "4"], ["5"]])
end

it "groups single items into specified isolation groups" do
expect(call(3, :single_process => [/1|2|3|4/], :isolate_count => 2)).to eq([["1", "4"], ["2", "3"], ["5"]])
end

it "groups single items with others if there are too few" do
expect(call(2, :single_process => [/1/])).to eq([["1", "3", "4"], ["2", "5"]])
end
Expand Down
20 changes: 20 additions & 0 deletions spec/parallel_tests/test/runner_spec.rb
Expand Up @@ -146,6 +146,26 @@ def call(*args)

expect(valid_combinations).to include(actual)
end

it "groups by size and use specified number of isolation groups" do
skip if RUBY_PLATFORM == "java"
expect(ParallelTests::Test::Runner).to receive(:runtimes).
and_return({"aaa1" => 1, "aaa2" => 3, "aaa3" => 2, "bbb" => 3, "ccc" => 1, "ddd" => 2})
result = call(["aaa1", "aaa2", "aaa3", "bbb", "ccc", "ddd", "eee"], 4, isolate_count: 2, single_process: [/^aaa/], group_by: :runtime)

isolated_1, isolated_2, *groups = result
expect(isolated_1).to eq(["aaa2"])
expect(isolated_2).to eq(["aaa1", "aaa3"])
actual = groups.map(&:to_set).to_set

# both eee and ccs are the same size, so either can be in either group
valid_combinations = [
[["bbb", "eee"], ["ccc", "ddd"]].map(&:to_set).to_set,
[["bbb", "ccc"], ["eee", "ddd"]].map(&:to_set).to_set
]

expect(valid_combinations).to include(actual)
end
end
end

Expand Down