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

Update runner.rb - A suggested fix for 'The command line is too long.' in Windows when the number of tests are huge #981

Closed
wants to merge 5 commits into from

Conversation

rajaramaniyer
Copy link

A suggested fix for 'The command line is too long.' in Windows when the number of tests are huge

Thank you for your contribution!

Checklist

  • Feature branch is up-to-date with master (if not - rebase it).
  • Added tests.
  • Added an entry to the Changelog if the new
    code introduces user-observable changes.
  • Update Readme.md when cli options are changed

A suggested fix for 'The command line is too long.' in Windows when the number of tests are huge
Copy link
Owner

@grosser grosser left a comment

Choose a reason for hiding this comment

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

way more complicated then I expected,
so might be best if this is only activated when needed (windows only or flag, when command is too long) to ease debugging for the 99% codepath and prevent bugs for other usecases
also needs some basic tests

lib/parallel_tests/test/runner.rb Outdated Show resolved Hide resolved
lib/parallel_tests/test/runner.rb Outdated Show resolved Hide resolved
lib/parallel_tests/test/runner.rb Outdated Show resolved Hide resolved
lib/parallel_tests/test/runner.rb Show resolved Hide resolved
lib/parallel_tests/test/runner.rb Outdated Show resolved Hide resolved
lib/parallel_tests/test/runner.rb Outdated Show resolved Hide resolved
def process_in_batches(cmd, os_cmd_length_limit, tests)
# Filter elements not starting with value in tests to retain in each batch
# i.e. retain common parameters for each batch
retained_elements = cmd.reject { |s| s.start_with?(tests) }
Copy link
Owner

Choose a reason for hiding this comment

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

isn't tests files[:first] so just a file and not a prefix ?

Copy link
Author

Choose a reason for hiding this comment

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

when we start the process we give features/ and files[:first] has that .. which has been expanded into individual files into cmd array

Copy link
Owner

Choose a reason for hiding this comment

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

ahh ... yeah that's a bit brittle, user could be giving multiple folders too ... I'll see if I can find a better way, otherwise keeping with a comment

Copy link
Owner

Choose a reason for hiding this comment

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

won't work with minitest since that uses requires and not a list of files
"%w[#{require_list}].each { |f| require %{./\#{f}} }",

lib/parallel_tests/test/runner.rb Outdated Show resolved Hide resolved
@grosser
Copy link
Owner

grosser commented Dec 30, 2024

I'll make a new PR with some more changes + tests

@grosser grosser closed this Dec 30, 2024
@@ -99,7 +134,24 @@ def execute_command(cmd, process_number, num_processes, options)

print_command(cmd, env) if report_process_command?(options) && !options[:serialize_stdout]

execute_command_and_capture_output(env, cmd, options)
result = []
process_in_batches(cmd, 8191, options[:files].first).map do |subcmd|
Copy link
Owner

Choose a reason for hiding this comment

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

I don't get the options[:files].first) part, why is the first file the suffix for all other files ?
(tested with start_with? in process_in_batches)

Copy link
Owner

Choose a reason for hiding this comment

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

... I'd split the commands at the first file

Copy link
Author

Choose a reason for hiding this comment

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

the common parameters can appear after the files as well .. hence i was doing a starts_with ..

Copy link
Owner

Choose a reason for hiding this comment

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

thx, updated in my draft, plz see if that looks alright

@grosser
Copy link
Owner

grosser commented Dec 30, 2024

lmk if #982 looks good then I'll add some tests
... it's going to have lots of edge-cases that don't work, but for rspec/cucumber it should kinda be fine

@grosser grosser mentioned this pull request Dec 30, 2024
@grosser
Copy link
Owner

grosser commented Dec 30, 2024

tried a new approach #984
... it uses max files instead of max chars, but I think this is more stable since we don't have to cut commands apart but just build more commands, lmk what you think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants