Class reloading triggered during request specs with Rails 7.2

[jscheid]

What Ruby, Rails and RSpec versions are you using?

• ruby 3.3.1
• rails 7.2.1
• rspec-core 3.13.0
• rspec-expectations 3.13.0
• rspec-mocks 3.13.1
• rspec-rails 6.1.4
• rspec-support 3.13.1

Observed behaviour

During a type: :request spec, when the request itself is triggered, autoloader kicks in and reloads a lot of classes - perhaps when resolving the route. The reloaded classes include some (ActiveRecord::Base-derived model classes) on which the spec had earlier installed stubs. The newly loaded class versions don't have the stubs, breaking the spec. Also, the class reloading takes some extra time, slowing down test runs.

Expected behaviour

I'm not sure why autoloading is triggered in the first place since the underlying files definitely aren't being touched at that point. However, I think autoloading should never trigger in the middle of test execution, outside of perhaps some exotic use cases (i.e. testing autoloading itself.)

Can you provide an example reproduction?

Sorry, not at this point, I can try to reduce it to one if necessary but it would be very time consuming because this happens in a large app and I can't yet reproduce it 100% reliably either. Let me know if you need me to try. Here is some extra information though:

• This seems to be caused by upgrading to Rails 7.2.1 (from 7.1.3.3) - maybe something has changed in how Zeitwerk works or interacts with request specs? Anyway, it was working fine in 7.1 (and also 7.0, 6.1.)
• Classes aren't autoloaded every time I run tests, only maybe 80-90% of the time. That seems to be pointing at some kind of race condition.

As for workarounds, I can't figure out how to inhibit Zeitwerk temporarily without also disabling reloading across the whole test env (where it is very much welcome.)

It might be a misconfiguration of Rails 7.2 on our part, which I can't rule out. Any ideas?

[pirj]

Is it stable if you provide the same seed?

autoloading should never trigger in the middle of test execution

What do you mean “middle of test”? Middle of test suite? Easily. Just put a const into the example’s code. Same even for “middle of example”.

Why is it problematic? How was it working before?

At this point this is a hard one to guess, so you’re most certainly on your own in figuring out what it is.

If you think there’s a bug somewhere, or a slippery configuration option that could lead others to frustration - please feel free to reopen.

[jscheid]

I do think there's a bug, I'm just not sure if it's in Rails 7.2 or in rspec support for Rails 7.2, and I was hoping you could help me figure it out.

Let me try to explain the problem better:

1. The request spec is first stubbing a class method: expect(Foo).to receive(:bar).and { ... }
2. Then it calls post. This causes autoloader to kick in, in Rails 7.2 only (used to work fine before), perhaps as part of route resolution?
3. Autoloader replaces class Foo by a new version, so bar is no longer stubbed and the rest of the spec fails because of it.

What do you mean “middle of test”? Middle of test suite? Easily. Just put a const into the example’s code. Same even for “middle of example”.

Sorry, I don't think I follow, but I hope what I mean is clear from the new description - I'm looking for a way to inhibit autoloading during the execution of a single test case (it block) and I'm trying to understand why it would be autoloading in the first place with Rails 7.2.

Is it stable if you provide the same seed?

There is no randomization involved in my test setup.

[jscheid]

Ah, I think I know now what you mean by "just put a const". You're right, autoloading is expected when the interpreter hits a new const that it hasn't seen before. What I'm trying to avoid is reloading.

The files haven't been touched so there should be no need to reload in the first place, and even if they had been touched I wouldn't want the const to be swapped out while a single example is running.

[pirj]

This is certainly interesting, but there’s too few information to get any lead.

I’d start with setting a breakpoint or putting a puts statement printing caller_locations in the beginning of the reloaded file to see at least what triggered this.

I’ll be happy to help with fixing this once you’ve figured out there’s a bug in rspec-rails.

[jscheid]

Thanks @pirj, I've got to the bottom of it. It's a combination of a number of things:

We're using Timecop in our specs and when the @watcher in ActionView::CacheExpiry::ViewReloader is first constructed in a situation where Time.now is set to be in the past (so that all watched files' mtimes are in the future), this confuses the watcher and it will later (on the next invocation of execute) report files as having been changed, when they haven't.

This is only an issue in Rails 7.2 due to this commit: with this change the watcher is being constructed lazily and therefore construction might happen inside a test case, where it can be subject to Timecop. In previous Rails versions it was always constructed at boot time, where Timecop isn't usually used.

The solution (or workaround) is simply to force construction of the watcher at the beginning of the test suite, so my immediate problem is solved.

However, one might still see strange effects when any of the watched files happens to be touched while an example is executing. That shouldn't normally happen and isn't a huge problem, however I'd like to come back to a point I've raised earlier:

I'm still having a hard time coming up with a scenario where one would want reloads to happen during the execution of an example. Perhaps this could be an improvement worth making to rspec-rails at some point, speaking of potential frustration? I don't currently see a mechanism in Rails to inhibit reloading temporarily, but it might be worth lobbying for one.

