-
Notifications
You must be signed in to change notification settings - Fork 22.1k
Fix CI eager loading when rake tasks invoke :environment before tests #56212
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.
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
Fix CI eager loading when rake tasks invoke :environment before tests #56212
Conversation
The test environment template sets config.eager_load = ENV["CI"].present? to enable eager loading in CI environments. However, when rake tasks invoke the :environment task (common with gems like tailwindcss-rails that hook into test:prepare), Rails overrides config.eager_load with config.rake_eager_load which defaults to false. This causes eager loading to be disabled in CI when running bin/rails test, even though ENV["CI"] is set, preventing Zeitwerk from catching autoloading errors that zeitwerk:check would catch. Setting config.rake_eager_load = ENV["CI"].present? ensures eager loading works consistently in CI regardless of whether gems invoke :environment before tests run.
| # recommended that you enable it in continuous integration systems to ensure eager | ||
| # loading is working properly before deploying your code. | ||
| config.eager_load = ENV["CI"].present? | ||
| config.rake_eager_load = ENV["CI"].present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did a similar thing in my test environment when i realized that. i didn't have the energy to explain all this in a rails PR. Sometimes i thank AI for this purpose.
I would suggest doing this instead though:
config.eager_load = ENV["CI"].present?
config.rake_eager_load = config.eager_loadSo that we don't duplicate the same logic if a developer needs to change it. and it also clearly states that we want the same configuration as config.eager_load 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, not a bad idea! I'll leave to Rails maintainers, I think, unless you wanted to consider another PR?
|
It feels like we should fix the root cause, rather than having to set two configs to the same value to actually eager load. |
|
@ghiculescu agreed, but I'm not sure what's best. See #56065 for some more conversation and see the other linked PRs, too. Any ideas for improvements welcome! |
|
I'm not sure on the how, but it feels like if we are saying Maybe that means changing the ActiveSupport.on_load(:before_initialize) { config.eager_load = config.rake_eager_load if config.eager_load == NOT_SET } |
|
I'm not sure if that would work, though, because in production eager loading would be set but we'd want rake eager loading to be disabled, from what I can tell. It takes a bit of spelunking but for example see: #28209 and the recent; rails/solid_queue#308 (comment) which make me think this is still intentional to have two different flags. I mentioned this also here: #56065 (comment) but I think perhaps one way to think about this is that Given that framing, I think it seems reasonable to say “in testing, we want to enable eager loading when running (booting) the application and also when running rake tasks” and it’s potentially fine to acknowledge that they’re two different config flags with two different contexts. Xavier’s closing thought on the Zeitwerk PR was that this still seems like a heavy hammer, in that now we’re enabling eager loading for all rake tasks, not just the one we care about. So, I think perhaps what we have is fine, but a better, more targeted fix would be to dig into the FWIW I still think the separate, dedicated Zeitwerk check in CI is a reasonable, clear, and targeted addition, but I understand the reluctance of Rails Core to add more to the default CI pipeline. So, sorry for the rambling post, I'm still a bit turned around about this issue, but I think the ideal fix here would be to have a more targeted change to make sure rake eager loading is enabled for |
Summary
This PR fixes an issue where
config.eager_load = ENV["CI"].present?in the test environment doesn't work as expected when gems invoke the:environmentrake task before tests run.The Problem
Related to #56065, which proposes adding
rails zeitwerk:checkto the default CI workflow. While investigating whyCI=1 bin/rails testpasses butbin/rails zeitwerk:checkfails for apps with Zeitwerk errors, I discovered a deeper issue with the test environment configuration.When the
:environmentrake task runs (triggered by gems liketailwindcss-railsthat hook intotest:prepare), Rails executes:Since
config.rake_eager_loaddefaults tofalse, this overrides theconfig.eager_load = ENV["CI"].present?setting inconfig/environments/test.rb, causing eager loading to be disabled in CI when runningbin/rails test.Reproduction
tailwindcss-railsand a gem that has a Zeitwerk naming error (e.g.,swarm_sdk 2.1.3)CI=1 bin/rails test- tests pass (eager loading is disabled)bin/rails zeitwerk:check- fails with Zeitwerk error (eager loading works)See https://github.com/trevorturk/issue_56065 for a minimal reproduction case.
The Fix
Set
config.rake_eager_load = ENV["CI"].present?in the generated test environment template to match theconfig.eager_loadbehavior. This ensures eager loading works consistently in CI regardless of whether rake tasks invoke:environmentbefore tests run.Why This Approach
This is the minimal fix that ensures the intended behavior works correctly. The alternative would be to change how Rails handles the
:environmenttask, but that could have broader implications. Setting both config options in the test environment is straightforward and aligns with the documented intent of eager loading in CI.History of rake_eager_load via Claude
Here's the history and purpose of rake_eager_load:
The Problem It Was Designed to Solve
Historical issue: Before Rails 5.1, rake tasks always disabled eager loading by hardcoding config.eager_load = false in the :environment task. This was done for "legacy
reasons" - specifically, problems with rake assets:precompile attempting database connections due to eager loading.
The regression: Disabling eager loading in rake tasks caused thread safety problems in production rake tasks. When code isn't eager loaded, classes might be autoloaded at
runtime by multiple threads, leading to race conditions.
What rake_eager_load Does
From PR #28209 (2017, merged 2019):
"I would wish to be able to control that behaviour so regardless how the application is accessed, the load order remains the same."
config.rake_eager_load allows you to opt-in to eager loading for rake tasks, overriding the default behavior where rake tasks never eager load.
Default behavior:
From the autoloading guide (commit 6cbd2c9):
"When a Rake task gets executed, config.eager_load is overridden by config.rake_eager_load, which is false by default. So, by default, in production environments Rake tasks
do not eager load the application."
The Irony
The test environment setting config.eager_load = ENV["CI"].present? was trying to enable eager loading in CI to catch autoloading bugs...but it was being overridden by
rake_eager_load = false whenever any gem triggered the :environment task!
So our fix is actually using rake_eager_load for its intended purpose - to control eager loading behavior in rake tasks, including bin/rails test which runs as a rake task.