Allow passing through RSpec arguments #47
Conversation
|
This is obviously my first potential contribution to the project, but I'm looking at using this to replace
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 |
There was a problem hiding this comment.
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, andRSpecQ::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.
| # 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. |
There was a problem hiding this comment.
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.
| queue = exec_build("tagged_suite", "-- --tag foo") | ||
|
|
||
| assert_equal 1, queue.example_count | ||
| end |
There was a problem hiding this comment.
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
barexample 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
| # 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? |
There was a problem hiding this comment.
Can we simply do if files_or_dirs_to_run.any??
lib/rspecq/worker.rb
Outdated
| @@ -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] | |||
There was a problem hiding this comment.
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
|
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. |
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. |
This uses
RSpec::Core::Parserto parseARGVafter parsing it for RSpecQ arguments. So this allowsrspecq --build foo --worker bar -- --pattern baz --tag fizz:buzz fileswithout changing the existing API ofrspecq --build foo --worker bar files.This fixes #23.