Allow passing through RSpec arguments #47

jacobthemyth · 2020-12-06T23:10:39Z

This uses RSpec::Core::Parser to parse ARGV after parsing it for RSpecQ arguments. So this allows rspecq --build foo --worker bar -- --pattern baz --tag fizz:buzz files without changing the existing API of rspecq --build foo --worker bar files.

This fixes #23.

bin/rspecq

fragoulis

Thank you for the contribution. We will take a thorough look shortly!

jacobthemyth · 2020-12-07T15:05:07Z

This is obviously my first potential contribution to the project, but I'm looking at using this to replace ci-queue for a very large test suite with 10s of thousands of tests with lots of flakes. A few questions for future contributions:

it doesn't look like there's a contribution guide, is there anything about this PR that you'd like me to change in the future?
are you open to pure refactoring PRs? I have some work locally to extract the option and environment parsing into objects that mirror RSpec's internals with RSpecQ::Parser, RSpecQ::ConfigurationOptions, and RSpecQ::Configuration, but didn't want to include a huge diff in this change.
I'm interested in making certain things pluggable in rspecq, especially the flaky test reporting so that we can quarantine flaky tests and open Jira tickets for them sort of like https://github.com/flexport/quarantine. My thought is to just modify the reporter to allow plugging in an arbitrary Ruby script, would that approach be accepted upstream do you think?

Thanks for the great library! I was about to write my own until I found this, and the code is much much nicer to read and understand than ci-queue in my opinion since it uses formatters instead of monkey patching everything 💚.

agis

Thanks for the contribution and the kind words @systemist! 👏🏽 It's great to hear that you're finding RSpecQ useful.

Overall it's very useful to accept whatever flags RSpec accepts, and even nicer if we can do so transparently, with minimum effort. In that sense, this seems like a great addition to me.

In that sense, I think this deserves a mention in the README with an accompanying example in the "Usage" section. Also, rspecq -h should be updated to mention that there's an alternative form of running rspecq, so something like this:

USAGE:
    rspecq [<options>] [spec files or directories]
    rspecq [<options>] -- [<rspec options>] [spec files or directories]

Moving on to your questions:

it doesn't look like there's a contribution guide, is there anything about this PR that you'd like me to change in the future?

We should really add a contribution guide. I'll try to do it sometime soon. Regarding this PR, there's nothing else that should be changed, besides the comments I've made.

are you open to pure refactoring PRs? I have some work locally to extract the option and environment parsing into objects that mirror RSpec's internals with RSpecQ::Parser, RSpecQ::ConfigurationOptions, and RSpecQ::Configuration, but didn't want to include a huge diff in this change.

It's better to do these as separate PRs, so you certainly did good for not including such a change in this current PR. In general, I'd say the answer is "it depends". It really comes down to the value that a refactoring brings. Since abstractions generally come at a cost, so I'd avoid introducing extra classes if they don't facilitate a new feature or a bug fix or something similar. I think it's important to keep things simple, unless there's a tradeoff that's worthy.

I'm interested in making certain things pluggable in rspecq, especially the flaky test reporting so that we can quarantine flaky tests and open Jira tickets for them sort of like https://github.com/flexport/quarantine. My thought is to just modify the reporter to allow plugging in an arbitrary Ruby script, would that approach be accepted upstream do you think?

Something like a callback-based approach seems definitely doable.

P.S. I've enabled Discussions so feel free to jump in if there are any questions.

agis · 2020-12-15T10:57:14Z

bin/rspecq

+# as `-- --format JUnit -o foo.xml` so that we can pass these args to rspec
+# while removing the files_or_dirs_to_run since we want to pull those from the
+# queue. OptionParser above mutates ARGV, so only options after `--`  or
+# non-flag arguments (such as files) will make it to this point.


So there are some disparities compared to what the rspec binary accepts, which makes me a bit skeptical about this approach.

For example, the following works in RSpec:

hyu:~/dev/rspecq [(HEAD detached at kajabi/master)] $ bundle exec rspec test/sample_suites/passing_suite/ --format progress . Finished in 0.00115 seconds (files took 0.06622 seconds to load) 1 example, 0 failures

while the same doesn't work with RSpecQ:

hyu:~/dev/rspecq [(HEAD detached at kajabi/master)] $ bundle exec bin/rspecq -w $RANDOM -b $RANDOM -- test/sample_suites/passing_suite/ --format progress Working for build 6541 (worker=6155) No timings found! Published queue in random order (size=1) Executing ./test/sample_suites/passing_suite/spec/foo_spec.rb bundler: failed to load command: bin/rspecq (bin/rspecq) ArgumentError: Formatter '--format' unknown - maybe you meant 'documentation' or 'progress'?. /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/formatters.rb:184:in `find_formatter' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/formatters.rb:152:in `add' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/configuration.rb:965:in `add_formatter' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/configuration_options.rb:118:in `block in load_formatters_into' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/configuration_options.rb:118:in `each' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/configuration_options.rb:118:in `load_formatters_into' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/configuration_options.rb:24:in `configure' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:132:in `configure' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:99:in `setup' /home/hyu/.gem/ruby/2.5.5/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:86:in `run' /home/hyu/dev/rspecq/lib/rspecq/worker.rb:117:in `block in work' /home/hyu/dev/rspecq/lib/rspecq/worker.rb:79:in `loop' /home/hyu/dev/rspecq/lib/rspecq/worker.rb:79:in `work' bin/rspecq:161:in `<top (required)>'

That said, putting the flags before the files to execute works fine.

agis · 2020-12-15T11:06:44Z

test/test_e2e.rb

