Skip to content

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
@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