-
Notifications
You must be signed in to change notification settings - Fork 495
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
Changes from 4 commits
22c9aeb
5293cff
470801e
5ed8475
fa326ef
034cc49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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| | ||
result = yield(item, index) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cli.rb:376 will need a change to accept the index There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 | ||
|
There was a problem hiding this comment.
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 thegroup
in theexecute_in_parallel
call on line 84If that's the case, I wonder if naming these to
groups
/group
instead ofitems
/item
makes it easier to understand?There was a problem hiding this comment.
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