+    queue = exec_build("tagged_suite", "-- --tag foo")
+
+    assert_equal 1, queue.example_count
+  end


To be on the safe side, I think we should test for more things here. A few thoughts:

we could have the "bar" example fail, and assert that the build is still successful in the end (i.e. the bar example was filtered out).

we could assert the actual contents of queue.processed_jobs

we could cover more surface area by using a more complex combination of flags, for example: --tag foo --format progress --format documentation --output foo.txt --no-color --backtrace --profile 2 spec/tagged_spec.rb

agis · 2020-12-15T11:41:13Z

bin/rspecq

+# queue. OptionParser above mutates ARGV, so only options after `--`  or
+# non-flag arguments (such as files) will make it to this point.
+files_or_dirs_to_run = RSpec::Core::Parser.new(ARGV).parse[:files_or_directories_to_run]
+if files_or_dirs_to_run.length.zero?


Can we simply do if files_or_dirs_to_run.any??

agis · 2020-12-15T11:49:54Z

lib/rspecq/worker.rb

@@ -107,7 +112,8 @@ def work
          RSpec.configuration.add_formatter(Formatters::JobTimingRecorder.new(queue, job))
        end

-        opts = RSpec::Core::ConfigurationOptions.new(["--format", "progress", job])
+        args = [*rspec_args, "--format", "progress", job]


Now that we accept a --format argument, having an additional, mandatory --format progress doesn't seem right. Could we, instead of setting it here, set it in bin/rspecq as a default value?

Add support for outputting junit formatted xml for CI

kyler-instructure · 2021-07-15T16:50:44Z

Is there any chance that this PR will be moved forward? This work looks great @systemist and would be hugely beneficial to support rspec arguments. Is there any assistance needed to push this through and get it merged?

fragoulis · 2021-07-16T06:45:43Z

Is there any chance that this PR will be moved forward? This work looks great @systemist and would be hugely beneficial to support rspec arguments. Is there any assistance needed to push this through and get it merged?

Really sorry but this needs to be reviewed thoroughly and the team is currently overwhelmed with other projects. I don't see us working on rspecq this quarter. Feel free to use this branch in your gemfile or any other solution really.

The project is not unmaintained or forgotten. We use it in our ci pipeline. It is just not currently prioritized. Sorry again for the delays.

Y.

jacobthemyth · 2021-07-16T12:18:54Z

Is there any assistance needed to push this through and get it merged?

Not really on my end, I just completely forgot about this PR 🤦. We’re using this branch in CI so that’s an option, it’s been working for us for many months, though it’s obviously not ideal. I’ll try to find some time next week to address the feedback that way when the maintainers have more time to review again we can hopefully move forward.

jacobthemyth force-pushed the master branch from 6cbc1a0 to ba6c5c3 Compare December 6, 2020 23:11

fragoulis reviewed Dec 7, 2020

View reviewed changes

bin/rspecq Outdated Show resolved Hide resolved

fragoulis reviewed Dec 7, 2020

View reviewed changes

Pass through RSpec arguments except files

75f5d36

jacobthemyth force-pushed the master branch from ba6c5c3 to 75f5d36 Compare December 7, 2020 14:57

rwc9u added 7 commits December 10, 2020 01:10

WIP formatter output with job id

a20a635

Add suite to filename when option is set.

28c576c

Fix counter.

2d08373

Add junit formatter.

68e4442

Cleanup debugging.

8a3e7df

Add support for rspec parallel multiple files.

8687720

Add ability to define formatter file name and path.

74202f3

agis reviewed Dec 15, 2020

View reviewed changes

rwc9u added 2 commits December 15, 2020 11:29

Rename junit-formatter option to junit-output.

b9c5763

Merge pull request #1 from Kajabi/add-job-to-junit-file

5316d7c

Add support for outputting junit formatted xml for CI

fragoulis force-pushed the master branch from 6ddca8b to 281c4d2 Compare April 8, 2021 10:04

jorge-wonolo mentioned this pull request Nov 9, 2021

Allow filtering by tags #69

Merged

littlemight mentioned this pull request Jul 28, 2022

MF Test Platform Init moneyforward/rspecq#1

Merged

prsimp added 2 commits August 23, 2022 12:01

Fixes gemspec dependency ordering

8c714a1

Merge remote-tracking branch 'upstream/master'

d5c165d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing through RSpec arguments #47

Allow passing through RSpec arguments #47

jacobthemyth commented Dec 6, 2020 •

edited

Loading

fragoulis left a comment

jacobthemyth commented Dec 7, 2020

agis left a comment •

edited

Loading

agis Dec 15, 2020

agis Dec 15, 2020

agis Dec 15, 2020

agis Dec 15, 2020

kyler-instructure commented Jul 15, 2021

fragoulis commented Jul 16, 2021

jacobthemyth commented Jul 16, 2021

Allow passing through RSpec arguments #47

Are you sure you want to change the base?

Allow passing through RSpec arguments #47

Conversation

jacobthemyth commented Dec 6, 2020 • edited Loading

fragoulis left a comment

Choose a reason for hiding this comment

jacobthemyth commented Dec 7, 2020

agis left a comment • edited Loading

Choose a reason for hiding this comment

agis Dec 15, 2020

Choose a reason for hiding this comment

agis Dec 15, 2020

Choose a reason for hiding this comment

agis Dec 15, 2020

Choose a reason for hiding this comment

agis Dec 15, 2020

Choose a reason for hiding this comment

kyler-instructure commented Jul 15, 2021

fragoulis commented Jul 16, 2021

jacobthemyth commented Jul 16, 2021

jacobthemyth commented Dec 6, 2020 •

edited

Loading

agis left a comment •

edited

Loading