Do not require RSpec if not already loaded

cucumber / cucumber-rails

Rails Generators for Cucumber with special support for Capybara and DatabaseCleaner

https://github.com/cucumber/cucumber-rails

MIT License

1.02k stars 327 forks source link

Do not require RSpec if not already loaded #448

Closed jsmestad closed 4 years ago

jsmestad commented 4 years ago

Summary

Fixes #446

How Has This Been Tested?

Types of changes

[x] Bug fix (non-breaking change which fixes an issue).
[ ] New feature (non-breaking change which adds functionality).
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

[ ] I've added tests for my code.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.

luke-hill commented 4 years ago

Actually having re-reviewed the codebase. If you want to add a 1liner to include the extra file. I'm happy with that as a bug request fix. But altering the requires would be a feature change (which is fine), but I'd want that tweaking in other files ideally.

The reason being is that the one autoload file which "isn't" autoloaded is this file (For a reason I'm not sure of). Essentially a user could in theory use cucumber without either of rspec or minitest in place (Which I'm not sure is great)

For your specific use case (Both loaded), this probably needs some form of manual user input. So a user would load something like cucumber/rails/rspec for rspec and cucumber/rails/minitest for minitest? I don't know, what do you think is best? Given you are using this currently.

Also there's some argument it might be a major change (Again not a problem), but would need treating accordingly.

jsmestad commented 4 years ago

Whilst the file already has conditionals. Given these are procedural loaders I'd rather remove conditionals.

Maybe look into making another file for minitest?

We could do something to the inverse maybe to avoid the breaking change? Maybe something like: if minitest is loaded and RSpec is not, then dont require RSpec?

luke-hill commented 4 years ago

Sounds like a good idea.

So basically we have 4 situations.

Neither (I don't like this situation, but for now we can tolerate it. I "believe" the suite would crash with a Minitest failure)
RSpec (This works I believe)
Minitest (This one I believe works, beause you would fail the first require, and then work on the 2nd require)
Both (This is the situation you have, which is awkward I admit)

Atm we load RSpec when only RSpec is present (:green_book:) The rest are all possibly subject to change / tweaks (:green_book:)

Anything you can do RE that is good in my eyes. If you wanted to check if these were already pre-loaded we could. However I don't know what users allow delegate their loading to us. i.e. it could be people just require 'cucumber/rails/rspec'

I know I've probably opened up a can of worms here. So I'll apologise for that. Even the name of the file gives away we perhaps are a bit behind the times here.

olleolleolle commented 4 years ago

@luke-hill Would that translate to: change the else to another if defined?(...) conditional around the second [Cucumber::Rails::World, ActionDispatch::Integration::Session].each do |klass| ?

luke-hill commented 4 years ago

Possibly. But I think a better approach would be to have a minitest initializer and an rspec one. Because at the moment, the rspec one (named as such), is controlling minitest because we (perhaps incorrectly), assumed that if one existed, the other didn't. And one always existed.

In the truth table, there's 4 situations. We need to cover each of them ideally. The OP did suggest we could cover 2 of the use cases (1 is loaded and the other isn't), by just checking for each of them

luke-hill commented 4 years ago

@jsmestad any thoughts on this?

jsmestad commented 4 years ago

@luke-hill I think the request became larger than what I was expecting and had to move onto other tasks. Since then we just switched to RSpec completely to avoid the issue altogether.

I agree with the approach, but the time involved was going to be too much for me to take on during year-end scramble.

luke-hill commented 4 years ago

I'll kill the PR @jsmestad but leave the issue open so someone else can come into it. I'm thinking this will be hard to fix in a regular PR, but we could make a brute force change and cut a 3.0 release and just say

"cucumber rails always had an assumption you would use rspec, but now you need to explicitly tell it you want to use rspec, or minitest"