[CMS-420] Preparing BrowserCMS for Rails 4.2 -> 5.0 upgrade - #16
Merged
Merged
Conversation
The patch reopened ActiveRecord::ConnectionAdapters::ColumnDumper to
override column_spec. Rails 5.0 changed that method's arity, so the
override stopped matching what Rails calls -- and because a schema dump
that emits nothing still exits 0, the failure was silent.
Measured on the 5.0 bundle before fixing: a real `rake db:migrate` dump
emitted 0 of 74 tables and exited successfully. Anyone running migrations
on Gemfile.next could have committed an empty schema.rb that looked fine
in review. That is why this went first in the phase rather than after the
ten red tests.
Guards the patch on ActiveRecord::VERSION::MAJOR < 5 rather than on
NextRails.next? -- it is reacting to the framework's implementation, not
to which bundle is booting, and those are different questions that happen
to coincide today.
schema_dumper_test.rb asserts the dumped *content*: the three weaker test
shapes ("does not raise", "output is non-empty", asserting on
column_spec's return value) all pass against the broken dumper, and the
file's header names them so they are rejected on the record rather than
by omission.
Criteria 8 and 9. Criterion 8 is met as amended -- the guard as the phase
document specified it would not have caught this, because ColumnDumper
still exists on 5.0; only the arity moved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…4.0) The next-rails job is green for the first time since Phase 2 made it gating. Ten failures, four root causes -- and three of the four were live defects on the 4.2 bundle that ships, not Rails 5 incompatibilities. Only the 5.0 suite happened to execute them. B.1 -- the cluster, 7 of the 10, one fix. create_content_table gives every versioned content table a lock_version, so optimistic locking is on everywhere. versioning.rb's after_save touch routinely runs against a record whose in-memory lock_version is behind (page_component.rb:31 and page.rb:256 both do it). 4.2 scoped the UPDATE by id alone and incremented from the stale value; 5.0 puts the locking column in the WHERE and raises. sync_locking_column_before_touch re-reads the column first, which keeps 4.2's outcome exactly and unblocks 5.0. This does NOT make optimistic locking work. A genuinely stale save still overwrites a concurrent edit, on both versions, exactly as it always has. That is a real data-integrity defect, left in place deliberately (D6) and pinned by a characterization test that fails if anyone makes conflicts raise. B.3 -- nine truthiness sites, six fixed. Phase 3 filed this as a single integer-cast failure. It is not: `if params[:some_id]` is true for a blank string, so a real `?some_id=` request was a 500 on 4.2 as well. Five of the nine have no test that would notice a mistake, so those are recorded rather than changed (D5). B.4 -- publishing.rb called quote_value with one argument against 4.2's two-parameter version. It raised ArgumentError on every call, and `publish`'s `rescue Exception` swallowed it, so publishing a non-versioned record silently did nothing for years. The test that should have caught it had been edited to agree with the bug; the assertion is inverted back with the history beside it. This was Tier B's B7, hidden by a bare rescue. B.5 -- content_controller's edit iframe lost every query parameter. An ActionController::Parameters-vs-Hash break (B9), found by a cucumber failure rather than by the B9 audit. B.6 -- PortletTest#test_.blacklist was order-dependent, not flaky: Cms::Portlet.blacklist memoizes into @Blacklist, so anything touching it earlier in the run meant the stub never applied. A job green only on lucky orderings is not green, so this had to close before criterion 13 could be claimed. Verified across seeds 1, 2, 3 and 7. Every fix has a characterization test passing on the Gemfile bundle (criterion 14). Where the cause was a 4.2 defect, the test asserts the corrected behaviour on both bundles rather than pinning 4.2 -- called out per item rather than blurred. ci.yml's comment goes from "red until Phase 4" to "gating, and green", and argues for keeping it gating now that it passes: it is not a Rails 5 canary, it is a second execution of the suite under different framework semantics, and it has found more 4.2 bugs than 5.0 ones. Criteria 13 and 14. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One hour of work, and the cheapest defence available against the largest
single item in the whole upgrade.
engine.rb:112-116 pushes nine paths onto
ActiveSupport::Dependencies.autoload_paths -- an API Zeitwerk does not
have at all. Zeitwerk replaces "search these directories for a missing
constant" with a strict mapping validated at boot: a file at
<root>/a/b_c.rb MUST define A::BC. Nothing checked that, so the 6.0 hop
would have discovered every violation at once, at boot, with no inventory.
eager_load! itself runs clean on both bundles, contrary to the stage's own
expectation. The value is the path-to-constant sweep: 128 files across 6
engine roots, one violation --
app/portlets/helpers/cms/list_portlet_helper.rb
path implies Helpers::Cms::ListPortletHelper
file defines bare top-level ListPortletHelper
missing both the Helpers:: and the Cms:: segments. It works today only
because portlet helpers are resolved by Rails' helper lookup at render
time, never by constant autoloading. Zeitwerk does not care how a constant
is reached. Recorded in KNOWN_ZEITWERK_MISMATCHES rather than fixed --
naming is 6.0 work -- and the test fails on any NEW violation, so the
allowlist cannot quietly grow. Proven in both directions.
Also measured, and it shrinks the 6.0 estimate considerably: of those nine
autoload_paths pushes, at most ONE is additive. Two point at empty or
missing directories and the rest duplicate paths Rails already globs.
Note for whoever reads this next to a coverage report: this test was
written here but deliberately held out of the gated suite until stage F,
because eager_load! loads files no suite otherwise touches and that widens
the coverage denominator. The file's header carries that history.
Criterion 5 (met in stage F, not here).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…4.1)
Adopts the existing audit file as the permanent version and closes its one
genuine hole.
The phase document's instruction to set
`config.active_record.belongs_to_required_by_default = true` in the test
environment first CANNOT be followed, and that is worth recording rather
than quietly skipping: the flag is read at class-definition time, so a
setup block is a no-op on 5.0, and the accessor does not exist at all on
4.2. The concern behind it is real, so criterion 3 is replaced by the
property it was reaching for -- the audit is made falsifiable instead.
Adding an unaudited belongs_to, contradicting a verdict, or moving the
count each fails a test (D1).
The missing assertion was a dynamic one, and it multiplies:
dynamic_attributes.rb:171 is reached from Cms::Portlet.inherited, so it
runs once per portlet subclass and every run does
`class_eval { belongs_to base_class, ... }` against the same
CmsPortletAttribute. That class accumulates one belongs_to per portlet
type -- four in this repo, plus one for every portlet a consuming project
defines.
Enumerates by reflection rather than a fixed list, so a declaration added
tomorrow is caught, and adds a count tripwire asserting
24 literal + 3 behavior-injected + 2 dynamic = 29, which makes criterion
4's stated check literally true (D2).
9 tests -> 11. Criteria 3 (as amended) and 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
create_content_table is the migration DSL every BrowserCMS migration and every downstream project's migrations go through, and it calls `create_table table_name, options, &block` -- the positional-options signature that changes at Rails 5+. Nothing ran the DSL at all. The DSL takes exactly two options, so "every option combination" is a 2x2 matrix and is now enumerated in full. Each case asserts the COMPLETE column set on both tables it creates -- the content table and the _versions table -- rather than one column's presence, plus the two asymmetries between them (lock_version is content-only, version_comment is versions-only) and the option pass-through to create_table that B3 flags. Asserting the complete set is the point: a sabotage that adds a column to one table is invisible to a test that only checks the columns it expects to find. Recorded because it nearly produced a false negative: the first sabotage run added version_comment to the content table with sed and silently did not match (10-space vs 12-space indentation), which looked exactly like the tests failing to catch it. Re-applied properly, it failed 6 tests. A sabotage that silently does nothing is indistinguishable from a test that does nothing, so every sabotage in this phase is now verified to have taken effect before its result is believed. 6 tests -> 13. Criterion 6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…(4.4)
Covers the four uncovered ActionController::Parameters sites (B9), takes
both Forms controllers off 0%, and re-baselines the branch coverage gate.
The two authorization sites are the ones that matter. pages_controller's
strip_visibility_params and sections_controller's group_ids deletion both
implement authorization by mutating a params sub-hash, and a regression
there fails OPEN -- a user who should not be able to set those fields
silently gets to. Both directions are asserted in every case: a test that
only checks the restricted user would pass if the strip ran
unconditionally and broke the feature for everyone; one that only checks
the privileged user would pass if the strip never ran. Neither half is
worth anything alone.
An ELEVENTH B9 site turned up at content_block_controller.rb:275, found
only because a 0%-coverage controller was finally instantiated.
`Hash#merge(Parameters)` coerces via to_hash, which 5.0 deprecates and 5.1
enforces -- so at 5.1 content blocks would have started silently losing
fields on save. `.to_unsafe_h` is explicit about what the code already
does and behaves identically on both bundles.
Three live defects found, characterized, NOT fixed -- each needs a product
decision and each test fails when someone repairs it:
- public form submission 500s. Cms::Form.layout does not exist, called
at form_entries_controller.rb:17 and :31. Every form showing
confirmation text, and every validation failure, returns 500 to an
unauthenticated visitor. The entry IS saved first, so no data is lost.
The most serious finding of the phase.
- the Forms admin UI 500s. Cms::Form.path does not exist either;
is_addressable is commented out. The same abandoned migration explains
the broken :form factory, which had never been called by anything.
- Cms::ToolbarController is vestigial: routed, no template, no layout.
This forces a correction to stage B -- one of those nine truthiness
fixes was to unreachable code, and the write-up should not be read as
implying otherwise.
COVERAGE GATE RE-BASELINED, 70.83% -> 70.49%. Read the note beside
COVERAGE_MINIMUM_BRANCH before "restoring" the old number.
The numerator never fell. Stage C's eager-load test widened the
denominator by six files no suite had ever loaded, so the old figure was
measured over a universe that silently excluded six untested controllers.
Line coverage moved the other way for the same reason, 78.44% -> 83.54%.
Closing it honestly was tried first: 18 new tests took it 69.54 -> 70.50.
The remaining five branches are in a controller with ZERO routes and in
view branches needing invented fixtures -- writing those to move a
percentage is exactly what the phase document rules out.
functional tests 89 -> 122. Criteria 5 and 7.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Answers B6's three questions and closes the partial fix deferred out of stage B. THE AUDIT (criterion 10). None of the three questions had an assertion anywhere in versioning_test.rb, against a file sitting at 96.91% line coverage. That gap between "almost every line ran" and "nothing checked what the lines did" is this phase's thesis in one example. failed validation -> no new version row CORRECT, now pinned rolled-back transaction -> no orphan row CORRECT, now pinned version_comment reflects THIS save WRONG on the CMS edit path Two of three were right all along and simply unguarded. Worth saying plainly: most of this file's value is a tripwire under behaviour that already works, not a defect count. versioning.rb does not override one method, it replaces the save call chain, and both its signatures were rewritten in Phases 1 and 2 -- it is the code most likely to shift again at a later hop, and until now nothing would have noticed. The third answer is "no", and the original author suspected it. versioning.rb:258-259 carries "This doesn't always seem to properly be applied, or is applying for ALL fields, not just the changed ones." It is right, and the cause is one line: build_object_from_version ends with a bare `clear_changes_information`, and `self` there is the Version record, not the obj being built and returned. So everything as_of_draft_version returns is dirty in every column, plus id/created_at/updated_at. That is the admin edit path -- pages_controller loads a draft object and updates it -- so every page edited through the CMS records a version comment listing the whole record. The history is intact and useless, which is why nobody has reported it. Second consequence: different_from_last_draft? short-circuits on changed?, so the "unchanged record, skip the save" branch never fires on the UI path. NOT FIXED. `obj.clear_changes_information` is a one-word change, but it also switches that skip-save branch on for the engine's busiest write path, where it has never run in any released version. Same line D6 drew. B.2 -- and there were TWO broken partial references in _main_form, not the one stage B named. version_conflict_diff on line 23 is broken the same way, so fixing only the named one would have moved the failure down eighteen lines and looked like a fix. Both now point at cms/application/. Each sabotaged separately to prove the tests catch them independently. The branch is unreachable through ordinary use -- versioning's create_or_update never issues an UPDATE against the page row, so the parent's lock_version is never checked on the write path, and B.1 removed the only thing that raised StaleObjectError. So the test raises the error the controller declares it rescues and lets everything downstream run for real. It is the render that was broken and the render that is tested. 14 + 5 tests. Unit suite 793 -> 808: the extra one over the 14 is namespaces_test.rb, which mints a no-op test per constant under Cms::. Criterion 10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
B7, B8 and B2. 28 tests, green on both bundles.
B2 WAS MISSING FROM THE IMPLEMENTATION PLAN. Work item 4.7 lists three
bullets and the plan's stage H carried two of them; the dynamic_attributes
chain was lost when the plan was written, not scoped out. It was recovered
by checking the stage against the phase document rather than against the
plan -- which is the reason the README calls the phase file the contract
and the plan an approach to it. It turned out to be the most productive of
the three.
B7 -- publish! writes content state with hand-built SQL, so every
assertion here reads the row back with SELECT. That distinction is the
whole item: publish! sets self.published = true in memory whatever the SQL
did, so an assertion against the object under test passes against a
completely broken write. Not hypothetical -- it is stage B.4 restated.
Covers the draft's values reaching the live row, both halves being marked,
no new version row, a no-op publish writing nothing, and the interpolated
WHERE touching only its own row. Deliberately does not assert API shape:
publishing.rb:161's two-argument connection.quote is removed at 5.1, and
these tests survive that.
B8 -- soft_deleting is already covered at the ActiveRecord level by
content_block_test.rb, so this targets only the parts that are not
ordinary ActiveRecord: that the startup `rescue StandardError` did not
swallow the default scope (its failure mode is a debug log line and
deleted content appearing everywhere), composition in both chaining
orders, that delete_all does NOT delete, and the alias-before-extend
ordering that is the only reason delete_all! is real.
B2 -- three disagreements between the alias chain and ActiveRecord, all
identical on both bundles, none caused by the upgrade:
- read_attribute/write_attribute are private on these models, and an
explicit-receiver call returns nil rather than raising, because
method_missing rescues the NoMethodError and looks up a dynamic
attribute of that name
- _read_attribute, the form ActiveRecord uses internally, was never
aliased
- nonversioned_class raises FrozenError in the only case it exists for:
`base_class = kls.name` then `sub!` mutates the frozen string
Class#name returns. Before Ruby froze it, this renamed the class.
Characterizations, not fixes. Each names why, and each fails when repaired.
A correction the sabotage caught: the first write-up blamed the
`private` at dynamic_attributes.rb:193, directly above the aliases and
what any reader reaches for. Removing it changes nothing. alias_method
ignores the ambient visibility and copies the target's, and the targets
are private from line 261. Recorded in the test file, because the next
person will make the same guess.
Fifteen sabotages run, each verified to have taken effect first. Two were
no-ops -- one hit a branch Cms::HtmlBlock does not take, one hit that dead
line -- and both are recorded struck rather than quietly re-run.
Branch coverage 70.49% -> 70.63%, measured twice on a cleared resultset;
COVERAGE_MINIMUM_BRANCH raised to match, under the same no-slack policy
stage F set it by. Line coverage 83.60% -> 83.67%. Unit suite 808 -> 837.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The documentation for the eight commits above, which accumulated across
every stage and cannot be split along the same lines.
phase-4-implementation-plan.md -- each stage written up with what was
found rather than what was intended, including where the plan was wrong:
stage C's deferral and its resolution, stage F's coverage re-baseline
reasoning, stage G's located one-line cause, and stage H's recovery of an
item this plan had dropped. R3 and R7 struck with what actually happened.
phase-4-characterization-tests.md -- the contract document, with its work
items checked off and annotated where reality diverged from the
instruction. Four of those are worth reading on their own:
- 4.1's preamble tells you to set belongs_to_required_by_default first.
That cannot be done; the flag is read at class-definition time and the
accessor does not exist on 4.2.
- 4.5's guard as specified would not have caught the real bug --
ColumnDumper still exists on 5.0, only column_spec's arity moved.
- 4.0's tasks_controller item blamed the integer cast; the cause was
truthiness, and it was nine sites rather than one.
- 4.0's missing-partial item named one broken reference; there were two.
Two work items remain open and are now labelled as such: B4 (Paperclip,
deliberately out of scope for this phase and needing a destination in
Phase 5 or 6) and Tier C's three error branches, which were missing from
the plan entirely -- the second item lost that way after B2 -- and are now
folded into stage I rather than carried forward.
NEW: D8, the first decision added mid-phase. It asks whether stage I
fixes or characterizes the move_to_position dedupe, and it opens by
correcting this plan's own statement of the rule. An earlier draft claimed
"a behaviour change to the shipping 4.2 bundle is not Phase 4's to make",
which the record contradicts -- stage B changed 4.2 behaviour ten times
and stage G changed it again. D6's actual line is narrower, and in
practice has meant: does anyone's WORKING behaviour change? A crash is not
a behaviour anyone depends on.
That correction came from review, and it changed the recommendation.
Also recovered: section_nodes_controller.rb:73 carries the repository's
only TODO(Phase 4) marker, left by Phase 3 and not tracked anywhere. Its
analysis is correct -- the .distinct binds inside the parens and cannot
dedupe across the union -- but Relation#uniq on 4.2 is an alias for
distinct, not Array#uniq, so Phase 3's rename was behaviour-preserving and
the defect is older than the upgrade. Stated explicitly, because anyone
finding a dedupe bug directly above a Phase 3 edit will assume otherwise.
README.md -- Phase 4 status through stage H, and Phase 0's criteria 1-2
marked unblocked now that the next-rails job is green.
All 718 internal doc links validated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tion dedupe Work item 4.7's final bullet. It was missing from the implementation plan entirely -- the second item lost that way after B2 -- and was recovered during the stage-H checkbox audit, then folded in here rather than carried to Phase 5. The three branches had already been FIXED by Phase 3 in 70b22bd, which converted both `render text:` calls to `render plain:` and move_to_position's `uniq` to `.distinct`. What was missing was the tests, and nothing had executed either branch afterwards, so the conversions were unverified. That matters because `render text:` and `Relation#uniq` are both removed at Rails 5.1. Each test asserts the CONTENT TYPE as well as the status: render text: "Fail" -> text/html render plain: "Fail" -> text/plain A status-only assertion passes against either and would have proved nothing about the thing we actually need to know at 5.1. Sabotage confirms it -- reverting either site to `render text:` turns a test red. THE DEDUPE IS FIXED, NOT CHARACTERIZED (D8). `.distinct` bound to the second relation only, so it was SELECT DISTINCT over rows already distinct and could not see across the two halves -- and duplicates between the halves are the only kind the method produces. It produced them on every move within a single folder, where both queries are the same query. Phase 3 spotted this and left the repository's only TODO(Phase 4) marker against it. Being precise about what Phase 3 did: `Relation#uniq` on 4.2 is an alias for `distinct`, not `Array#uniq`, and `.children` returns a Relation -- so that rename bound identically and changed nothing. The defect is older than the upgrade and is NOT a Phase 3 regression. D8 required reading the consumer before deciding. Sitemap.prototype.updateValuesOnSuccess (cms/sitemap.js:188) is pure assignment, so the duplicate was cosmetic and the fix is free. The marker is removed; the dedupe is now outside the union and keyed on id. THREE MORE DEFECTS, found by instrumenting the three branches. Both characterized, neither caused by the upgrade: - move_to_position's `rescue StandardError` interpolates node_to_move.node.name and target_parent.node.name, but both locals are assigned by SectionNode.find calls INSIDE the begin block. So whenever a find is what raised, the handler raises NoMethodError on nil and nothing catches it. No case was found where the JSON error branch renders at all. - form_fields_controller#update cannot fail. Three independent facts close every route: :name is the only validated attribute, it is assigned by before_validation(on: :create) so an update never recomputes it, and permitted_params is `super - [:name]` so a request cannot set it directly. Two wrong drafts of that test established this -- a colliding label returned 200, then a colliding name also returned 200 -- which is why all three facts are asserted rather than described. The branch is reached by stubbing so the render is still verified. section_nodes_controller_test.rb is the first coverage move_to_position has ever had: no unit test, no functional test, no feature. Branch coverage 70.63% -> 70.97%, gate raised to match under the same no-slack policy stage F set it by. functional tests 127 -> 139. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by a flake, not by looking for it, and in code Phase 4 has never
touched.
A full ci:test run during stage I failed on sitemap_test.rb's "pages"
test with two pages transposed -- identical timestamps, no tiebreaker --
and passed the next nine runs. #pages collected straight off ancestry's
`children`, which carries no ORDER BY, so PostgreSQL returned the rows in
whatever order it liked. Usually insertion order. Once, measurably, not.
It was the ONLY reader on Cms::Section that did not order its results.
child_sections (section.rb:71), visible_child_nodes (:143) and :226 all
chain `.in_order`, which is `order("position asc")` on SectionNode. That
is what makes the omission read as an oversight rather than a decision.
This is a behaviour change to the 4.2 bundle that ships and no Phase 4
work item covers it, so it was raised as a question rather than absorbed
-- the same way D8 was -- and fixed on the user's call. What made it
cheap to say yes to:
- Nothing in the engine calls it. `grep -rn "\.pages\b" app/ lib/`
finds only the test. A downstream caller gets a stable sitemap order
where it previously got an arbitrary one.
- #child_nodes is deliberately left alone. It is also unordered, but
every order-sensitive caller adds .in_order itself and the rest only
ask it for .count or .empty?.
The test now asserts position order AGAINST insertion order rather than
the set, so removing .in_order turns it red instead of leaving it to
luck. Verified by sabotage.
Worth keeping the general point: a test that passes nine runs in ten is
not green, it is unmeasured -- and this one had presumably been unmeasured
for years. It surfaced only because the plan requires running the full
chain after every stage, for an entirely unrelated reason.
Written up as D9 in docs/rails-upgrade/phase-4-implementation-plan.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The record for the two commits above. phase-4-implementation-plan.md -- stage I.1's findings, and two new decisions. D8 and D9 are the only decisions added mid-phase; D1-D7 were all written before anything ran. D8 (the move_to_position dedupe) opens by correcting this plan's own statement of its rule. An earlier draft claimed "a behaviour change to the shipping 4.2 bundle is not Phase 4's to make", which the record contradicts -- stage B changed 4.2 behaviour ten times and stage G changed it again. D6's actual line is narrower, and in practice has meant: does anyone's WORKING behaviour change? A crash is not a behaviour anyone depends on. That correction came from review and it changed the recommendation from characterize to fix. D9 (Cms::Section#pages ordering) records a defect this phase did not go looking for, and the reasoning for raising it rather than absorbing it. phase-4-characterization-tests.md -- work item 4.7's final bullet closed. B4 is now the only unchecked box in the contract document, and it is labelled as deliberately out of scope with a note that it needs a destination in Phase 5 or 6. All 732 internal doc links validated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes Phase 4. Both bundles green and identical: 838 unit / 145 spec / 139 functional / 7 orphan / 154 cucumber, 0F/0E. 13 of 14 criteria met, two of them as amended. phase-4-report.md -- the record, written to be read by someone who was not here. Its headline is deliberately not the green suite: Of the fifteen defects this phase found, FOURTEEN fail identically on Rails 4.2. None was caused by the upgrade. The upgrade was the excuse to look. That is also the argument, now recorded in ci.yml, for keeping the next-rails job gating after it went green: it is not a Rails 5 canary, it is a second execution of the codebase under different semantics, and it found more 4.2 bugs than 5.0 ones. The report also records where the phase documents were wrong -- two criteria that could not be met as written, two work items the implementation plan dropped, three diagnoses in the phase document that were wrong in instructive ways, and one correction to this phase's own stated rule that came out of review and flipped a recommendation. AN OPEN QUESTION, recorded rather than smoothed over. Criterion 1 is met with a caveat: two of roughly fourteen full ci:test runs during stage I failed on pre-existing intermittent unit tests. One cause was found and fixed (D9). The other was not isolated -- not reproducible in 15+ subsequent runs, not machine load, and not pollution from this phase's tests, which are transactionally rolled back. Suspected and unproven: the units suite runs two database-cleaning strategies in one process. publishing_mini_test.rb is a Minitest::Spec inside the units glob that truncates after every example, and test_helper.rb:42-52 already documents that arrangement causing order-dependent flakes once before. Handed to Phase 5 with a recommendation to resolve it BEFORE the bump, because a suite that fails one run in seven is at its most expensive when every red build is already suspect. Carried into phase-5-the-5.0-bump.md as a new work item 5.5a, so nothing lapses: B4 (the one work item Phase 4 did not do), the `or` half of B8 (ActiveRecord::Relation#or does not exist on 4.2 -- already measured on 5.0, the answer is attached), the cleaning-strategy fix, and tickets for the ten defects Phase 4 characterized but deliberately did not fix. Phase 5's manual-verification list updated: form_entries_controller now has 10 tests, and its known pre-existing 500 is flagged so it is not mistaken for a bump regression. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KHCode
changed the title
| @@ -0,0 +1,228 @@ | |||
| name: CI | |||
Member
There was a problem hiding this comment.
Looks like the tests aren't running on Github.
Also, it looks like these tests are running against Ruby 2.7.8, which is incorrect. CMS currently runs against 3.1.6, so we should start there, but presumably we'll try to push that higher.
rlee21
approved these changes
Sep 17, 2026
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
4 participants
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.
CMS-420 Rails 4.2 → 5.0 upgrade: baseline through characterization tests (Phases 0–4)
Prepares
browsercmsfor the Rails 5.0 bump. This branch does not bump Rails —Gemfileis still 4.2.11.3 and still ships. What it does is make the upgrade provable: a second bundle (Gemfile.next) boots and runs the full suite on Rails 5.0.7.2, and both bundles are now green and identical for the first time.Phases 0–4 of the plan in
docs/rails-upgrade/are complete. Phase 5 is the bump itself.All I really expect in a review is to run the tests on the 4.2 bundle and the 5.0 bundle and verify that all tests pass in both.
What each phase did
Phase 0 — Baseline and CI. Got the suite running and wrote down exactly what it did (994 tests, cucumber 154/154, 75.82% coverage) before changing anything. Wired the orphaned test files into the rake chain —
test/*_test.rb,test/helpers/**and the dummy app's own tests were in the repo but run by nothing — and replaced the dead Travis config with GitHub Actions.Phase 1 — Gem compatibility and dual-boot. Resolved
Gemfile.nextonto Rails 5.0.7.2 with no unfixable gem blockers. The important find wasn't in the gem list:test/dummy/config/boot.rbreassignedBUNDLE_GEMFILEunconditionally, so every spawned test process silently fell back to 4.2 — dual-boot would have reported green while testing the wrong Rails.Phase 2 — Harness migration.
factory_girl→factory_bot, 89 positional controller-test calls → keyword form, mocha requires,rails-controller-testing, dummy-app config, and SimpleCov 0.12 → 0.22 with branch coverage enabled. Took the 5.0 unit suite from 323 errors to 3; every remaining 5.0 failure was application behaviour, not test plumbing.Phase 3 — Backwards-compatible fixes. Applied the version-neutral source changes that can land before a bump:
update_attributes→update, explicitbelongs_to ... required: false,render text:→render plain:,uniq→distinct, and the Rails 5 asset chain. Took the 5.0 functional suite from a load error to 85/88 and cucumber from 6/154 to 150/154.Phase 4 — Characterization tests. Cleared the last ten 5.0 failures and pinned the behaviours Rails 5 changes silently, so future semantic drift fails loudly instead of quietly. Both bundles now run identically green. The phase found fifteen defects along the way — fourteen of which fail identically on Rails 4.2 and were never caused by the upgrade. Ten are left deliberately unfixed, each pinned by a test that goes red when the behaviour is repaired; see
phase-4-report.md§3.Results
Coverage instrument changed in Phase 2 (SimpleCov 0.12 → 0.22), which re-read the same covered lines as 78.35%. Against that comparable baseline the move is 78.35% → 83.94%.
Both CI jobs are gating. The
next-railsjob stays gating now that it's green — it is not a Rails 5 canary so much as a second execution of the suite under different framework semantics, and it has found more 4.2 bugs than 5.0 ones. The reasoning is recorded inci.yml.Running the tests
RAILS_ENV=testis required on every command below. Without it:ActiveRecord::AdapterNotSpecified: 'development' database is not configured.Rails 4.2 (the bundle that ships)
RAILS_ENV=test bundle exec rakerake→ci:test→db:drop db:create:all db:install test, thencoverage:check. It drops and reseedsbrowsercms_test.Rails 5.0
RAILS_ENV=test BUNDLE_GEMFILE=Gemfile.next DISABLE_DATABASE_ENVIRONMENT_CHECK=1 bundle exec rakeDISABLE_DATABASE_ENVIRONMENT_CHECK=1is needed whenever you switch bundles locally. Rails 5'sdb:droprunscheck_protected_environmentsfirst, which requires thear_internal_metadatatable — a Rails 5 addition that the 4.2 bundle doesn't create. So a database last built by 4.2 makes 5.0 abort withActiveRecord::NoEnvironmentInSchemaError. CI never sees this because each job gets a fresh Postgres container.It's safe here: the check exists to stop you dropping production, and
test/dummy/config/database.ymldefines exactly one database,browsercms_test. The alternative one-shot fix isbundle exec rake app:db:environment:set(note theapp:namespace — this is an engine, so Rails' suggestedbin/rails db:environment:setdoesn't apply as written).One suite at a time
There is no
functionalororphanstask, andrake app:testexits 0 having run nothing — it looks like the answer and isn't.One file
rake coverage:checkwill fail against the 83.94% / 70.97% gate until you re-run the full suite. Stale artifact, not a regression — but it looks exactly like one.Next steps to Rails 5.0
1. Seven bug-fix tickets (filed separately). Phase 4 left ten live defects characterized but unfixed, bundled into seven tickets by the decision each one needs. They are not upgrade work and should not be fixed during the bump — mixing a pre-existing defect fix into an upgrade makes every regression ambiguous. Each ticket's acceptance criteria is already written: the characterization test goes red when the behaviour is repaired, and its comment block says what to do.
publishswallowsException, including programming errors2. Phase 5 — the bump itself. See
phase-5-the-5.0-bump.md. Swap the version, review config, sync CI, and decide theload_defaultscontract (the real work — this is a gem, so the decision is about which host values are supported). Three items carried in from Phase 4, in item 5.5a:ActiveSupport::TestCaserolls back in a transaction whilepublishing_mini_test.rb— aMinitest::Specinside the units glob — truncates after every example. Two intermittent failures across ~14 full runs, only one isolated. A suite that fails one run in seven is at its most expensive during a bump, when every red build is already suspect.validates_attachment_presenceis defined twice, and the first is dead code.orhalf of thesoft_deletingscope test —ActiveRecord::Relation#oronly exists on 5.0, so it couldn't be written on this branch. Already measured as correct; one line once the bump lands.