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

Make --require spec_helper the default unless otherwise specified #2700

Open
ioquatix opened this issue Feb 23, 2020 · 21 comments
Open

Make --require spec_helper the default unless otherwise specified #2700

ioquatix opened this issue Feb 23, 2020 · 21 comments

Comments

@ioquatix
Copy link
Contributor

Would it make sense to make --require spec_helper the default unless otherwise specified? It's such a common pattern.

@JonRowe
Copy link
Member

JonRowe commented Feb 24, 2020

It's in the default .rspec file generated by install, I'd lean away from making it any more magic...

@ioquatix
Copy link
Contributor Author

I understand your point of view.

It's just that every project has this file, with basically just this one line, and if it's not there things break, so it seems like a defacto standard. It also forces every project to have a .rspec just to follow the conventions. I think if the spec_helper.rb file exists, it should just be required by default. Then at least in my case 99% of the .rspec files could be removed.

@JonRowe
Copy link
Member

JonRowe commented Feb 24, 2020

It's just that every project has this file, with basically just this one line, and if it's not there things break, so it seems like a defacto standard.

Except the ones that don't, or the ones that choose to require 'rails_helper' all the time instead, etc. It is a defacto standard, but it is by configuration. Think about people who have multiple spec helpers doing different things. Auto requiring it would break their configuration.

It also forces every project to have a .rspec just to follow the conventions.

It doesn't, it is a connivence to allow you to omit it from your spec files. You also don't have to have a spec_helper.rb at all. You also can have one .rspec file in your home folder that does it, etc etc.

@ioquatix
Copy link
Contributor Author

All fair points.

Except the ones that don't, or the ones that choose to require 'rails_helper' all the time instead,

Understood. But at least in this very specific case, shouldn't be an issue since this isn't spec_helper.

Think about people who have multiple spec helpers doing different things. Auto requiring it would break their configuration.

IMHO, these users are the exception, they shouldn't use spec_helper so they don't invoke the proposed default behaviour. To me, if users have spec_helper but don't use it in the expected way, it's super confusing.

It doesn't, it is a convenience to allow you to omit it from your spec files.

It is convenient, but it's also required in cases where it's setup that way. i.e. a missing .rspec file would break the specs.

Maybe this is a strong argument for putting require 'spec_helper' at the top of every spec.

You also can have one .rspec file in your home folder that does it.

That sounds very risky :p

I'm happy for you to close this issue if you don't think there is merit in considering it further. However, to me, this is an established convention, and it would be good to thing about how we can make rspec more canonical without default configuration files. Establishing conventions like this make it easier for people to do the right thing.

@JonRowe
Copy link
Member

JonRowe commented Feb 25, 2020

