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

Allow TEST_ENV_NUMBER to support duplicate file groups #943

Merged
merged 6 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions lib/parallel_tests/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ def execute_in_parallel(items, num_processes, options)
Tempfile.open 'parallel_tests-lock' do |lock|
ParallelTests.with_pid_file do
simulate_output_for_ci options[:serialize_stdout] do
Parallel.map(items, in_threads: num_processes) do |item|
result = yield(item)
Parallel.map_with_index(items, in_threads: num_processes) do |item, index|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand all of this correctly, the item that gets yielded on the next line is the group in the execute_in_parallel call on line 84

If that's the case, I wonder if naming these to groups/group instead of items/item makes it easier to understand?

Copy link
Owner

Choose a reason for hiding this comment

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

execute_in_parallel is a generic method that does not always get groups passed
for example execute_in_parallel(runs, runs.size, options)
so afaik this should stay items

result = yield(item, index)
Copy link
Owner

Choose a reason for hiding this comment

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

cli.rb:376 will need a change to accept the index

Copy link
Owner

Choose a reason for hiding this comment

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

... hmm I guess no it does not since it ignores the extra arg 👍

reprint_output(result, lock.path) if options[:serialize_stdout]
ParallelTests.stop_all_processes if options[:fail_fast] && result[:exit_status] != 0
result
Expand All @@ -81,8 +81,9 @@ def run_tests_in_parallel(num_processes, options)
end

report_number_of_tests(groups) unless options[:quiet]
test_results = execute_in_parallel(groups, groups.size, options) do |group|
run_tests(group, groups.index(group), num_processes, options)
test_results = execute_in_parallel(groups, groups.size, options) do |group, index|
test_env_number = options[:first_is_1] ? index + 1 : index
Copy link
Owner

Choose a reason for hiding this comment

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

the first_is_1 logic was not here before, so it should get removed or be removed from some other place ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, good catch. I didn't trace the test_env down far enough; I see it in test/runner.rb now.

I've updated the PR to remove line 85 and just use index.

run_tests(group, test_env_number, num_processes, options)
end
report_results(test_results, options) unless options[:quiet]
end
Expand Down
39 changes: 39 additions & 0 deletions spec/parallel_tests/cli_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,45 @@ def self.it_prints_nothing_about_rerun_commands(options)
subject.run(['test', '-n', '3', '--only-group', '2,3', '-t', 'my_test_runner'])
end
end

context 'when --allow-duplicates' do
jaydorsey marked this conversation as resolved.
Show resolved Hide resolved
let(:results) { { stdout: "", exit_status: 0 } }
let(:processes) { 2 }
let(:common_options) do
{ files: ['test'], allow_duplicates: true, first_is_1: false }
end
before do
allow(subject).to receive(:puts)
expect(subject).to receive(:load_runner).with("my_test_runner").and_return(ParallelTests::MyTestRunner::Runner)
allow(ParallelTests::MyTestRunner::Runner).to receive(:test_file_name).and_return("test")
expect(subject).to receive(:report_results).and_return(nil)
end

before do
expect(ParallelTests::MyTestRunner::Runner).to receive(:tests_in_groups).and_return(
[
['foo'],
['foo'],
['bar']
]
)
end

it "calls run_tests with --only-group" do
options = common_options.merge(count: processes, only_group: [2, 3], group_by: :filesize)
expect(subject).to receive(:run_tests).once.with(['foo'], 0, 1, options).and_return(results)
expect(subject).to receive(:run_tests).once.with(['bar'], 1, 1, options).and_return(results)
subject.run(['test', '-n', processes.to_s, '--allow-duplicates', '--only-group', '2,3', '-t', 'my_test_runner'])
end

it "calls run_tests with --first-is-1" do
options = common_options.merge(count: processes, first_is_1: true)
expect(subject).to receive(:run_tests).once.with(['foo'], 1, processes, options).and_return(results)
expect(subject).to receive(:run_tests).once.with(['foo'], 2, processes, options).and_return(results)
expect(subject).to receive(:run_tests).once.with(['bar'], 3, processes, options).and_return(results)
subject.run(['test', '-n', processes.to_s, '--first-is-1', '--allow-duplicates', '-t', 'my_test_runner'])
end
end
end

describe "#display_duration" do
Expand Down
Loading