[swicken]

Proposed Changes

Integration tests only run in CI when they are listed in a suite (MainSuite1a/1b/2a/2b/3a, Junit5Suite1, OpenSearchUpgradeSuite). Everyone appended new tests to the end of the list. Appending edits the last line, to add a comma, so any two PRs adding a test to the same suite conflicted. In the merge queue that shows up as PRs bumped as unmergeable. On 2026-10-28, six queued PRs (#38282, #38180, #38252, #38270, #38244, #38224) each appended to the end of MainSuite2a. Every one after the first was bumped, even though each merged cleanly with main on its own.

• just add-it-test <TestClass> <Suite> (scripts/it-suites.py add) registers a test without touching the end of the list:
  • It inserts the entry next to the existing entry with the nearest simple name. The lists are unsorted and are not re-sorted, so the existing test order is unchanged.
  • It writes the entry fully qualified, with no import. The import blocks are unsorted too, so new imports would collide at the end of the import block.
  • Every entry gets a trailing comma, so an insert never edits a neighbouring line.
  • It refuses: a class already registered in any suite (and names that suite); an unknown suite (and lists the CI suites from .github/test-matrix.yml); a JUnit 4 test in the JUnit 5 suite, or the reverse. JUnit 4 or 5 is decided by which @Test annotation the class imports. It also refuses an ambiguous simple name.
  • Suite files it cannot parse safely make it stop with an error instead of being edited.
• One-time trailing comma on the last entry of the suites that lacked one. The order is otherwise unchanged.
• A CI guard, cicd_pr_it-registration.yml. It runs on PRs, needs only a sparse checkout and no stack, and finishes in seconds. It runs the script's unit tests and then it-suites.py check, which fails when a concrete integration test class is in no suite. Today such a class compiles and is silently never run.
  • 77 such classes exist on main today, including SpeedyAssetServletTest, ReindexQueueAPITest, ApiTokenResourceTest, HTMLPageAssetAPIImplTest and several Task*Test. They are seeded into dotcms-integration/unregistered-tests.allowlist, so the guard only catches new ones.
  • The allow-list can only shrink: check fails if an allow-listed class becomes registered or is deleted.
• Docs. CLAUDE.md, docs/testing/INTEGRATION_TESTS.md and .claude/commands/gh-issue-troubleshoot.md now say to register with just add-it-test and never by hand.
• Retires scripts/validate-integration-test-registration.sh. The new guard supersedes it, and it was not wired into CI. Its one reference, in docs/BACKEND_GAP_BACKLOG.md, is updated.

Fixes #38318

Checklist

• Tests
• Translations
• Security Implications Contemplated (add notes if applicable)

Additional Info

Merge proof. Six throwaway branches each added one of the 2026-10-28 MainSuite2a classes with the tool. 14 of the 15 pairs merge cleanly with git merge-tree, and five cumulative merge orders were clean at every step. The one conflict is ContentMapWidgetFieldRenderTest with ContentRelationshipsResourceIntegrationTest: no existing entry sorts between those two names, so any name-based placement puts them on the same spot. That is rare, and the docs say so. Under the issue's original rule ("first larger name from the top"), 4 of the 6 landed on the same line, because the lists are unsorted. That is why the tool uses nearest-name placement and fully qualified entries.

Tests. python3 scripts/it-suites.test.py runs 15 unit tests on a fixture tree. They cover insertion, trailing commas before comments, refusal of unparseable shapes, JUnit 4 tests that import Jupiter Assertions, nested abstract classes, and stale allow-list entries. python3 scripts/it-suites.py check passes on this branch, and every matrix suite parses. The new workflow has not run on GitHub yet; this PR is its first run.

Not included: sorting the existing suites, which would reorder tests and surface order dependencies in bulk, and suite discovery.

This PR fixes: #38318

[claude]

Claude finished @swicken's task in 2m 52s —— View job

---

Code Review

I reviewed the diff against origin/main, tracing the add/insertion_line/check logic in scripts/it-suites.py and the 19 unit tests. No new correctness or security bugs. The design holds up on the edge cases I pushed on:

• Trailing-comma invariant is safe. The only entry that may lack a comma is a suite's current last entry, and the PR's one-time fix gave all of them commas. An insertion whose preceding line (at-1) is a comment can never sit after a comma-less entry, because any entry followed by a comment-then-entry is not the last entry and already has its comma. So add cannot emit invalid Java (FTest.class\n// c\nNew.class).
• Grouped ("run FIRST") blocks are never used as insertion anchors (self.grouped excluded from free), and inserting after the last free entry when a grouped block follows lands before the block with the anchor line already comma'd — confirmed by test_insertion_never_lands_inside_a_comment_headed_group.
• JUnit 4/5 detection resolves wildcard imports and Jupiter-Assertions-in-a-JUnit4-test correctly (test_junit4_*), and add refuses a version mismatch both ways.
• check fails closed on an unparseable suite (SystemExit) and on stale allow-list entries; the shrink-only contract is enforced by stale_allowlist.

I could not execute python3 here (sandbox denied), but fabrizzio-dotCMS confirmed it-suites.test.py 19/19 and check passing on this branch.

New Issues

• 🟡 Medium: .cursor/rules/test-context.mdc:23 and .github/copilot-instructions.md:87 — both still describe registration generically ("register each new or moved class exactly once") and point at the doc without naming just add-it-test. Now that cicd_pr_it-registration.yml fails a PR with an unregistered class, an agent reading only these files will hand-edit the @SuiteClasses list — the exact merge-queue hotspot this PR removes — and won't know CI now blocks on it. One line each, matching the CLAUDE.md wording. (Same as the human reviewer's request Add .gitignore #3; verifiable from the two files.) Fix this →

Non-blocking; nothing here blocks merge. The substantive follow-ups (measured residual-collision numbers for the PR body/INTEGRATION_TESTS.md, and an owning ticket for the 77 seeded classes referenced from the allow-list header) are documentation/process items already captured in fabrizzio-dotCMS's approving review, not defects in this diff.

· branch issue-38318-suite-registration-script

[fabrizzio-dotCMS]

Approving. The design is right and the implementation is careful: the one-time trailing comma, the fully qualified entry with no import, and the fail-closed parser each remove a distinct hotspot, and it-suites.py aborts rather than editing a suite file it cannot parse safely. I verified it locally on this branch — python3 scripts/it-suites.test.py is 19/19 OK, python3 scripts/it-suites.py check passes, and just add-it-test com.dotcms.cache.DotJSONCacheTest MainSuite2a produces a one-line insertion in MainSuite2a.java that touches no existing line, plus the allow-list removal. JUnit 4/5 detection resolves unambiguously for all 77 allow-listed classes, so add will not refuse any of them when someone goes to register one.

Two things I would like addressed before merge, and three optional ones.

1. "That is rare" understates the residual conflict rate — please replace it with a measured number.

The merge proof uses six names. I ran the same placement over a larger realistic sample: the 77 simple class names in dotcms-integration/unregistered-tests.allowlist, which are real classes from this repo. Those 77 names map to only 29 distinct insertion lines in MainSuite2a, with 12 of them landing on the same line.

Drawing six names at random (20k trials per suite), the probability that at least two of six concurrent PRs still collide is:

suite	entries	free	≥1 collision among 6 PRs
MainSuite2b	318	311	20%
MainSuite1b	96	96	46%
MainSuite2a	84	83	52%
MainSuite1a	80	77	52%
MainSuite3a	71	62	62%
Junit5Suite1	33	33	98%

Reproduce with:

import importlib.util, pathlib, collections, random
spec = importlib.util.spec_from_file_location('it','scripts/it-suites.py')
it = importlib.util.module_from_spec(spec); spec.loader.exec_module(it)
root = pathlib.Path('.')
names = [l.split('#')[0].strip().rsplit('.',1)[1]
         for l in open('dotcms-integration/unregistered-tests.allowlist')
         if l.strip() and not l.startswith('#')]
random.seed(7)
for s in it.suites(root):
    free = [(i, w.rsplit('.',1)[-1]) for i, w, _ in s.entries if i not in s.grouped]
    pts = {n: it.insertion_line(free, n) for n in names}
    hit = sum(len({pts[n] for n in random.sample(names, 6)}) < 6 for _ in range(20000))
    print(f'{s.name:22} {100*hit/20000:5.1f}%')

This is not a bug — the insertion point is the alphabetic successor, so two names collide exactly when no existing entry falls between them, and that is driven by how densely the existing names cluster in the alphabet. It is still a large improvement over today, where all 15 pairs of 6 conflict. But "That is rare, and the docs say so" is not what the data says, and Junit5Suite1 is essentially unimproved: 33 entries give only 8 distinct insertion points. Worth stating the real expectation in the PR body and in INTEGRATION_TESTS.md, so nobody reads an occasional queue bump as a regression in the tool. If you want to close Junit5Suite1 too, the cheap tie-break is to derive the offset within the gap from the fully qualified name rather than the simple one.

2. The 77 dead test classes need a ticket.

This is the most valuable thing the PR uncovered and the only place it leaves unrecorded debt. ApiTokenResourceTest, ReindexQueueAPITest, HTMLPageAssetAPIImplTest, SpeedyAssetServletTest and several Task*Test are coverage we believe we have and do not. Seeding them is the right call — they cannot be fixed here — and the shrink-only design is good (stale_allowlist fails the build once an entry becomes registered or disappears, and add removes the line for you). But nothing will push the list down without an owner. Please link or open an issue to drain it, and reference it from the header comment in unregistered-tests.allowlist.

3. Two agent-instruction files were left behind (#38283 touched five, this PR touches three).

.cursor/rules/test-context.mdc:23 and .github/copilot-instructions.md:87 still describe registration generically ("register each new or moved class exactly once") and point at the doc without naming the command. They do not contradict this PR — neither says "append" — but an agent reading only those will hand-edit the list and will not know CI now fails on an unregistered class. One line each, matching the CLAUDE.md wording.

---

Optional, none blocking:

• cicd_pr_it-registration.yml triggers on pull_request: branches: [main] only, so a PR against release-*_lts never runs the guard. Consistent with those branches not running the build, but at ~11 s it is close to free to include them.
• unregistered() skips any file where BLOCK_START matches anywhere in the text, including inside a comment, while Suite.__init__ correctly excludes lines starting with // or *. A test class that mentions @SuiteClasses in its Javadoc would escape the guard silently. Today only Junit5Suite1.java has such a comment, so it is theoretical — but the asymmetry between the two call sites is free to remove.
• The docstring in Suite.add says a list made only of ordered groups gets the new entry after its last entry; the fallback free or [(self.entries[-1][0], "")] inserts it before. No suite reaches that branch today (the largest grouped count is 9 of 71 in MainSuite3a), so it is a comment fix.

[dotCMS-Machine-User]

✅ dotbot review: all reviewer models (meta/muse-spark-1.3, ~z-ai/glm-latest) agree — patch is correct.

_{approved automatically by dotbot}

[dotCMS-Machine-User]

✅ dotbot review: all reviewer models (meta/muse-spark-1.3, ~z-ai/glm-latest) agree — patch is correct.

_{approved automatically by dotbot}

[github-actions]

dotbot code review:

• Reviewer: meta/muse-spark-1.3 (medium)
• Overall: patch is correct
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 0

New it-suites.py add/check, workflow guard, and trailing-comma edits verified; all suites parse, unit tests and check pass, with no introduced correctness or security defect.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · meta/muse-spark-1.3 · medium}

[github-actions]

dotbot code review:

• Reviewer: ~z-ai/glm-latest (medium)
• Overall: patch is correct
• New findings this run: 0
• Prior unresolved dotbot findings still relevant: 0
• Active findings total: 0

Verified scripts/it-suites.py add/check logic, the CI workflow, justfile recipe, and seeded allow-list: the 20 unit tests pass and it-suites.py check passes on this branch. Trailing-comma, insertion-anchor, grouped-block, JUnit 4/5, and stale-allowlist edge cases are handled and tested; no correctness, security, or concurrency defect introduced by this patch.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · ~z-ai/glm-latest · medium}

[dotCMS-Machine-User]

✅ dotbot review: all reviewer models (meta/muse-spark-1.3, ~z-ai/glm-latest) agree — patch is correct.

_{approved automatically by dotbot}