Respect RSpec default_path for generators

[vivekmiyani]

Fixes #2507.

TODO

• Replace hardcoded spec directory to default_path
• Change log entry
• Add feature test for each generator listed below:
  • channel
  • controller
  • feature
  • generator
  • helper
  • install
  • integration
  • job
  • mailbox
  • mailer
  • model
  • request
  • scaffold
  • system
  • view

[pirj]

Something failed

[vivekmiyani]

Hii, I've updated the branch.

At the moment, rails installer does not support --default_path, So lib/generators/rspec/install/install_generator.rb remains untouched.
I think to change the default_path at the time of rails generate rspec:install, We need to do modify the .rspec file provided by rspec-core.

I'll work on it when get a chance.

[pirj]

Looks great, thank you. You may (completely optionally) add a feature to features/generator_specs/generator_specs.feature.

Just noticed that our Generators documentation is split into two, and the second one's name is misleading, Generator specs. It's a topic for another PR to bring them together, though.

[vivekmiyani]

You may (completely optionally) add a feature to features/generator_specs/generator_specs.feature.

@pirj Are we only going to test features/generator_specs/generator_specs.feature for different default_path and not others features. ?

Forgive me if I misunderstood anything 😅 .

[pirj]

Yes, just your fix. Those features are published as documentation, and are end-to-end tests. No worries if you get stuck with it. Altering the default spec path is a pretty rare case. Also, with your fix it now works intuitively, so maybe documentation for it is redundant. Leaving it up to you to decide.

[vivekmiyani]

Okay I will update the PR. But while implementing the feature I am running into one issue.:

I thought that default_path will be initialized from .rspec OR spec_helper.rb at the time of running rails generate ... but that is not the case with actual project.

It looks the blocker is the ConfigurationOptions which was parsing options when I ran bundle exec rspec but not for rails generate ....

[vivekmiyani]

We have to do something like, https://github.com/rspec/rspec-core/blob/main/lib/rspec/core/runner.rb#L132 (i.e. RSpec::Core::Runner#configure) is doing the same as we need here.

Any thoughts or suggestion?

[pirj]

It's all kind of weird. If I'm not mistaken, the doc ‎https://relishapp.com/rspec/rspec-core/v/3-9/docs/configuration/setting-the-default-spec-path says that `default_path` is not supported on `RSpec.configuration`, but it seems that it is. The reasoning is not exactly clear. Let me check later when I get back to the computer.

[pirj]

--default-path ... is not supported on RSpec.configuration, as it needs to be set before spec files are loaded

But https://github.com/rspec/rspec-core/blob/247d0a706ffa955718d600f3588aeb1e60cebde4/lib/rspec/core/configuration.rb#L35

    #     RSpec.configure do |c|
    #       c.default_path = 'behavior'

and

      # Path to use if no path is provided to the `rspec` command (default:
      # `"spec"`). Allows you to just type `rspec` instead of `rspec spec` to
      # run all the examples in the `spec` directory.

and what's more important:

      # @note Other scripts invoking `rspec` indirectly will ignore this
      #   setting.

which is exactly the case with generators.

Though .rspec should support setting the default path with:

--default-path behaviour

Can you please confirm that defining the default path in .rspec works with this fix for generators?
Wondering if it gets loaded when you require 'rspec/coreas you've added. And whyspec/spec_helper.rbis not loaded, even though a typical.rspecfile defines--require spec/spec_helper.rb`. 🤔

If it works, I suggest you to write the feature test in such a way that the default path is set in the .rspec file, not spec/spec_helper.rb.

[pirj]

I think I have bad news. Options from files, .rspec etc are loaded by ConfigurationOptions that is exclusively called by Runner that is responsible for running specs, and only immediately before running specs.
spec/spec_helper.rb is required after that if .rspec has a --require spec_helper option.
So require 'rspec/core' only loads up the default Configuration with default_path set to spec. Alas.

[vivekmiyani]

Yes that's what I exactly found. See my #2508 (comment) I've mentioned the line no. where ConfigurationOptions#configure updates the Configuration.

I will try to find more and will update the PR.

[JonRowe]

I'd like to see a feature file demonstrating this behaviour so that its both documented and tested, the changes to the generator look fine but I'm not sure its sufficient to load rspec/core before accessing configuration, the config must have been loaded / set before being accessed.

[vivekmiyani]

@JonRowe @pirj I will implement feature file.
but please check the ca907cf commit, I've added temporary workaround to parse the .rspec file (Extracted from the rspec/core/runner.rb).

Can we initialize ConfigurationOptions when Configuration initializes in rspec-core, So separate initialization of ConfigurationOptions can be removed.?

[pirj]

I believe with those discoveries, adding a feature test becomes a necessity.

[vivekmiyani]

Hey,
I have added rspec install option:

# Install rspec at custom directory (this will append `--default-path` option to `.rspec` file)
$ rails generate rspec:install --default-path behaviour

Also look at 5f7b640 which implements the feature you've asked for!!

[vivekmiyani]

@pirj @JonRowe Do you want me to write the feature for all other generators (like: controller, model, etc.. etc..)?

I've only added feature for generator generator, as I mentioned above.

[pirj]

The code is copied over from rspec-core. It might be exposed as a public interface, but this may be postponed to a later version, and we can do this in rspec-core 4.0.1, and set the minimum dependent version to it.

Thank you!

[vivekmiyani]

@pirj Can we initialize ConfigurationOptions in Configuration constructor directly in rspec-core, So separate initialization of ConfigurationOptions can be completely removed from codebase.?

[pirj]

That's an option to consider, too. RSpec.configuration is memoized, though:

  def self.configuration
    @configuration ||= RSpec::Core::Configuration.new
  end

and there could be cases when it's necessary to run a suite with different options, e.g. filters. It would make ConfigurationOptions class redundant, and some may make use of it. It could be done in a major rspec-core version, but we'd prefer not to do this in RSpec 4.

I don't feel that this code duplication is a big issue, it just hints that something can be optimized.

[JonRowe]

Sorry for the delayed feedback, looking good but some more tweaks needed.

Code review notes addressed, proposed changes accepted

[JonRowe]

This changes behaviour for all generators, yet is only tested against one generator. This needs the additional tests and scenarios.

[vivekmiyani]

Hello guys, Sorry for delayed response. I was super busy in some stuffs.

As @JonRowe asked, I am writing feature tests for below items (List grabbed from spec/generators/rspec/**/*_spec.rb):

• channel
• controller
• feature
• generator
• helper
• install
• integration
• job
• mailbox
• mailer
• model
• request
• scaffold
• system
• view

EDIT: Added the entry of generator feature in TODO list.

[vivekmiyani]

@pirj @JonRowe Check channel generator feature, is it correct OR any modification needed?
If it is correct I'll repeat for others.

[pirj]

channel generator feature, is it correct

Looks good to me 👍
Thanks for getting back to this.

[vivekmiyani]

Hey @pirj @JonRowe, Added more feature tests 🎉 .

Below are not implemented as I couldn't figure how to tackle them 🤕 :

• Install generator feature, Because I' not able to run the installation using When I run 'bundle exec rails generate rspec:install' in feature, as our directory has already installed rspec structure.
• Model generator feature, Because I am stuck at matching the migration timestamp output 🤔 . How to match it in Then the output should contain block?.
• Scaffold generator feature, Same I am stuck at matching the migration timestamp output.

[JonRowe]

@pirj can you review this?

[vivekmiyani]

@JonRowe @pirj Can you please review this?

Requested changes done

[pirj]

Thank you!