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

Support parallel_test options: pattern, exclude-pattern & group-by #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

inkstak
Copy link

@inkstak inkstak commented Sep 19, 2023

This PR add suport for some parallel_test options:

    -p, --pattern [PATTERN]          run tests matching this regex pattern
        --exclude-pattern [PATTERN]  exclude tests matching this regex pattern
        --group-by [TYPE]            group tests by:
                                     found - order of finding files
                                     steps - number of cucumber/spinach steps
                                     scenarios - individual cucumber scenarios
                                     filesize - by size of the file
                                     runtime - info from runtime log
                                     default - runtime when runtime log is filled otherwise filesize

@inkstak
Copy link
Author

inkstak commented Sep 19, 2023

There could be one issue around --group-by default:
it relies on private methods from parallel_tests : https://github.com/serpapi/turbo_tests/pull/41/files#diff-36313b614b64b302e0ceec80e7a9a2dbaea194523dc46a44b7fffda7741674a5R109

@inkstak
Copy link
Author

inkstak commented Sep 19, 2023

I would also suggest another PR to handle properly Fuubar #23 depending on --pattern and --exclude-pattern.
It'll also required one of those private methods.

Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, @inkstak!

Let me think if we want turbo_tests to depend more on parallel_tests.

@kalashnikovisme
Copy link

kalashnikovisme commented Dec 15, 2023

Hey, @ilyazub! Do you have a decision about the level of dependency on parallel_tests? 🙂

NOTE: as I know, these options belong to rspec.

@steobrien
Copy link

+1, this doesn't increase the dependency on parallel_tests (although this gem is a drop-in replacement for that, so should support anyway), because --pattern and --exclude-pattern are RSpec flags

Any chance you could merge, @ilyazub?

@inkstak
Copy link
Author

inkstak commented Dec 21, 2023

FYI: --pattern & --exclude-pattern are both supported by RSpec & parallel_tests but they don't use the same implementation and doesn't support the same arguments.

For example, we use a CI script to launch our specs :

if parallel
  command = "bundle exec parallel_rspec"
  command = "bundle exec turbo_tests" if parallel == "turbo_tests"

  command += " --exclude-pattern spec/system" if arg == "unit"
  command += " --pattern spec/system"         if arg == "system"
else
  command = "bundle exec rspec"

  command += " --exclude-pattern 'system/**/*_spec.rb'" if arg == "unit"
  command += " --pattern 'system/**/*_spec.rb'"         if arg == "system"
end

In this PR, I forward those arguments to parallel_tests so it uses its implementation and not the one from RSpec.

Copy link
Collaborator

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

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

@inkstak @kalashnikovisme @steobrien Thanks for your work and sorry for not reviewing this PR sooner.

Let's record runtime log only when --group-by runtime is passed.

Because we want to support most of the ParallelTests options, may we call ParallelTests::CLI.send(:parse_options!, @argv) in TurboTests::CLI#run?

parallel_options = {}

lib/turbo_tests/runner.rb Outdated Show resolved Hide resolved
lib/turbo_tests/runner.rb Outdated Show resolved Hide resolved
lib/turbo_tests/runner.rb Show resolved Hide resolved
@ilyazub ilyazub mentioned this pull request Jan 17, 2024
@inkstak
Copy link
Author

inkstak commented Apr 4, 2024

Because we want to support most of the ParallelTests options, may we call ParallelTests::CLI.send(:parse_options!, @argv) in TurboTests::CLI#run?

PR updated ! You can pass any options supported by parallel_tests after -- :

bundle exec turbo_tests -n 4 -- --only-group 1 --pattern spec/system

Because OptionParser raises an exception on unknown options, I found it easier to split options like this.
Parallel_tests suggests the same syntax to pass test options :

parallel_rspec -- -t acceptance -f progress -- spec/foo_spec.rb spec/acceptance

)

setup_tmp_dir

subprocess_opts = {
record_runtime: use_runtime_info
record_runtime: @parallel_options[:group_by] == :runtime
Copy link
Author

@inkstak inkstak Apr 4, 2024

Choose a reason for hiding this comment

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

Let's record runtime log only when --group-by runtime is passed.

Not sure if this line is still relevant...

@darokel
Copy link

darokel commented May 2, 2024

Thanks for all your work on this. @ilyazub Is this close to being merged? @ilyazub looks like the changes have been addressed - are we good to merge this?

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.

5 participants