Even if files aren't ever touched during execution, watchers still consume needless cycles checking for changes at a time when they would ideally be ignored anyway.

For completeness here is the failing test case, it needs config.enable_reloading = true to trigger, Rails 7.2 as mentioned before, and a view that references the Test class:

require 'rails_helper'

describe 'class reload during example', type: :request do
  before do
    # Take ActionView::CacheExpiry::ViewReloader into a broken state
    # because all mtimes will be in the future compared to Time.now.
    Timecop.freeze('1970-01-01') { get(rails_health_check_path) }
  end

  it 'would ideally not reload' do
    # Install a stub.
    expect(Test).to receive(:test).and_return(0)

    # Hit Railtie, where reloaders will be checked and because of the broken
    # state, classes will be reloaded, including `Test` if it's referenced
    # by any view.
    get(rails_health_check_path)

    # This would pass if the stub was still installed, but the `Test` class
    # was swapped out by the reloader and the new class version doesn't
    # have the stub, therefore this fails.
    expect(Test.test).to be_zero
  end
end

[pirj]

Nice. Thanks for the in-depth analysis.

Reloading can be enabled or disabled. The setting that controls this behavior is config.enable_reloading, which is true by default in development mode, and false by default in production mode.

Wondering, why is the default for the test env is not mentioned?
In the new project generator, it’s false for the test env.

It’s a new option, opposite if cache_classes. Can it be you have cache_classes somehow turned to false in the test env? Or the new enable_reloading not set?

I certainly may be using some use cases for reloading code in the test env, but if it’s wasteful and may cause weird issues like you’ve seen, is it worth it? Why is it even an option to turn it on?

Do you think it is possible to reproduce the same issue with the Rails native test framework? If so, it’s more likely to need a fix on Rails side.

In rspec-rails we don’t control those options, so there’s little we can do.

[jscheid]

It’s a new option, opposite if cache_classes. Can it be you have cache_classes somehow turned to false in the test env? Or the new enable_reloading not set?

I certainly may be using some use cases for reloading code in the test env, but if it’s wasteful and may cause weird issues like you’ve seen, is it worth it? Why is it even an option to turn it on?

Yes we do have cache_classes turned off (and enable_reloading turned on), following the recommendations here.

Do you think these recommendations are wrong or misleading? Unfortunately, no rationale is given for what exactly happens if these settings are left at their defaults and to be honest I've never thought to try.

Do you think it is possible to reproduce the same issue with the Rails native test framework? If so, it’s more likely to need a fix on Rails side.

I'm almost certain it's possible to reproduce there, but we're using rspec so that's what matters to me.

In rspec-rails we don’t control those options, so there’s little we can do.

I was imaging something like this (pseudo-code):

config.around { |example| Rails.application.without_reloading { example.run } }

without_reloading is a fictional method that I've made up, nothing like it exists as far as I could see. We'd probably have to lobby for it to be added.

If it did exist I'd be adding the above to our spec_helper and I would suggest to you to consider making it the default so that other users are spared this particular footgun.

[jscheid]

Do you think these recommendations are wrong or misleading? Unfortunately, no rationale is given for what exactly happens if these settings are left at their defaults and to be honest I've never thought to try.

Wait -- I just realized these are instructions for Spring, which we're not using anymore. It seems it's not needed for Bootstrap? Gah... 🤦

If so then perhaps the best thing to do would be for Rails to print a warning if cache_classes is false (or enable_reloading is true) in test env and Spring isn't loaded. I might suggest that to the Rails people, wdyt?

[pirj]

And why would someone include spring and turn the reloading option off? And vice versa?

Reloading is a totally reasonable option when using Spring, but the glitch with AS view reloader - why it does what normally Spring does?

And if you had spring loaded, why would it reload classes mid-test? Might be the reason why spring is out of being turned on by default.

[atef-ds]

Hello @jscheid,

I have the same issue in our project after upgrading rails from 7.1 to 7.2( we are usingTimecop.freeze in our feature tests)
Could you let me know if you created an issue somewhere?
Thanks

[jscheid]

@atef-ds no, I haven't. It's a really gnarly one but I don't think it's a bug in Rails. It's a misconfiguration on our part, or rather a historical accident - we were using settings recommended for Spring until after (way after) we stopped using it. It probably didn't matter much until it came back to bite us for the Rails 7.2 upgrade.

For the record, the fix ended up being:

diff --git a/config/environments/test.rb b/config/environments/test.rb
index f4a4cab770..f003060cb5 100644
--- a/config/environments/test.rb
+++ b/config/environments/test.rb
@@ -6,7 +6,7 @@ Waysact::Application.configure do

-  config.cache_classes = false
+  config.enable_reloading = false

Hope this helps you.

There probably is some improvement in Rails that would be appropriate here - maybe some check to warn users of footgun settings - but after their bot closed a number of tickets on me I can't be bothered to write it up rn.