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

Check existence of RSpec.configure for test helpers #816

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

pat
Copy link
Contributor

@pat pat commented Jan 12, 2024

When Rails commands such as db:migrate are executed (formerly rake tasks), they load the broader set of rake tasks, and that means that RSpec's rake task is loaded without all of the RSpec context.

This means there is an RSpec constant (from lib/rspec/core/rake_task.rb in rspec-core), but it does not have the configure method. This causes the commands to fail because Flipper's presuming configure will always be there if RSpec is defined:

NoMethodError: undefined method `configure' for RSpec:Module (NoMethodError)

  RSpec.configure do |config|
       ^^^^^^^^^^
/Users/pat/.asdf/installs/ruby/3.2.1/lib/ruby/gems/3.2.0/gems/flipper-1.2.0/lib/flipper/test_help.rb:20:in `<main>'

I considered loading rspec/core here instead to ensure the method exists, but that feels counter to what's actually needed: i.e. only if we have that full RSpec context should we be adding the Flipper helpers.

If we don't have RSpec fully loaded, then we should respect the current context, not choose to load RSpec unexpectedly, and so not add Flipper's helpers.

pat and others added 2 commits January 12, 2024 12:10
When Rails commands such as db:migrate are executed (formerly rake
tasks), they load the broader set of rake tasks, and that means that
RSpec's rake task is loaded without all of the RSpec context - which
means there _is_ an RSpec constant, but it does not have the configure
method.
i.e. lib/rspec/core/rake_task.rb in rspec-core

I considered loading `rspec/core` here instead, but that feels counter
to what's needed: if we have that full RSpec context, then we should
add the Flipper helpers. But if we don't, then we should respect the
current context, not load all of RSpec, and not add Flipper's helpers.
@bkeepers
Copy link
Collaborator

Thanks @pat! I'll push 1.2.1 out shortly.

@bkeepers bkeepers merged commit 8959098 into flippercloud:main Jan 12, 2024
36 checks passed
@pat
Copy link
Contributor Author

pat commented Jan 12, 2024

Thanks @bkeepers - and good move with the respond_to? change, not sure why my brain didn't take that option!

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

Successfully merging this pull request may close these issues.

2 participants