rspec / rspec-core

RSpec runner and formatters
http://rspec.info
MIT License
1.23k stars 765 forks source link

Move dry_run option higher in ordered option list #3008

Closed tubaxenor closed 1 year ago

tubaxenor commented 1 year ago

Currently dry_run option was set at the end of the options list, which makes RSpec.configuration.dry_run? always return false while processing required libs, especially spec_helper.rb.

For example in spec_helper.rb

RSpec.configure do |config|
  puts "Dry?: #{config.dry_run?}"
end
$ rspec --dry-run

Dry? false
pirj commented 1 year ago

@tubaxenor May I kindly ask you to describe your use case, why do you need dry_run? to be available in the configure block?

tubaxenor commented 1 year ago

@tubaxenor May I kindly ask you to describe your use case, why do you need dry_run? to be available in the configure block?

Here is an abstract version of how we use this flag:

RSpec.configure do |config|
  config.when_first_matching_example_defined(type: :system) do
    unless config.dry_run?
      require 'graphql/gateway_runner'
      Graphql::GatewayRunner.start
    end

    # Some other stuff runs at very first time a system spec was touched
  end
end
tubaxenor commented 1 year ago

can we get a spec check for this, but otherwise I'm happy with this change.

Just pushed a commit to include dry_run option ordering 🙇

pirj commented 1 year ago

when_first_matching_example_defined docs suggest performing expensive operations inside hooks:

    RSpec.configure do |config|
      config.when_first_matching_example_defined(:db) do
        require "support/db"
      end
    end

And
    a file named "spec/support/db.rb" with:

    RSpec.configure do |config|
      config.before(:suite) do
        puts "Bootstrapped the DB."
      end

      config.around(:example, :db) do |example|
        puts "Starting a DB transaction."
        example.run
        puts "Rolling back a DB transaction."
      end
    end

While --dry-run docs tell that hooks won't be run:

Use the --dry-run option to have RSpec print your suite's formatter output without running any examples or hooks.

Would it work for you like this?

tubaxenor commented 1 year ago

I probably should provide more context about this, we are using kanpsack pro in queue mode to run our specs across CI nodes, which uses dry run at the beginning of every process. And this is why we do lots of dry_run? checks to make sure we are not launching unnecessary services.

Would it work for you like this?

It could work, but the whole point here is to not running extra services (in hooks or not) in dry run, shouldn't a if config.dry_run? makes more sense? Plus it's a CLI argument, it feels surprising (at least for me 😅 ) that rspec --dry-run returns a false value when callingRSpec.confoguration.dry_run? in spec_helper or inside RSpec.configure block...

pirj commented 1 year ago

the whole point here is to not running extra services (in hooks or not) in dry run

Right. That's what I suggest, putting your Graphql::GatewayRunner.start etc inside a before(:suite) that is conditionally defined depending on the presence of examples with a certain metadata in a portion of specs.

shouldn't a if config.dry_run? makes more sense?

Seems explicit. Maybe overly. I admit I may have a bias towards the current state of things with hooks and metadata and against explicit.

it feels surprising (at least for me 😅 ) that rspec --dry-run returns a false value when calling RSpec.confoguration.dry_run? in spec_helper or inside RSpec.configure block...

Indeed. Wondering if it's the only option like this.

In addition to this, there's dry_run = ... writer. Wouldn't it be surprising that it would override the setting from the command line? Wondering if it does before or after your changes 🤔

tubaxenor commented 1 year ago

In addition to this, there's dry_run = ... writer. Wouldn't it be surprising that it would override the setting from the command line?

Yes, it does feel wrong to have dry_run as a configurable option imo, but maybe someone wants that flexibility...

Wondering if it does before or after your changes 🤔

After I think? process_options_into is the first thing happen in setup.

JonRowe commented 10 months ago

Released in 3.13.0, apologies it has taken so long.