docs(testing): fix MainSuite registration rule and audit INTEGRATION_TESTS.md - #37583
Merged
jcastro-dotcms merged 2 commits intoSep 17, 2026
Merged
jcastro-dotcms merged 2 commits into
jcastro-dotcms merged 2 commits into
Conversation
The "Registering Tests in a MainSuite" section documented a fabricated rule (register new integration tests by package/feature sibling grouping). The real rule, per Team Falcon's lead: register in whichever MainSuite* currently has the shortest CI runtime, since the suites run in parallel and CI wants their wall-clock times balanced. Replaced the fabricated rule with the real one, including the method to check current runtimes (sample >=3 recent PR CI runs, not one) and a note that the suite set itself isn't fixed/exhaustive. Also corrected suite registration to append new entries at the end of @SuiteClasses (not alphabetized/grouped), matching the real MainSuite2b.java. Also audited every code example in the file against real source and fixed: - ContentletAPI.findByIdentifier(...).isPresent() is fabricated; real method is findContentletByIdentifier(...), which returns a Contentlet directly and throws DotContentletStateException when not found. - createTestContentlet() called with no args in one example but defined with a required ContentType arg in another. - Several examples referenced systemUser/contentletAPI/workflowAPI/ defaultLanguage as if inherited fields without declaring or initializing them anywhere. - DataProviderRunner/DataProviderWeldRunner usage counts and the *Test.java/*IT.java ratio had drifted from real counts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
11 tasks
Contributor
|
Claude finished @jcastro-dotcms's task in 1m 50s —— View job Review:
|
| Claim in diff | Source check | Result |
|---|---|---|
findContentletByIdentifier(id, false, langId, user, false) returns Contentlet, throws DotContentletStateException |
ContentletAPI.java:234 — public Contentlet findContentletByIdentifier(String, boolean live, long languageId, User, boolean) throws ... DotContentletStateException |
✅ signature & throws match exactly |
APILocator.systemUser() |
APILocator.java:1003 |
✅ exists |
contentlet.setHost(Host.SYSTEM_HOST) |
Host.java:58 SYSTEM_HOST = "SYSTEM_HOST" (String); Contentlet.setHost(String) at Contentlet.java:1093 |
✅ valid — sets system-host identifier |
New entries appended at end of @SuiteClasses, not alphabetized |
MainSuite2b.java:572-574 — RoleResourceUsersIntegrationTest / ReindexDeleteJournalTest are the last real entries; example matches |
✅ accurate, incl. the two anchor entries |
DataProviderRunner 68 usages |
68 files with @RunWith(DataProviderRunner.class) |
✅ |
DataProviderWeldRunner ~41 usages |
40 @RunWith matches, 41 total references |
✅ "~41" is defensible |
~680 *Test.java to ~19 *IT.java |
find → 680 / 19 |
✅ exact |
The MainSuite section rewrite (register into the shortest-runtime suite, sample ≥3 recent runs, suite set not fixed/exhaustive, gh pr view --json statusCheckRollup recipe) is internally consistent and self-caveats correctly against hardcoding a "fast suite." The createTestContentlet(contentType) fix resolves the prior self-contradiction, and the added @Before setups now declare/initialize every field the examples reference (contentletAPI, systemUser, defaultLanguage, workflowAPI).
New Issues
No issues found. Every referenced class, method, signature, constant, and count in the diff matches current source.
· issue-37128-integration-tests-registration-and-audit-fixes
- *IT.java count was still stale after the count-refresh pass: real count is 19, not ~12 (several *IT.java files landed since the last check). - DataProviderRunner count regressed from accurate to inaccurate: the 68-usages number (grep on @RunWith(DataProviderRunner.class)) was correct all along; my ~74 count grepped the import statement instead, which also matches DataProviderWeldRunner.java, a second/unrelated CustomDataProviderRunner.java, and a few tests that import it without using it as their runner. - The MainSuite2b registration example appended a class that's already registered there (WebAssetResourceV2IntegrationTest, line 423) — copying the example literally would double-register a real test. Swapped in a hypothetical placeholder class instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
nollymar
approved these changes
Sep 17, 2026
jcastro-dotcms
deleted the
issue-37128-integration-tests-registration-and-audit-fixes
branch
September 17, 2026 15:58
fabrizzio-dotCMS
added a commit
that referenced
this pull request
Sep 17, 2026
…te detection Review feedback from @nollymar and @jcastro-dotcms on PR #37583: the Cursor rule restated suite mechanics that had just changed, and any doc touching integration tests should point at docs/testing/INTEGRATION_TESTS.md instead. #37583 established that the suite to register in is whichever MainSuite* has the shortest current CI runtime, and that the suite set is explicitly not fixed. The rule now gives the consequence (unregistered = silently never run in CI) and defers the choice to the doc, with a line telling the reader not to hardcode suite names here — the same drift the Java-version fix in this PR addresses. Two bugs in the script, found while acting on that feedback: - The suite list was a hardcoded glob of MainSuite*/Junit5Suite*, which missed OpenSearchUpgradeSuite (a real CI suite, 19 classes) and would have counted a future MainSuite3b as nonexistent. It now reads the suite list from .github/test-matrix.yml, which is what CI actually runs — so QuickSuite, which exists in the tree but is not in the matrix, correctly does not count. - Class extraction matched every X.class token in the file, per the bot review. Anchoring it to the @SuiteClasses block then silently dropped all of MainSuite3a, which spells the annotation @Suite.SuiteClasses. Both forms are now matched, and the script aborts if any CI suite yields zero classes rather than reporting its tests as unregistered. Corrected figures: 609 registered, 673 concrete test classes, 87 never run in CI, and 0 of the 28 added in the last 60 days unregistered. The 1-of-28 quoted in the original PR body was an artifact of the missing OpenSearchUpgradeSuite. Refs #37577, #37581 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 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.
Summary
docs/testing/INTEGRATION_TESTS.md's "Registering Tests in a MainSuite" section — it said to register new integration tests by grouping with package/feature siblings. Team Falcon's lead confirmed that's not real: the actual rule is to register in whicheverMainSuite*currently has the shortest CI runtime, since the suites run in parallel and CI wants their wall-clock times kept roughly balanced. Documents the real check (sample ≥3 recent PR CI runs viagh pr view --json statusCheckRollup, not just one) and notes the suite set isn't fixed/exhaustive (a newMainSuite3betc. may appear later). Also fixes suite-list mechanics: new entries are appended at the end of@SuiteClasses, not alphabetized/grouped — verified against the realMainSuite2b.java.ContentletAPI.findByIdentifier(...).isPresent()— fabricated; real method isfindContentletByIdentifier(...), returns aContentletdirectly and throwsDotContentletStateExceptionwhen not found.createTestContentlet()called with no args in one example but defined requiring aContentTypeargument in another (self-contradiction).systemUser/contentletAPI/workflowAPI/defaultLanguageas inherited fields without ever declaring/initializing them — wouldn't compile as written.DataProviderRunner/DataProviderWeldRunnerusage counts and the*Test.java/*IT.javaratio had drifted from current real counts.This continues the Backend AI-Context Rock (epic #37124); this specific file was rewritten as an out-of-scope discovery during M3 (#37128) and merged in #37506. Filing against #37128 since that's the milestone this file is associated with.
Test plan
ContentletAPI,WorkflowAPI,CacheLocator,DotCacheAdministrator,ContentTypeBuilder,Host,FolderAPI,HibernateUtil,DbConnectionFactory,Logger) — all realMainSuite*CI-timing claim against 3 independent recent PR runs (fix(users): honor orderby/direction on GET /v1/users/filter (#37458) #37509, fix(publishing-queue): scope Pending tab delete to its bundle and stop the dojo parse abort (#36861) #37494, fix(browser): bind the MIME type filter value in the browser query #37483) before writing the ruleThis PR fixes: #37128
This PR fixes: #37128