From 3ffd1c14062e020bd4f75f454cc8896f7b001b77 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Wed, 16 Sep 2026 11:20:11 -0600 Subject: [PATCH 1/2] docs(testing): fix MainSuite registration rule and audit code examples 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 --- docs/testing/INTEGRATION_TESTS.md | 102 +++++++++++++++++++++++------- 1 file changed, 78 insertions(+), 24 deletions(-) diff --git a/docs/testing/INTEGRATION_TESTS.md b/docs/testing/INTEGRATION_TESTS.md index ef3f782eb3a..fa6f0978d33 100644 --- a/docs/testing/INTEGRATION_TESTS.md +++ b/docs/testing/INTEGRATION_TESTS.md @@ -32,8 +32,8 @@ dotcms-integration/ For JUnit4 tests that need `@DataProvider`-style parameterization (from the `com.tngtech.junit.dataprovider` library — a real dependency declared in `dotcms-integration/pom.xml`), annotate the class with one of these two `@RunWith` runners: -- **`DataProviderRunner`** (`com.tngtech.java.junit.dataprovider.DataProviderRunner`): plain data-provider-driven parameterized tests, no CDI container — **68 real usages** in this module (e.g. `LanguageUtilTest.java`) -- **`DataProviderWeldRunner`** (`com.dotcms.DataProviderWeldRunner`): same, but extends `DataProviderRunner` to additionally spin up a real Weld CDI container so test classes can be resolved as CDI beans — **40 real usages**. This is a CDI-aware superset, not a replacement for the plain runner; both stay in active use for different needs. +- **`DataProviderRunner`** (`com.tngtech.java.junit.dataprovider.DataProviderRunner`): plain data-provider-driven parameterized tests, no CDI container — **~74 real usages** in this module (e.g. `LanguageUtilTest.java`) +- **`DataProviderWeldRunner`** (`com.dotcms.DataProviderWeldRunner`): same, but extends `DataProviderRunner` to additionally spin up a real Weld CDI container so test classes can be resolved as CDI beans — **~41 real usages**. This is a CDI-aware superset, not a replacement for the plain runner; both stay in active use for different needs. ### Naming: use the `Test` suffix, not `IT` ⚠️ @@ -45,32 +45,59 @@ plus `**/QuickSuite.java` and `**/OpenSearchUpgradeSuite.java` in their profiles is driven entirely by suite registration and never by the class-name suffix. Surefire is skipped in this module, so a `*Test` name here can't be mistaken for a unit test either. -Match the siblings already in your package — the module is ~654 `*Test.java` to ~12 `*IT.java`. +Match the siblings already in your package — the module is ~680 `*Test.java` to ~12 `*IT.java`. ### Registering Tests in a MainSuite (CI gate) ⚠️ -CI runs integration tests **only** through the JUnit `@SuiteClasses` aggregator suites -(`MainSuite1a`, `MainSuite1b`, `MainSuite2a`, `MainSuite2b`, `MainSuite3a` in -`dotcms-integration/src/test/java/com/dotcms/`). **A new test class that is not listed in one -of these suites compiles fine but is silently never executed in CI** — green build, zero -coverage. This is easy to miss because the class runs locally via `-Dit.test=MyTestClass`. +CI runs integration tests **only** through the JUnit `@SuiteClasses` aggregator suites in +`dotcms-integration/src/test/java/com/dotcms/` — currently `MainSuite1a`, `MainSuite1b`, +`MainSuite2a`, `MainSuite2b`, `MainSuite3a` (plus `Junit5Suite1` and `OpenSearchUpgradeSuite`, +see below). **This set isn't fixed** — a new `MainSuite3b` (or beyond) may get added later as the +module grows; check `dotcms-integration/src/test/java/com/dotcms/MainSuite*.java` for the +current list rather than assuming this doc's names are exhaustive or permanent. **A new test +class that is not listed in one of these suites compiles fine but is silently never executed in +CI** — green build, zero coverage. This is easy to miss because the class runs locally via +`-Dit.test=MyTestClass`. When you add a new integration test class, register it: -1. Pick a suite — group it with sibling tests in the same package. The v1 asset tests live in - `MainSuite2b`, so a new v2 asset test goes there too. -2. Add **both** the `import` (in alphabetical order) and the `Foo.class,` entry inside - `@SuiteClasses({ ... })`. +1. **Pick the suite with the shortest current CI runtime** — not by package/feature grouping. + The `MainSuite*` jobs (currently five — see the note above on this list growing over time) + run in parallel in CI, so the goal is to keep their wall-clock times roughly balanced; adding + to whichever suite already finishes fastest keeps the overall PR build time down. **Don't + hardcode a specific suite name as "the fast one" in this doc or in memory** — suite runtimes + shift over time as tests are added/removed elsewhere, so check current timings before each new + registration. The real way to check: pull job start/completion times from **at least 3 recent, + independent PR CI runs** (not just one — a single run can be skewed by runner contention or + noise, and durations should agree before you trust them) — + ```bash + gh pr view --repo dotCMS/core --json statusCheckRollup \ + -q '.statusCheckRollup[] | select(.name? and (.name | test("Integration Tests"))) | "\(.name) \(.startedAt) \(.completedAt)"' + ``` + repeated across 3 different recent PR numbers, and compute the duration of each `MainSuite*` + job in each (ignore `Junit5Suite1` and `OpenSearchUpgradeSuite` — those are structurally + different, purpose-built suites, not general destinations for a new JUnit4 test). Add the new + test to whichever `MainSuite*` is consistently fastest across the samples. If every current + `MainSuite*` is consistently near-saturated and none is meaningfully faster, that's a signal a + new `MainSuite` class may be needed — that's a call for whoever owns the CI/test infra, not + something to decide unilaterally from a PR sample. +2. Add the `import` (alphabetized, matching the file's existing import block) **and** append the + `Foo.class,` entry to the **end** of the `@SuiteClasses({ ... })` list — the real + `@SuiteClasses` lists are not alphabetized or grouped by feature/package at all; new tests are + consistently appended at the end. ```java +// Say the check in step 1 found MainSuite2b currently has the shortest CI runtime — +// register the new class there: import alphabetized with the rest, class entry appended +// at the end of the @SuiteClasses list: // MainSuite2b.java -import com.dotcms.rest.api.v2.asset.WebAssetResourceV2IntegrationTest; // sorted with siblings +import com.dotcms.rest.api.v2.asset.WebAssetResourceV2IntegrationTest; // alphabetized import ... @SuiteClasses({ ... - WebAssetHelperIntegrationTest.class, - WebAssetResourceV2IntegrationTest.class, // <-- register here or it won't run in CI - ... + RoleResourceUsersIntegrationTest.class, + com.dotmarketing.common.reindex.ReindexDeleteJournalTest.class, + WebAssetResourceV2IntegrationTest.class, // <-- appended at the end }) ``` @@ -127,7 +154,7 @@ public class MyResourceIntegrationTest extends IntegrationTestBase { @Before public void setup() { - contentTypeAPI = APILocator.getContentTypeAPI(systemUser); + contentTypeAPI = APILocator.getContentTypeAPI(APILocator.systemUser()); contentletAPI = APILocator.getContentletAPI(); } @@ -181,6 +208,17 @@ public class MyResourceIntegrationTest extends IntegrationTestBase { ```java public class ContentletAPITest extends IntegrationTestBase { + private ContentletAPI contentletAPI; + private User systemUser; + private Language defaultLanguage; + + @Before + public void setup() { + contentletAPI = APILocator.getContentletAPI(); + systemUser = APILocator.systemUser(); + defaultLanguage = APILocator.getLanguageAPI().getDefaultLanguage(); + } + @Test public void testContentletLifecycle() throws Exception { // Create content type @@ -190,7 +228,7 @@ public class ContentletAPITest extends IntegrationTestBase { Contentlet contentlet = new Contentlet(); contentlet.setContentTypeId(contentType.id()); contentlet.setStringProperty("title", "Test Title"); - contentlet.setHost(defaultHost); + contentlet.setHost(Host.SYSTEM_HOST); contentlet.setLanguageId(defaultLanguage.getId()); // Save @@ -205,9 +243,15 @@ public class ContentletAPITest extends IntegrationTestBase { // Delete contentletAPI.delete(contentlet, systemUser, false); - // Verify deletion - assertFalse(contentletAPI.findByIdentifier(contentlet.getIdentifier(), - defaultLanguage.getId(), false, systemUser, false).isPresent()); + // Verify deletion — findContentletByIdentifier throws DotContentletStateException + // when nothing is found; it returns a Contentlet directly, never an Optional/null + try { + contentletAPI.findContentletByIdentifier(contentlet.getIdentifier(), + false, defaultLanguage.getId(), systemUser, false); + fail("Contentlet should have been deleted"); + } catch (DotContentletStateException expected) { + // expected — content no longer exists + } } } ``` @@ -216,10 +260,20 @@ public class ContentletAPITest extends IntegrationTestBase { ```java public class WorkflowAPITest extends IntegrationTestBase { + private WorkflowAPI workflowAPI; + private User systemUser; + + @Before + public void setup() { + workflowAPI = APILocator.getWorkflowAPI(); + systemUser = APILocator.systemUser(); + } + @Test public void testWorkflowFiresOnCheckin() throws Exception { // Given - Contentlet contentlet = createTestContentlet(); + ContentType contentType = createTestContentType(); + Contentlet contentlet = createTestContentlet(contentType); // When — fires the contentlet's default workflow scheme as part of checkin WorkflowProcessor processor = workflowAPI.fireWorkflowPreCheckin(contentlet, systemUser); @@ -480,7 +534,7 @@ public class TestDataManager { .name(name) .build(); - contentType = APILocator.getContentTypeAPI(systemUser).save(contentType); + contentType = APILocator.getContentTypeAPI(APILocator.systemUser()).save(contentType); createdContentTypes.add(contentType.id()); return contentType; } @@ -538,7 +592,7 @@ public void testCreateUser() throws Exception { // Test operations // Cleanup - APILocator.getUserAPI().delete(user, systemUser, false); + APILocator.getUserAPI().delete(user, APILocator.systemUser(), false); } ``` From 8455dda88316c59027e0db32ca4594be423fc882 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Wed, 16 Sep 2026 15:17:16 -0600 Subject: [PATCH 2/2] docs(testing): address review feedback on registration/audit fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - *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 --- docs/testing/INTEGRATION_TESTS.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/testing/INTEGRATION_TESTS.md b/docs/testing/INTEGRATION_TESTS.md index fa6f0978d33..6a8f74d7b14 100644 --- a/docs/testing/INTEGRATION_TESTS.md +++ b/docs/testing/INTEGRATION_TESTS.md @@ -32,7 +32,7 @@ dotcms-integration/ For JUnit4 tests that need `@DataProvider`-style parameterization (from the `com.tngtech.junit.dataprovider` library — a real dependency declared in `dotcms-integration/pom.xml`), annotate the class with one of these two `@RunWith` runners: -- **`DataProviderRunner`** (`com.tngtech.java.junit.dataprovider.DataProviderRunner`): plain data-provider-driven parameterized tests, no CDI container — **~74 real usages** in this module (e.g. `LanguageUtilTest.java`) +- **`DataProviderRunner`** (`com.tngtech.java.junit.dataprovider.DataProviderRunner`): plain data-provider-driven parameterized tests, no CDI container — **68 real usages** (`@RunWith(DataProviderRunner.class)`) in this module (e.g. `LanguageUtilTest.java`) - **`DataProviderWeldRunner`** (`com.dotcms.DataProviderWeldRunner`): same, but extends `DataProviderRunner` to additionally spin up a real Weld CDI container so test classes can be resolved as CDI beans — **~41 real usages**. This is a CDI-aware superset, not a replacement for the plain runner; both stay in active use for different needs. ### Naming: use the `Test` suffix, not `IT` ⚠️ @@ -45,7 +45,7 @@ plus `**/QuickSuite.java` and `**/OpenSearchUpgradeSuite.java` in their profiles is driven entirely by suite registration and never by the class-name suffix. Surefire is skipped in this module, so a `*Test` name here can't be mistaken for a unit test either. -Match the siblings already in your package — the module is ~680 `*Test.java` to ~12 `*IT.java`. +Match the siblings already in your package — the module is ~680 `*Test.java` to ~19 `*IT.java`. ### Registering Tests in a MainSuite (CI gate) ⚠️ @@ -89,15 +89,16 @@ When you add a new integration test class, register it: ```java // Say the check in step 1 found MainSuite2b currently has the shortest CI runtime — // register the new class there: import alphabetized with the rest, class entry appended -// at the end of the @SuiteClasses list: +// at the end of the @SuiteClasses list. (MyNewFeatureIntegrationTest below is a placeholder — +// don't copy an existing suite member as your "new" entry, or you'll register it twice.) // MainSuite2b.java -import com.dotcms.rest.api.v2.asset.WebAssetResourceV2IntegrationTest; // alphabetized import +import com.dotcms.myfeature.MyNewFeatureIntegrationTest; // alphabetized import ... @SuiteClasses({ ... RoleResourceUsersIntegrationTest.class, com.dotmarketing.common.reindex.ReindexDeleteJournalTest.class, - WebAssetResourceV2IntegrationTest.class, // <-- appended at the end + MyNewFeatureIntegrationTest.class, // <-- appended at the end }) ```