fix(ci): drop dead build artifact, guard an NPE in a catch block, and make integration tests report their failures

[wezell]

Fixes #36942 (partially — the shard rebalance is deferred, see below).

A set of independent CI and test fixes, none of which depend on each other.

Build

Dead build-classes artifact removed. It was uploaded on every build and downloaded by nobody — the only restore-classes: true caller is deploy-javadoc, which no workflow invokes. Costs ~0.5m on the serial critical path that gates all ~25 test jobs.

A production bug

AWSS3PublishingEndPoint.validatePublishingEndPoint() catches S3 connection failures to report them to the user, then builds that notification with PortalUtil.getUser().getUserId(). But:

final HttpServletRequest req = HttpServletRequestThreadLocal.INSTANCE.getRequest();
return req == null ? null : getUser(req);

Null whenever no request is bound to the thread — scheduled jobs, background tasks, tests. So the error handler NPE'd, replacing the S3 error it was trying to report and defeating the catch entirely. Now null-guarded.

Tests that could not report their own failures

A scan of all 845 integration test files found 18 catch blocks that fail a test while discarding the exception:

catch (Exception e) { Assert.fail("Should work"); }

Surefire retries then produce N identical, contentless messages. This cost a full CI run to diagnose on PublishingEndPointTest — four retries, four copies of "No Exception should be thrown", no way to tell what threw. Two were worse: a bare Assert.fail() with no message at all.

All 18 now throw new AssertionError(msg, e) — same failure, cause chained. The scanner is included as .github/scripts/test-balance/find_swallowed.py; a re-scan reports zero remaining. For balance, 90 catch blocks already did this correctly — these were the outliers.

Test performance

	before	change
Task240306...Test	7.26m	teardown batched: find+destroy per inode over ~150 Language Variables × 3 tests, each its own transaction → one query, one transaction
ImportUtilTest	34s of Thread.sleep	→ awaitility; the 30s one slept before its first check, so it always paid full cost
BrowserAPITest	4.06m	145 file assets → 36; the assertions depend on page-size relationships, not absolute counts

FolderResourceTest got the same treatment, but only where safe — three of its five large loops are load-bearing (they pin the former 20-item cap, the @DefaultValue("40") limit, and that limit=-1 bypasses it). Shrinking those would make them pass trivially.

Also removed Task240306's deliberate delete of the Language Variable content type — see #36958, which tracks marking it system so that delete becomes impossible.

Why the shard rebalance is not here

It works. Measured on run 31203516115 with fail-fast disabled so every shard reported:

IT shards    22.2 - 26.2m   (was 38.9m)
Postman      18.6 - 28.8m   (was 37.8m)

But 3 of 7 IT shards and 5 of 9 Postman shards failed — all tests that only passed because of which suite-mates ran before them.

The Postman group names turned out not to be labels but dependency clusters: category-content grouped Category + ContentResourceV1 + Content_Resource because those collections share data. Rebalancing on time alone shattered them.

Preserved on issue-36942-shard-rebalance-wip (f612b27) so it can be resumed rather than rebuilt. It is worth resuming — it is an effective detector of tests that depend on their neighbours, and every failure it surfaced was a real defect, including the production NPE above and the deletable content type in #36958. But that is a test-independence programme, and it should not gate these fixes.

Verification

Both :dotcms-core and :dotcms-integration compile. Not run locally — needs the full IT stack.

[claude]

Claude finished @wezell's task in 3m 38s —— View job

---

Code Review

Reviewed the diff against origin/main. The changed set here is one production null-guard, a batch of integration-test hardening (swallowed-exception fixes, fixture-timing fixes, size parameterization), a new diagnostic script, and a Language.properties trim. Note: the changed files in this diff are not the CI shard-rebalancing described in the PR body — those .github/workflows / config.json / MainSuite changes are not present in origin/main...HEAD. This review covers what's actually in the diff.

New Issues

No issues found.

Notes (non-blocking)

• ✅ AWSS3PublishingEndPoint.java:122-126 — The core fix is correct. PortalUtil.getUser() returns null when no request is bound to the thread (verified: PortalUtil.java:398-400 returns null for req==null), so the previous PortalUtil.getUser().getUserId() threw an NPE out of the catch block, masking the real S3 error. Guarding on null and relying on the already-logged Logger.warn is the right call. Import matches the return type.

• The catch → throw new AssertionError(msg, e) conversions across ~10 test files (BrowserAPITest, ContentTypeTest, TailLogResourceTest, LocalTransactionAndCloseDBIfOpenedFactoryTest, etc.) are a clean, uniform improvement — they preserve the original message and the cause, which is exactly what assertTrue(msg, false) destroyed. No behavior regression: each still fails the test, now with a diagnosable stack trace.

• RuleBundlerTest.java:48-78 — Good catch on the root cause. Moving fixture creation out of the @DataProvider (evaluated at suite construction) and into createRule() (called inside the test) genuinely fixes the "rule is null" ordering flake. The toString() override for readable parameterized names is a nice touch.

• ImportUtilTest.java / Task240306...Test.java — Replacing fixed Thread.sleep(...) with Awaitility.await().atMost(...) and batching teardown destroys is sound. Two things worth a glance (not blocking):

  • awaitIndexed issues one searchIndex per inode per poll. For large inode lists this is N queries every 200ms — fine at current test sizes, but if reused with big sets it could get chatty.
  • Task240306 deletes testDropThenRecreateLanguageVariableContentType, and the comment honestly acknowledges the lost coverage (checkContentType recreating a genuinely-absent type) is deferred to Language Variable content type is not marked system and can be deleted, breaking i18n site-wide #36958. That's a reasonable trade given Language Variable content type is not marked system and can be deleted, breaking i18n site-wide #36958 makes the delete path impossible, but the coverage gap is real until that lands.

• find_swallowed.py:16 — import os, re, os, glob, collections imports os twice (harmless duplicate). Minor.

• Language.properties — Removed keys (publishing-queue.bundles, .bundles.count, .column.items.view, .select-bundle.remove.confirm.header, publishing-queue.delete, .asset-list.remove-confirm.header) — this diff is backend/test-only, so I can't verify from here whether the corresponding frontend dm(...) / translate references were all removed. Assumption: the paired frontend PR drops these usages. What to verify: no remaining core-web reference to the deleted keys, or they'll render as raw key strings in the UI.

Overall: clean, well-reasoned changes with unusually good explanatory comments. Nothing blocking.
· issue-36940-ci-speedup