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 all 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -7,6 +7,7 @@
### Added

### Fixed
- The `--allow-attributes` flag now runs duplicate tests in different groups
Copy link
Owner

@grosser grosser Apr 3, 2024

Choose a reason for hiding this comment

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

Suggested change
- The `--allow-attributes` flag now runs duplicate tests in different groups
- The `--allow-duplicates ` flag now runs duplicate tests in different groups


## 4.6.0 - 2024-03-25

Expand Down
8 changes: 4 additions & 4 deletions lib/parallel_tests/cli.rb
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,8 @@ 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|
run_tests(group, index, 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
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'], 0, processes, options).and_return(results)
expect(subject).to receive(:run_tests).once.with(['foo'], 1, processes, options).and_return(results)
expect(subject).to receive(:run_tests).once.with(['bar'], 2, 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