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.

Sign up for GitHub

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

Add flipper/test_help to automatically configure Flipper in tests/specs. #808

Merged
merged 9 commits into from
Jan 8, 2024
Merged

Add flipper/test_help to automatically configure Flipper in tests/specs. #808

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2672714
Add flipper/test_help to automatically configure Flipper in tests/specs.
bkeepers Jan 3, 2024
f76d663
Merge remote-tracking branch 'origin/main' into test_help
bkeepers Jan 5, 2024
f5e26fb
Automatically load test_help if Rails.env.test?
bkeepers Jan 5, 2024
32b3928
Add SystemTestCase to ensure Flipper::TestHelp works with threaded ca…
bkeepers Jan 6, 2024
f3620af
Skip system test on old Rails versions
bkeepers Jan 8, 2024
414579e
Fix test by loading defaults for current Rails version
bkeepers Jan 8, 2024
a7d9266
Force module to be included
bkeepers Jan 8, 2024
903b9a5
Merge remote-tracking branch 'origin/main' into test_help
bkeepers Jan 8, 2024
05e2ce0
🤞 Fix brittle test
bkeepers Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions lib/flipper/test_help.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module Flipper
module TestHelp
extend self

def before_all
Flipper.configure do |config|
config.adapter { Flipper::Adapters::Memory.new }
config.default { Flipper.new(config.adapter) }
end
end

def before_each
Flipper.instance = nil # Reset previous flipper instance
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got some spec failures after this - you get a new flipper instance but using the same memory adapter, so there's leakage between tests.

I can make everything green again if I add Flipper.instance.import(Flipper::Adapters::Memory.new) to the reset (which was the simplest way I could find to delete all the data from the shared adapter)

Copy link

@rnestler rnestler Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I hit the same. Maybe it would be better to just disable all feature flags in reset? Ah no, for me it just doesn't work in system specs with the memory adapter. The feature flag just doesn't get set.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I hit this. The app thread and spec thread end up using a different flipper instance (fine). The app thread is running for the entire duration of the specs so keeps the same flipper instance.

On the second spec the before each creates a new flipper instance but this only affects the spec thread, the app thread still has the old memory adapter.

To make this work I had to disable the builtin flipper/test_help (with the env var) and then do what I was doing before: create the shared memory adapter once in before(suite) and clear out its contents in before each

end
end
end

if defined?(RSpec)
RSpec.configure do |config|
config.before(:all) { Flipper::TestHelp.before_all }
config.before(:each) { Flipper::TestHelp.before_each }
end
elsif defined?(ActiveSupport::TestCase)
Flipper::TestHelp.before_all

ActiveSupport::TestCase.setup do
Flipper::TestHelp.before_each
end
end
1 change: 1 addition & 0 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
require 'flipper/api'
require 'flipper/spec/shared_adapter_specs'
require 'flipper/ui'
require 'flipper/test_help'

Dir[FlipperRoot.join('spec/support/**/*.rb')].sort.each { |f| require f }

Expand Down