I sort of see your point, after all we have a couple of magic files, (home folder .rspec, project level .rspec etc) so why not have our "default" configuration file required automatically and ignored if not present (although then we'd probably want a warning unless there was a configuration option added to the... er... .rspec to surpress it).

I'm leaving this open for community feedback, I wouldn't change this until RSpec 4 however, as it is a change in behaviour I feel could catch people out, which is fair enough on a major version change, less so on a minor.

it would be good to think about how we can make rspec more canonical without default configuration files.

Totally irrelevant aside, I found this amusing, how to make the default configuration file more default? 🙃

@ioquatix
Copy link
Contributor Author

So, I guess --require spec_helper is an option many people use. And I respect your need to avoid breaking backwards compatibility. So here is what I would explore:

Firstly, I don't like manipulating $LOAD_PATH, so I'd probably avoid doing that and instead I'd say given a spec file, spec/..._spec.rb, look up the directory for rspec.rb and if you see that file, require it.

spec_helper.rb is only rspec specific by convention, but not by namespace. So, I'd suggest you use rspec.rb.

So you'd have:

project/spec/rspec.rb # contains configuration
project/spec/my_spec.rb # contains tests

The rspec command would be responsible for loading this file. I suggest just a simple directory traversal, e.g.

while true
  path = File.dirname(spec_path)
  rspec_path = File.join(path, "rspec.rb")
  if File.exist?(rspec_path)
    return rspec_path
  end
  spec_path = File.dirname(spec_path)
  break if path == spec_path
end

However that introduces potential mutable state as you execute specs... so maybe another option would be to consider if rspec.rb or spec/rspec.rb exists in the current directory. Does the rspec command change the working directory?

@ioquatix
Copy link
Contributor Author

Another question I have is how .rspec works.

If I'm in a different directory, e.g.

cd project2
rspec ../project1/spec/**/*_spec.rb

Will it load project2/.rspec or project1/.rspec? Because to me, if it loads project2/.rspec then I think we have a bigger issue that needs to be addressed. The main point being, specs should to a certain extent have a deterministic environment based on the directory hierarchy they are in.

@pirj
Copy link
Member

pirj commented Feb 26, 2020

Just my 2c. .rspec by default requires spec_helper, and those other specs that need more usually have to explicitly require other helpers, e.g. require 'rails_helper', and require 'rubocop_helper' etc. Those specs are typically grouped together in the directory structures, so having a per-directory rspec.rb or something would help to save a line of code in each of those files.
I've seen enormous efforts to untangle those different test helper (configuration) files to reduce the loading time of specs that have fewer dependencies than the others.

Orthogonal to the above, more an alternative to ~/.rspec and .rspec-local to configure using env vars, when you set up e.g. the number of examples to profile as RSPEC_PROFILE=10 and use your tool of preference to load it up for rspec to consume.

@ioquatix
Copy link
Contributor Author

having a per-directory rspec.rb or something would help to save a line of code in each of those files

Not only that, but it would also unburden users from figuring out their own bespoke solutions.

@JonRowe
Copy link
Member

JonRowe commented Feb 28, 2020

Will it load project2/.rspec or project1/.rspec? Because to me, if it loads project2/.rspec then I think we have a bigger issue that needs to be addressed. The main point being, specs should to a certain extent have a deterministic environment based on the directory hierarchy they are in.

It loads .rspec from the current local directory, thats by design its project specific, if you keep spec files outside your project thats non standard and not supported.

I'm really not keen on having spec folder specific config, that seems like a headache waiting to happen. Especially as we can't mutate running config, especially with plans to go parallel...

@ioquatix
Copy link
Contributor Author

why would you need to mutate the running config?

@JonRowe
Copy link
Member

JonRowe commented Feb 28, 2020

Because the reason why .rspec does --require spec_helper.rb is to apply configuration before we load specs. If you dervive config file locations from spec files you have chicken and egg problems.

@JonRowe
Copy link
Member

JonRowe commented Feb 28, 2020

Its much easier to do things before we load spec files.

@ioquatix
Copy link
Contributor Author

I think that's part of my suggestion though - to make a canonical way to load that before running specs.

@JonRowe
Copy link
Member

JonRowe commented Feb 28, 2020

You can't have that and have folder level config files, we don't know they exist until we load specs.

@JonRowe
Copy link
Member

JonRowe commented Feb 28, 2020

Config files can also add spec files, meaning you have an ever expanding matrix of files and trees to load config from...

@JonRowe
Copy link
Member

JonRowe commented Feb 28, 2020

In my opinion if you want a canonical autoloaded config file it's spec/spec_helper.rb , or maybe spec/rspec.rb even though I'm unconvinced of the benefit of changing the name. Its not more config files.

@timcraft
Copy link

How about replacing .rspec and spec/spec_helper.rb with config/rspec.rb (in RSpec 4).

Simpler than the status quo (1 location to look for configuration instead of 2).

Simpler than directory specific config file (for users and for maintainers).

Similar to other projects, e.g. puma's configuration file.

Addresses this specific issue, --require spec_helper would no longer be needed.

Is there anything that wouldn't support?

@pirj
Copy link
Member

pirj commented Jul 27, 2020

Not necessarily all projects do have a config folder, I'd go with spec/config.rb.

Global ~/.rspec/~/.config/rspec can be helpful by overriding project-local format, colour, and profile settings, and by adding a couple of require's that are not in the bundle, but still available from RVM global gemset, e.g. pry or pp.
Having this global config as a Ruby file feels a bit awkward though.

For specs, this opens an incredible opportunity™: it will be possible to omit the require 'spec/other_helper', by placing this helper (I'd rather call in a config) in the directory where specs need it, e.g.:

specs \
 - config.rb # applies to all specs
 - foo \
   - foo_spec.rb
 - bar \
   - config.rb # only loaded when running `bar` specs
   - bar_spec.rb

@JonRowe
Copy link
Member

JonRowe commented Jul 27, 2020

I'm really not keen on co-locating config with spec files, I suspect it would very surprising to longer term rspec'ers, in addition it raises questions of how we handle conflicting spec config...

Currently we have a spec config that applies once across the entire suite run...

If we allowed per folder spec config are we saying we're going to merge config together? If so in what order? What takes precedence?

If we say we're going to apply that config only to those specs we're going to have to build a way to merge configs and restore the previous, which makes no sense when we're dealing with global configuration.

In short that is going to be a maintenance nightmare for very little benefit...

The original point of this issue was to reduce boilerplate by autoloading a file if present, I support that for RSpec 4, but I'm heavily favouring the existing spec/spec_helper.rb file for simplicity, maybe I could be convinced to give it a different name, but I'm not in favour of removing .rspec entirely.

@ioquatix
Copy link
Contributor Author

ioquatix commented Aug 6, 2022

I've worked through this issue for my own test framework, sus. I decided to load config/sus.rb if it exists. I think this fits with most other models. I'd recommend rspec does the same, load config/rspec.rb if it exists (as suggested by @timcraft).

Not necessarily all projects do have a config folder

it's okay, they can create it if they want default config. I don't see any problem with that?

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

No branches or pull requests

4 participants