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 for duplicate tests with --allow-duplicates #940

Merged
merged 3 commits into from
Mar 25, 2024

Conversation

jaydorsey
Copy link
Contributor

@jaydorsey jaydorsey commented Mar 22, 2024

tldr; It's useful to run 1 or more tests multiple times when debugging and I'd like to propose an option to do so with parallel_tests

I often find that I'd like to run 1+ files locally, in parallel, to rule out certain flakey test behaviors (order tests are being run). Kind of like a form of "test fuzzing" I guess? The purpose is to generally rule out the test is flakey.

I often find myself doing something like this in zsh:

repeat 10 bin/rspec path/to/my_spec.rb

Sometimes it's more than 10, and sometimes it's more than 1 file I'd like to run at the same time.

I'd like to do this with parallel_tests instead and I'd like to propose this PR as a potential solution (I didn't see this functionality present with the current options)

Example usage:

export TEST_FILES="
spec/test_1.rb
spec/test_1.rb
spec/test_1.rb
spec/test_1.rb
spec/test_1.rb
"

# This is proposed optional behavior
bin/spring parallel_rspec -n 5 --allow-duplicates -- $(echo $TEST_FILES)
# 5 processes for 5 specs, ~ 1 spec per process

# This is current behavior
bin/spring parallel_rspec -n 5 -- $(echo $TEST_FILES)
# 1 process for 1 spec, ~ 1 spec per process

There's one minor caveat:

These duplicates will still be unique per process because RSpec also calls uniq in a couple places

here, here, and here

If there is interest in this PR, I'd be happy to propose a similar PR to RSpec. I think that would make this feature behave as-expected (but is still useful in parallel_tests only).

One other thought: I was also thinking some type of companion flag might be useful:

  1. --repeat <Integer>, where it multiplies the file list N times and then builds the commands.

    bin/spring parallel_rspec --repeat 100 -- path/to/my_spec.rb

  2. --repeat where it runs 1 file N times for every available or specified processor

    bin/spring parallel_rspec --repeat -- path/to/my_spec.rb

Would happy to hear any thoughts or opinions on that as well. I'd be happy to do a separate PR; it would require a similar to change to what's in this PR

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

> tldr; It's useful to run 1 or more tests multiple times when debugging
and I'd like to propose an option to do so with parallel_tests

I often find that I'd like to run 1+ files locally, in parallel, to rule
out certain flakey test behaviors (order tests are being run). Kind of
like a form of "test fuzzing" I guess? The purpose is to generally rule
out the test is flakey.

I often find myself doing something like this in zsh:

    repeat 10 bin/rspec path/to/my_spec.rb

Sometimes it's more than 10, and sometimes it's more than 1 file I'd
like to run at the same time.

I'd like to do this with parallel_tests instead and I'd like to propose
this PR as a potential solution (I didn't see this functionality present
with the current options)

Example usage:

```
export TEST_FILES="
spec/test_1.rb
spec/test_1.rb
spec/test_1.rb
spec/test_1.rb
spec/test_1.rb
"

bin/spring parallel_rspec -n 5 --allow-duplicates -- $(echo $TEST_FILES)

bin/spring parallel_rspec -n 5 -- $(echo $TEST_FILES)
```

There's one _minor_ caveat:

These duplicates will still be unique per process because RSpec _also_
calls uniq in a couple places

[here](https://github.com/rspec/rspec-core/blob/1e661db5c5b431c0ee88a383e8e3767f02dccbfe/lib/rspec/core/configuration.rb#L2202), [here](https://github.com/rspec/rspec-core/blob/1e661db5c5b431c0ee88a383e8e3767f02dccbfe/lib/rspec/core/configuration.rb#L2222), and [here](https://github.com/rspec/rspec-core/blob/1e661db5c5b431c0ee88a383e8e3767f02dccbfe/lib/rspec/core/configuration.rb#L1636)

If there is interest in this PR, I'd be happy to propose a similar
PR to the RSpec team.

One other thought: I was also thinking a companion `--repeat <Integer>`
flag could be useful, where it multiplies the file list N times
and then builds the commands.

    bin/spring parallel_rspec --repeat 100 -- path/to/my_spec.rb

Would happy to hear any thoughts or opinions on that as well. I'd
be happy to do a separate PR
@jaydorsey jaydorsey changed the title Allow for duplicate tests Allow for duplicate tests with --allow-duplicates Mar 22, 2024
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.

looks nice, thx!

@grosser
Copy link
Owner

grosser commented Mar 23, 2024

rubocop wants a word with you though :)

@grosser
Copy link
Owner

grosser commented Mar 23, 2024

I think this is good enough to be used by some other helper script to feed a bunch of tests
--repeat could still work but supports a narrower usecase, so prefer this

jaydorsey and others added 2 commits March 25, 2024 09:26
Co-authored-by: Michael Grosser <michael@grosser.it>
@jaydorsey
Copy link
Contributor Author

I integrated the change you suggested w/ the file rename and fixed the rubocop errors. I think it should be good to another look (at your leisure). Thanks!

@grosser grosser merged commit bc79c20 into grosser:master Mar 25, 2024
13 checks passed
@grosser
Copy link
Owner

grosser commented Mar 25, 2024

thx!
4.6.0

@jaydorsey
Copy link
Contributor Author

I'm going to attempt the companion rspec change as well.

I will link back to this PR once I've submitted the PR.

@jaydorsey
Copy link
Contributor Author

I found one bug/behavior that I need to solve: tests are grouped by an array of filename, and then the index of that group is used as a lookup to decide the TEST_ENV_NUMBER

# for example:
groups = ['file1.rb', 'file2.rb', 'file1.rb']

# actual code
test_results = execute_in_parallel(groups, groups.size, options) do |group|
  run_tests(group, groups.index(group), num_processes, options)
end

It puts both file1.rb as TEST_ENV_NUMBER=0 because groups.index(group) always finds the first instance.

I need to spend a little bit more time with this but I'll come up with a solution for this that doesn't break anything. The allow-duplicates isn't quite as useful without "fixing" this because it tries to use the same database number for every instance of the same spec.

@grosser
Copy link
Owner

grosser commented Mar 28, 2024 via email

@jaydorsey
Copy link
Contributor Author

lmk if it's revert worthy or just "not perfect"

I don't think we need a revert yet, I have some time this weekend to try and fix it. If I can't get some tests and a draft solution by the end of the weekend, I can do a PR to roll it back.

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