Capture boot-time deprecations by booting the app from the capture script - #199
Capture boot-time deprecations by booting the app from the capture script#199JuanVqz wants to merge 3 commits into
Conversation
41f3fa2 to
d2e25ff
Compare
The per-test tracker attaches per example, so it never sees the deprecations an app emits while it boots: the ones from initializers, and the association / scope / callback declaration warnings that fire when a class body is evaluated during eager-load. No test necessarily triggers those, so they stay invisible until a Rails upgrade turns them into errors. BootCapture provides the command; boot_capture_runner.rb does the work. The runner boots the app itself, doing what config/environment.rb does with the attach wedged in the middle: install the tracker <- Kernel#warn patch live from here on require config/application (Bundler.require loads the gems here) configure deprecations <- before any initializer runs initialize! <- app and framework initializers, captured eager_load! <- class bodies, captured Running inside `rails runner` instead would hand the script an app that is already initialized, so everything left of that last arrow would already be gone. Measured against a Rails 4.0 app: 189 unique warnings this way versus 167 attaching after boot, with the difference concentrated in models an initializer happens to reference, whose class bodies are already loaded by eager-load time and so never warn again. The deprecation setup goes through config.active_support rather than the deprecators, because Rails' own active_support.deprecation_behavior initializer assigns behavior from those config values and would overwrite anything set beforehand. It is re-installed from a railtie initializer ordered behind :load_environment_config, since config/environments/<env>.rb is loaded during initialize! and a plain `config.active_support.deprecation = :stderr` there would otherwise replace the collector; again from before_eager_load, after every initializer, for environments that eager-load during boot; and once more after initialize! for environments that do not. No capture logic is reimplemented: init_tracker installs the version-correct hooks and KernelWarnTracker, and add / after_run collect and write. Verified on Rails 3.2.22.5, 4.0.13 and 8.1, on Ruby 2.3.8 and 3.4.
`deprecations boot` runs the capture and summarizes it like `info` does. The result is written to spec/support/deprecation_warning.boot.shitlist.json (or the .next variant with --next, which selects the next bundle through BUNDLE_GEMFILE and BUNDLE_CACHE_PATH), keyed under a single "boot" bucket. The capture is written to a .partial sibling and renamed onto the real output only on success, so a boot that fails never destroys a previous good capture. A missing partial afterwards means the run never reached after_run, which is why a failed boot reports the failure instead of an empty, misleading "clean" result. --pattern is refused, since the capture cannot filter what an app emits while booting. Verbose output labels the bucket "Source" rather than "Test files", because "boot" is not a spec file. The boot branches are covered by driving the CLI against a stubbed `bundle`, so they run without a Rails app.
d2e25ff to
c553e32
Compare
arielj
left a comment
arielj
left a comment
There was a problem hiding this comment.
I'm requesting some changes mainly because of two things:
- the comments added in this PR are too verbose, some things don't really look like code comments but just PR comments but something like Claude added them there, when comments are so many it's hard to understand which comments are relevant and which ones are not
- there are A LOT of tests just testing implementation (almost all the boot_capture_spec.rb tests), the correct way to test this is to add one (or a few) minimal rails app in the test directory with specific known deprecations this feature would capture and make sure they are captured, all the implementation should be irrelevant for the tests as long as things work as expected
| total_messages = result.values.map(&:size).reduce(0, :+) | ||
| puts "Merged #{shards} shard files into #{path} (#{result.size} buckets, #{total_messages} deprecation messages)" | ||
| when "boot" | ||
| abort "--pattern is not supported with 'boot': it captures everything the boot emits." if options[:pattern] |
There was a problem hiding this comment.
maybe we can just warn an not abort here? using the Tint module, a yellow/orange message should be fine, I don't think it's that critical to ignore the pattern and still run the thing
| # The runner boots the app itself (require config/application, configure, then | ||
| # initialize!) rather than running inside an already-initialized `rails runner`, | ||
| # so initializer-time and gem-load-time warnings are in scope too. See | ||
| # boot_capture_runner.rb for what remains out of reach (config/boot.rb, and | ||
| # deprecator-based warnings during Bundler.require). |
There was a problem hiding this comment.
this paragraph sounds out of context, it's an explanation on why the previous implementation was changed to the new implementation in this same PR, but for anyone reading the comment in the future it's irrelevant that this is not using "rails runner", feels like a comment that belongs to just the PR and not the actual code
| # Kept compatible with the gem's supported Rubies (>= 2.0): no safe-navigation, | ||
| # no squiggly heredocs, stdlib only. |
There was a problem hiding this comment.
this comment also feels out of context, this class has no heredocs for example, it's not relevant to mention it's not using squiggly heredocs for compatibility when no heredocs at all are being used
this feels like something the tests would catch, no need for a comment
I understand some of these things are added by Claude because it's too verbose, but too many comments make the real important comments harder to get
| # The command that runs the gem's runner script in the app's bundle. Plain | ||
| # `bundle exec ruby`, not `rails runner`: the runner boots the app itself so it | ||
| # can attach before the initializers run, and `rails runner` would have already | ||
| # booted it (and, on Rails 3.x, would eval the script instead of loading it). |
There was a problem hiding this comment.
Similar to a previous comment, it's irrelevant that this is not using "rails runner" here, there's no reason to expect this to run with "rails runner" other than having seen the previous implementation.
| # Safe to call even when the environment already eager-loaded during initialize! | ||
| # (config.eager_load = true, or Rails 3.x cache_classes): loading is idempotent, and | ||
| # because we attached before initialize! those warnings were captured either way. | ||
| # That is why this approach needs no "this environment eager-loads at boot" refusal. |
There was a problem hiding this comment.
this comment is also irrelevant, it's only an explanation of why the approach changes in this same PR, but for anyone without that context there's no "this environment eager-loads at boot" refusal approach to understand this line.
| it "is valid Ruby" do | ||
| # `ruby -c` is portable across implementations; RubyVM::InstructionSequence is MRI-only. | ||
| expect(system(RbConfig.ruby, "-c", described_class::RUNNER_PATH, out: File::NULL)).to be(true) | ||
| end |
There was a problem hiding this comment.
this is a really weird test, not sure what it's trying to test about this feature
| expect(system(RbConfig.ruby, "-c", described_class::RUNNER_PATH, out: File::NULL)).to be(true) | ||
| end | ||
|
|
||
| it "reuses DeprecationTracker rather than reimplementing capture" do |
There was a problem hiding this comment.
this sounds too much like "testing implementation", which is always a bad idea for tests
I don't think it's relevant to test that it's not reimplementing something
| expect(described_class::OUTPUT_ENV).to eq("DEPRECATION_BOOT_OUTPUT") | ||
| end | ||
|
|
||
| it "stores project-relative paths by stripping Rails.root (committable shitlist)" do |
There was a problem hiding this comment.
this sounds like it's testing that the actual code has a method called "transform_message" and calls "gsub(..."? it's just testing implementation, I would remove this
| expect(script).to include('gsub("#{Rails.root}/"') | ||
| end | ||
|
|
||
| it "installs the tracker before requiring the app, so gem-load warnings are in scope" do |
There was a problem hiding this comment.
all these tests seem to be testing the implementation, that the actual code is where it is, which is a bad idea
I don't think most of these tests are relevant and we should test behavior, not implementation (so if the implementation changes but we get the same result then it's fine)
| end | ||
| end | ||
|
|
||
| describe "DeprecationTracker save outside a test process (boot runs it via `rails runner`)" do |
There was a problem hiding this comment.
I'm confused about this test, we don't use rails runner anymore, I'm not sure what this test is testing
2 participants
Replaces #194. Same goal, different attach point: the capture script boots the app itself instead of running inside
rails runner, which is what lets it see the deprecations an app emits before eager-load.Why
The per-test tracker attaches per example, so it never sees what an app emits while booting: initializer-time warnings, and the association / scope / callback declaration warnings that fire when a class body is evaluated. No test necessarily triggers those, so they stay invisible until an upgrade turns them into errors.
rails runnerhands a script an app that is already initialized, so those warnings are gone before the script gets control. Booting the app ourselves moves the attach point as early as a script can reach:Out of reach either way:
config/boot.rb, and deprecator-based warnings duringBundler.require, since loadingconfig/application.rbis what requires the gems.Measured against #194
Controlled, on freshly generated
rails new --minimalapps with two deliberate probes, one warning from an initializer and one from a model's class body:In every #194 run the initializer-time warning was printed to stderr and absent from the shitlist. Same result with
CI=1, where the generatedtest.rbeager-loads during boot.On a real dual-boot app (Rails 3.2 current bundle / 4.0 next bundle), same commit, same day, both configured to succeed:
Strict superset: nothing #194 finds is missing here. The 22 extra unique warnings are concentrated in models an initializer happens to reference (
Person,User,Contact,EmailInvitation, theCareManagement::Messagingcluster). Their class bodies are evaluated duringinitialize!, so by the time #194 runseager_load!the constants are already loaded and nothing warns again. The warnings are not late, they are already gone.Runtime is a wash: 26s vs 23s with warm caches. Both boot the app once.
Two further differences:
deprecations bootto capture load-time deprecations #194 needs an app config change on some apps. Wheneager_loadis truthy it refuses (correctly, rather than reporting a false clean) and tells the operator to setconfig.eager_load = false. It avoids that on stock modern apps by unsettingCIfor the boot, but not where the app arrives at a truthyeager_loadanyway, which is the default outcome for--nexton the app above. This PR needs no app edit:eager_loadeither way,deprecation = :raise, andreport_deprecations = falseare all handled, so the refusal path and theCIunsetting are both gone.deprecations bootto capture load-time deprecations #194 raisesNoMethodErroron Rails < 4.0, whereconfig.eager_loaddoes not exist. This PR works on 3.2.