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 passing through RSpec arguments #47

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
16 changes: 15 additions & 1 deletion bin/rspecq
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,19 @@ else
redis_opts[:host] = opts[:redis_host]
end

# Use the RSpec parser to parse any command line args intended for rspec such
# 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

files_or_dirs_to_run = RSpec::Core::Parser.new(ARGV).parse[:files_or_directories_to_run]
if files_or_dirs_to_run.length.zero?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we simply do if files_or_dirs_to_run.any??

opts[:rspec_args] = ARGV
else
opts[:rspec_args] = ARGV[0...-files_or_dirs_to_run.length]
opts[:files_or_dirs_to_run] = files_or_dirs_to_run
end

if opts[:report]
reporter = RSpecQ::Reporter.new(
build_id: opts[:build],
Expand All @@ -139,7 +152,8 @@ else
redis_opts: redis_opts
)

worker.files_or_dirs_to_run = ARGV[0] if ARGV[0]
worker.rspec_args = opts[:rspec_args]
worker.files_or_dirs_to_run = opts[:files_or_dirs_to_run] if opts[:files_or_dirs_to_run]
worker.populate_timings = opts[:timings]
worker.file_split_threshold = opts[:file_split_threshold]
worker.max_requeues = opts[:max_requeues]
Expand Down
8 changes: 7 additions & 1 deletion lib/rspecq/worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ class Worker
# Defaults to 0
attr_accessor :fail_fast

# Optional arguments to pass along to rspec.
#
# Defaults to nil
attr_accessor :rspec_args

attr_reader :queue

def initialize(build_id:, worker_id:, redis_opts:)
Expand Down Expand Up @@ -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]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

opts = RSpec::Core::ConfigurationOptions.new(args)
_result = RSpec::Core::Runner.new(opts).run($stderr, $stdout)

queue.acknowledge_job(job)
Expand Down
4 changes: 4 additions & 0 deletions test/sample_suites/tagged_suite/spec/tagged_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
RSpec.describe do
it("foo", :foo) { expect(true).to be true }
it("bar", :bar) { expect(true).to be true }
end
6 changes: 6 additions & 0 deletions test/test_e2e.rb
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,10 @@ def test_suite_with_failures_and_fail_fast

assert_includes [2, 3], queue.processed_jobs.length
end

def test_suite_with_rspec_arguments
queue = exec_build("tagged_suite", "-- --tag foo")

assert_equal 1, queue.example_count
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

  1. 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).
  2. we could assert the actual contents of queue.processed_jobs
  3. 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

end