Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Move dry_run option higher in ordered option list #3008

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

tubaxenor
Copy link
Contributor

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
Copy link
Member

pirj commented Feb 22, 2023

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

Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

I'd reword the explanation slightly and can we get a spec check for this, but otherwise I'm happy with this change.

@tubaxenor
Copy link
Contributor Author

@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 tubaxenor requested a review from JonRowe February 22, 2023 12:02
@tubaxenor
Copy link
Contributor Author

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
Copy link
Member

pirj commented Feb 22, 2023

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
Copy link
Contributor Author

tubaxenor commented Feb 22, 2023

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
Copy link
Member

pirj commented Feb 22, 2023

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
Copy link
Contributor Author

tubaxenor commented Feb 22, 2023

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 JonRowe merged commit 36dd88d into rspec:main Feb 22, 2023
JonRowe added a commit that referenced this pull request Feb 22, 2023
@tubaxenor tubaxenor deleted the reorder-config-options branch February 22, 2023 22:37
@JonRowe
Copy link
Member

JonRowe commented Feb 4, 2024

Released in 3.13.0, apologies it has taken so long.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants