RSpec does not reset ActiveSupport::CurrentAttributes around each example

[mattbrictson]

What Ruby, Rails and RSpec versions are you using?

Ruby version: ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [x86_64-darwin20]
Rails version: 6.1.3.2
RSpec version: RSpec 3.10

• rspec-core 3.10.1
• rspec-expectations 3.10.1
• rspec-mocks 3.10.2
• rspec-rails 5.0.1
• rspec-support 3.10.2

Observed behaviour

rspec-rails does not reset CurrentAttributes. That means if one test sets a value using CurrentAttributes, it can very easily pollute other tests.

Expected behaviour

Rails ActiveSupport::TestCase automatically resets CurrentAttributes between each test (see active_support/current_attributes/test_helper.rb). I would expect rspec-rails to do the same.

Can you provide an example app?

Here is a trivial example app using CurrentAttributes and an ActiveSupport::TestCase that passes:

require "rails"
require "rails/test_help"

class MyApp < Rails::Application
  config.eager_load = false
end

MyApp.initialize!

class Current < ActiveSupport::CurrentAttributes
  attribute :user_id
end

class CurrentTest < ActiveSupport::TestCase
  test "allows setting user_id" do
    Current.user_id = 123
    assert_equal(123, Current.user_id)
  end

  test "has a nil user_id by default" do
    assert_nil(Current.user_id)
  end
end

Test output:

$ bundle exec ruby testcase.rb 
Run options: --seed 52779

# Running:

..

Finished in 0.001012s, 1976.2846 runs/s, 1976.2846 assertions/s.
2 runs, 2 assertions, 0 failures, 0 errors, 0 skips

However the same app, when tested via rspec-rails, fails:

require "rails"
require "action_controller/railtie"
require "action_view/railtie"
require "rspec/rails"

class MyApp < Rails::Application
  config.eager_load = false
end

MyApp.initialize!

class Current < ActiveSupport::CurrentAttributes
  attribute :user_id
end

describe Current do
  it "allows setting user_id" do
    Current.user_id = 123
    expect(Current.user_id).to eq(123)
  end

  it "has a nil user_id by default" do
    expect(Current.user_id).to be_nil
  end
end

Spec output:

$ bundle exec rspec rspec.rb 
.F

Failures:

  1) Current has a nil user_id by default
     Failure/Error: expect(Current.user_id).to be_nil
     
       expected: nil
            got: 123
     # ./rspec.rb:23:in `block (2 levels) in <top (required)>'

Finished in 0.01094 seconds (files took 0.66551 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./rspec.rb:22 # Current has a nil user_id by default

Here is the Gemfile I used:

source "https://rubygems.org"

gem "rails", "~> 6.1"
gem "rspec", "~> 3.10"
gem "rspec-rails", "~> 5.0"

[pirj]

As per Rails doc:

Abstract super class that provides a thread-isolated attributes singleton, which resets automatically before and after each request. This allows you to keep all the per-request attributes easily available to the whole system.

The reset code is injected by Rails here into ActiveSupport::TestCase.
It was introduced here, but I can't say I follow the "we were missing plain tests, so this is that" rationale behind this change.
Do you have an idea?

In rspec-rails, we don't inherit from ActiveSupport::TestCase, and this reset code is never called.

On the one hand, it makes total sense to reset the current attributes. On the other - it is reset before and after each request, and why would we explicitly reset it in test support code once again?

      app.executor.to_run              { ActiveSupport::CurrentAttributes.reset_all }
      app.executor.to_complete         { ActiveSupport::CurrentAttributes.reset_all }

Can you please check if the issue still reproduces for controller/feature/request specs?

[mattbrictson]

Thanks for the quick response!

Yes, I think your assessment is correct. I just confirmed that request specs do not have this problem. As you said, it is only an issue for "plain tests".

My understanding is that request specs, since they simulate actual web requests, are inherently using the app "executor". That executor takes care of resetting the current attributes. In this case the test environment is very similar to the actual production environment, and so the behavior of current attributes is similar.

For a "plain test" like a model spec, there is no request/response or active job execution being simulated. Hence the app executor is not involved, and so current attributes will not be reset via the executor. That is why rails/rails#39493 introduced the explicit reset in ActiveSupport::TestCase, so that current attributes would get reset for these "plain tests" as well.

I think this is reasonable behavior and expected for a framework feature like ActiveSupport::CurrentAttributes. Unit tests are supposed to be independent of one another, and thread-local storage breaks that independence. Since CurrentAttributes is a standard part of the Rails framework I would expect a built-in solution for this test interdependence issue.

[pirj]

Agree. I see your point and the reasoning behind rails/rails#39493 now as well.
However, if the subject under test relies on some specific current attribute, and this value may affect the results of the tests, shouldn't it be explicitly set in the test setup?

What I mean by this is e.g. Time.zone. It's set somewhere in rack in the app, but, obviously, this isn't happening in model specs.

What I could quickly find in the wild:

# chatwoot/app/models/user.rb
class User < ApplicationRecord
  def current_account_user
    account_users.find_by(account_id: Current.account.id) if Current.account
  end
...
  def assigned_inboxes
    inboxes.where(account_id: Current.account.id)
  end

So, apparently, an example group with no explicit setup could rely on the fact that Current.account is nil, but it can't, since CurrentAttributes leak state.

Personally, I feel a smell when Current. is directly used in models. And when it's reset four times for a single test. It's subjective, and I don't account for that in my reasoning.

I have no objections to add the reset code to all Rails-specific example groups (RSpec::Rails::RailsExampleGroup) by including ActiveSupport::CurrentAttributes::TestHelper to it. I did not test this approach, though. To make it work for everyone, I suppose a check for defined?(ActiveSupport::CurrentAttributes::TestHelper) needs to be made.
Would you like to submit a pull request?

[JonRowe]

If you add type: :model the problem should similarly go away, we don't include rails helpers except when running a rails test, so currently you'd need to pick one of the rails types. We could add a generic "type: :rails" for the base class, or help guide towards defaulting towards including the helpers, but its not something we can do into bare RSpec tests, because they are for bare Ruby, not bare Rails, and there are people who mix these (e.g. write proper plain old ruby with specs, even in Rails app)

[mattbrictson]

If you add type: :model the problem should similarly go away

Ah, you're right!

I had originally encountered this problem with current attributes in a rails app where I have app/commands and app/services classes. Apparently when I write specs for these, rspec-rails does not have any built-in behavior via infer_spec_type_from_file_location! for "commands" and "services". Thus my specs are treated as "plain ruby".

I see this is a mistake on my part; I need to configure rspec to recognize those directories. Something like this, I guess?

RSpec.configure do |config|
  config.include(
    RSpec::Rails::RailsExampleGroup,
    file_path: %r{spec/commands|spec/services}
  )
end

In any case my original issue does not seem to be a bug, so I will close.

[javierjulio]

To help others finding this issue, I was confused for awhile as to why this was closed. I realize it has to do with new Rails versions since. This was for Rails 6.1 so likely using type: :model on a plain spec fixed the issue on that version, but once on Rails 7.0 and loading 7.0 defaults (so executor_around_test_case is true) this is not the case and CurrentAttributes will leak from both plain tests and some rails example group tests.

Since rspec-rails already includes the execution context test helper, I've created #2752 with a temporary fix to include the current attributes test helper to match what Rails 7 does. That will still require a rails based spec (e.g. type: :model) but makes sense based on rspec-rails setup. The ideal fix is in #2753 as that should account for both cases, where executor_around_test_case is true and false, with the same rails based spec type requirement.