Question: Testing with system tests

[fcheung]

I've got some flipper features that (ultimately) end up triggering different behaviour in our frontend code, so I'd like to write system tests covering both branches.

I'm using the approach suggested at https://www.flippercloud.io/docs/testing of setting

Flipper.instance = Flipper.new(Flipper::Adapters::Memory.new)

in a before(:each).

This doesn't seem to work for system tests - the application code runs in a separate thread to the test code, and Flipper.instance is stored in Thread.curren. As a result my test code and application code end up using separate memory adapters and can't see application code can't see the features enabled by individual test cases.

It seems to work if I instead do

shared_test_adapter = Flipper::Adapters::Memory.new if Rails.env.test?

Flipper.configure do |config|
  config.adapter do
    if Rails.env.test?
      shared_test_adapter
    else
       ... # adapter setup for other environments
    end
  end
end

and instead of reseting Flipper.instance to a new instance I do Flipper.instance.adapter.get_all.clear to clear out changes between specs (which works because get_all just returns the hash that backs the memory adapter)

Is this a sensible way of testing or have I overlooked something?

[jnunemaker]

Good question. We probably need to add some nuance to the test docs. The memory adapter is not thread safe, so I don't think it is a good choice for system tests, but it depends on how they run I guess (and I haven't used them).

The pstore adapter is thread safe by default I believe. It might be a better choice for this. You'll just need to reset it before each test or delete and re-create the file so you know each test starts clean.

What do you think? Seem ok? I could probably throw a thread safe adapter together for this purpose but so far it hasn't really come up. Let me know what you are thinking. Closing this to keep things tidy but happy to continue the conversation.

[fcheung]

I think the non thread safety isn’t a huge deal in practice. In a system test rails usually runs under the hood to serve your application and the test thread uses capybara to exercise the app via selenium/cuprite/etc, so the only time I’m mutating state is in the test setup and the application thread(s) are only reading state.

Perhaps a nicer and also threadsafe approach would be

storage = Concurrent::Hash.new
Flipper.configure do |c|
  c.adapter { Flipper::Memory.new(storage) }
end

(which does depend on concurrent-ruby but so does rails so not an issue I suspect for many uses)

pstore sounds like it would also work ok

[jnunemaker]

@fcheung though concurrent hash is thread safe, our use of it probably would not be. But it would be easy to make an adapter based on concurrent hash that protected around multiple operations that should be seen as one to outside readers. But if the system test runs the way you say, you should be good with what you were doing without the thread safe stuff (as you said, just confirming).

[fcheung]

Cool - thanks for confirming/looking into this!

[Bandes]

Some documentation around this would be very helpful. I've been googling for a while before I found this (closed) ticket and have been really struggling with why my feature spec was failing despite using the approach described in the docs.

[kleinjm]

I also had to loop through my features and disable them before each test. Otherwise, there was shared state between rspec test runs.

RSpec.configure do |config|
  config.before do
    Flipper.configure do |flipper|
      flipper.adapter { Flipper::Memory.new(Concurrent::Hash.new) }
    end
    Flipper.features.each(&:disable)
  end
end

[jnunemaker]

@kleinjm Odd. I can't think why you would need to loop through features other than state sharing in general.

To confirm, these are just regular rails system tests, right? If so, I can try to setup an example that fails and then tweak until it passes and add that to the docs.

[joemsak]

I ran into this issue too, as the Testing docs here https://www.flippercloud.io/docs/testing are still not updated. Weird thing is, the docs worked fine on my local machine but failed on CI.

I went with the pstore option like so:

require 'flipper/adapters/pstore'

RSpec.configure do |config|
  config.before(:each) do
    pstore_path = Rails.root.join("tmp", "flipper.pstore")

    FileUtils.rm(pstore_path, force: true)

    Flipper.configure do |config|
      config.adapter { Flipper::Adapters::PStore.new(pstore_path) }
    end
  end
end

This worked both locally and on CI for me. The memory + concurrent hash option above did not work in my case.

gem "flipper-redis", "~> 0.28.0"

[rnestler]

I hit this to. I think the docs in https://www.flippercloud.io/docs/testing should at least mention that there are issues with system specs.

[jnunemaker]

Added a note to the Testing page. Let me know if that works if you anyone has suggestions.

https://www.flippercloud.io/docs/testing#system-tests

[bkeepers]

I spent a little time investigating this today related to #808, and at least in Rails' System Tests, you can actually share a single memory adapter across threads (Memory adapter is thread safe since #702). I haven't tested with RSpec, but I assume it will work the same since they both just use Capybara under the hood.

The trick is to assign a Memory adapter to an instance outside of config.adapter { … } and return that same instance every time (otherwise Flipper will return a new instance for each thread).

require "application_system_test_case"

class FlipperTest < ApplicationSystemTestCase
  setup do
    # Memory works here, as long as the same instance is shared across this thread and the app thread
    @adapter = Flipper::Adapters::Memory.new

    Flipper.configure do |config|
      config.adapter { @adapter }
    end
    Flipper.instance = nil # Ensure flipper gets reset
  end

  test "feature flags in system tests" do
    Flipper.disable(:some_feature)
    visit root_path
    assert_selector "span", text: "Feature disabled"

    Flipper.enable(:some_feature)
    visit root_path
    assert_selector "span", text: "Feature enabled"
  end
end

@jnunemaker can you think of any issues with this?

[jnunemaker]

Good point! Can't think of any problems.