test: platform guards for two matrix cells the PR lane never runs - #283
Merged
Conversation
ActiveSupport::Testing::TestsWithoutAssertions does not exist in Rails 7.1, so the rails71 matrix cell died with NameError at class-definition time -- eight errors, on master, from a test added for #270. Skip rather than stub. A hand-rolled stand-in would assert that our code cooperates with a module Rails never prepends on that version, which proves nothing and reads as coverage. Guarded on rather than a version comparison: the question is whether the constant is there to prepend, and edge/main moves independently of the version string. Verified both ways -- constant forced absent: 3 skips; present: 0 skips, 15 assertions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe test setup detects whether Rails provides ChangesRails assertion compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized test-only change skips a Rails-specific assertion check on Rails 7.1 while retaining it on supported versions where the feature exists. The reported checks pass in both configurations, so no actionable merge-blocking risk remains beyond normal review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the missing-assertions tests to support Rails 7.1 by capability-detecting File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Every JRuby cell in the last 15 Test runs was `cancelled` -- 29 cancelled, 1 failure, zero successes. Fail-fast killed the lane before it could report, and `cancelled` reads as an absence rather than a verdict, so nothing said the lane had been dark since #266. Fixing the rails71 failure in the previous commit let JRuby run far enough to report, and it reported six errors: NotImplementedError: fork is not available on this platform org/jruby/RubyKernel.java:2073:in 'fork' test/unit/parallel_report_merge_test.rb:224:in 'fork_worker' All six route through `fork_worker`. This is not a product bug: Rails' `parallelize(workers: N)` forks, JRuby has no fork, and JRuby suites parallelize with threads instead -- which record in the process that finalizes and are already covered by "merging is a no-op when no worker ever forked". The behaviour is inapplicable there, so a skip is the honest report. Detection measured rather than assumed, on jruby-10.0.6.0: Process.respond_to?(:fork) => false Process.fork { } => NotImplementedError so the plain idiom is enough; no RUBY_PLATFORM sniffing. Verified both directions, because a guard that skips everywhere would be worse than the bug: MRI 4.0.6 14 runs, 68 assertions, 0 failures, 0 skips JRuby 10.0.6.0 14 runs, 34 assertions, 0 errors, 6 skips and the full unit suite on jruby-10.0.6.0 + rails72_gems.rb is 720 runs / 2090 assertions / 0 failures / 0 errors / 6 skips, so these six were the only thing broken in that lane. CI's own run of the superset (`bin/rake test`, 757 runs) failed on exactly these six and nothing else.
pftg
changed the title
This was referenced Aug 24, 2026
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
…t support (#288) Rails 7.1's gemspec says `required_ruby_version >= 2.7.0`. That is a floor with no upper bound, not a statement of support: 7.1 shipped in October 2023, years before Ruby 4.0 existed, and Rails never tested the pair. We have been propping the combination up rather than testing it: - `gemfiles/rails71_gems.rb` carries an explicit `require: %w[logger ...]` purely because Ruby 3.5+/4.0 dropped logger from the default gems; - #283 added a `defined?` guard for `ActiveSupport::Testing::TestsWithoutAssertions` on the same cells. Each was a real fix, but together they mean a red there is upstream's unsupported configuration, not our defect -- and today that cell blocked a release twice while every supported combination was green. Rails 7.1 stays covered on 3.3, 3.4 and jruby-10.0, which Rails does support. The #283 guard stays: 7.1 lacks that constant on every Ruby, not just 4.0. Open question left deliberately: 3.4 + Rails 7.1 is also outside what Rails formally lists, though 7.1.5+ carries 3.4 fixes. Not touched here -- that call belongs with whoever sets the support policy, and unlike Ruby 4 it is not currently costing us anything.
pftg
added a commit
that referenced
this pull request
Aug 24, 2026
…289) `jruby-10.0/rails80` timed out on master run 32763256451 and took the whole run red. Tests were still printing dots when SIGTERM landed, so the cell was SLOW, not hung -- and the same cell had passed the previous three runs. The 15-minute per-attempt budget was sized from run 32643567648, where a clean JRuby attempt was 545-713s: "~26% headroom over the slowest", as the comment says. That measurement is stale. Re-measured on run 32758898367, the last green one before this bit: rails71 652s rails81 740s rails80 870s rails72 881s <- against a 900s cap The suite grew from 646 to 757 runs in between (#274, #277, #278, #279, #283), and the headroom went with it: 881/900 is 2%. The cells have been passing by seconds, which is why this looked stable for three runs and then was not. 20 minutes restores the ~26% margin this was originally sized for, and the job cap follows to keep the arithmetic true: 1 + 20 + 20 = 41. Both numbers move together on purpose -- a per-attempt timeout that does not fit the cap kills the last attempt mid-run and reports `cancelled`, which reads as an absence rather than a failure. MRI is unchanged: 128s against 3 minutes. Costs nothing on a green run; it is a ceiling, not a sleep.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
…281) (#292) The matrix stays off PRs for free-tier minutes, and the consequence is that the job which breaks master is a job that never ran on the PR. Three breakages this week, all invisible on a green PR for exactly that reason: - a Rails 7.1-only constant (#283) - JRuby not implementing Kernel#fork (#283) - a zero-width skip_area mask (#280) Every one was found by adding `full-ci` BY HAND after master had already gone red, which is a process that works only when someone remembers. All three came from `test/`, `gemfiles/` or `.github/`, so the matrix now runs automatically when a PR touches those. The label stays for everything else. Paths are asked for over the API rather than `git diff`: checkout is depth-1, so the base commit is not in the clone to diff against. `lib/` is deliberately NOT on the list. It changes on nearly every PR, and the functional and minimal-setup jobs already cover it -- putting it here would run 25 cells on almost everything and give back the cost decision the exclusion exists to make. That is a trade, not a claim that `lib/` is safe, and CONTRIBUTING.md says so. CONTRIBUTING.md also gets the two reading rules that cost real time this week: `cancelled` is not a pass, and `gh run list --branch master` without `--workflow Test` will hand you whichever workflow ran last -- it reported a Dependabot success while `Test` was failing on the same commit. Detection self-tested against real paths; YAML validated.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
Measured on run 32770212615 first: 130 min/run, and JRuby was 56% of it -- five
cells at ~15 min proving the same JVM facts five times.
22 cells -> 12 master ~130 min -> ~46 min
- Ruby x Rails: 16 cells -> 6. A spanning set, not a cross product: every Ruby
once, every Rails once, oldest Ruby against both the oldest and newest Rails.
The cross product re-proved facts -- TestsWithoutAssertions being Rails 7.2+
(#283) shows on every Ruby running 7.1; JRuby lacking Kernel#fork (#283) shows
on every Rails. Coverage asserted by script, counting functional-test as
4.0 x rails81.
- JRuby: 5 cells -> 1, on jruby-10.1, newest Rails. cuprite and vips are already
what it gets and the comment says so rather than a matrix key restating a
default.
- Drivers: 6 cells -> 2. Both screenshot backends, which is the job's purpose,
on the cheapest Capybara driver.
- PRs run only the critical jobs: the automatic path-based full-ci trigger is
removed, the label stays as a manual opt-in.
Deliberately NOT in this change, having tried them: a weekly job re-running
everything cut (that is not a reduction, and `schedule:` already fires this
workflow), a CI Gate aggregator, a permissions block, and fail-fast: false.
Each was defensible and none was asked for.
What is given up: Ruby x Rails pairs outside the span, selenium as a Capybara
driver, Rails edge and jruby-head are not run anywhere now. #285 lives on
selenium_headless and stops being observed; workflow_dispatch reproduces it.
pftg
added a commit
that referenced
this pull request
Aug 25, 2026
22 cells -> 12; master run 130 min -> ~46. - Ruby x Rails 16 -> 6: a spanning set, every Ruby once and every Rails once. The cross product re-proved the same facts (#283 twice). - JRuby 5 -> 1, on jruby-10.1. Its differences are JVM-level, not Rails-level. - Drivers 6 -> 2: both screenshot backends, cheapest Capybara driver. - PRs run the critical jobs only; `full-ci` stays as a manual opt-in. Not run anywhere now: pairs outside the span, selenium, edge, jruby-head.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two test-only platform guards. Both fix cells that the PR lane never runs, which is why
neither was visible until master went red.
1.
TestsWithoutAssertionsis Rails 7.2+ (rails71 cells)ActiveSupport::Testing::TestsWithoutAssertionsdoes not exist in Rails 7.1, so the #270test died at class-definition time:
Guarded on
defined?and skipped rather than stubbed — a hand-rolled stand-in wouldassert that our code cooperates with a module Rails never prepends there, which is green
for the wrong reason.
Verified both ways: constant forced false → 3 skips; present → 0 skips, 15 assertions.
2.
Kernel#forkdoes not exist on JRuby (jruby cells)Once (1) stopped cancelling the lane, JRuby ran far enough to report six errors, all
routed through
fork_worker:Rails'
parallelize(workers: N)forks; JRuby has no fork and parallelizes with threads,which record in the process that finalizes and are already covered by "merging is a no-op
when no worker ever forked". The behaviour is inapplicable, so a skip is the honest report.
Detection measured on jruby-10.0.6.0 rather than assumed —
Process.respond_to?(:fork)isfalse, so the plain idiom suffices and noRUBY_PLATFORMsniffing is needed.The JRuby run above is the check that these six were the only breakage in that lane.
Why this was invisible
Across the last 15
Testruns the JRuby cells were 29 cancelled, 1 failure, 0 successes.Fail-fast killed the lane before it could report, and
cancelledoccupies the same visualslot as a verdict while carrying none. The lane has effectively been dark since #266.
This PR carries the
full-cilabel so the matrix actually runs here instead of after merge.Not fixed here
Test Drivers (selenium_headless, chunky_png)is flaky (4 success / 1 failure over recentruns) and unrelated to these changes — filed as #284, together with the shared screenshot
name that overwrites the failure's own evidence.