test: faster + cleaner unit suite - fork per-core, ArchUnit integration-test guard, RunLengthEncoder speedup, DI - #69
Merged
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #69 +/- ##
============================================
- Coverage 78.05% 78.01% -0.04%
- Complexity 74 1073 +999
============================================
Files 81 81
Lines 4014 4039 +25
Branches 372 372
============================================
+ Hits 3133 3151 +18
- Misses 707 713 +6
- Partials 174 175 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
❌ Mutation Testing (PIT) ReportPIT did not produce a report. Most commonly this means a test failed in the baseline (PIT runs all tests unmodified first to establish green) and PIT aborted before mutating. See the "Run PIT mutation testing" step logs for the failing test, then either fix it or add it to |
astubbs
added a commit
that referenced
this pull request
Jul 28, 2026
…one (PR #69) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv
astubbs
added a commit
that referenced
this pull request
Jul 28, 2026
#68 squash-merged to master, orphaning the original unsquashed #68 commits still carried on this branch and making a rebase conflict on every one. Merged instead - resolving the 7 conflicts once. Conflict resolution (all in #68-owned files): - Pure-#68 files taken from master (its finalized, post-review content): self-hosted-tests.yml, SELF_HOSTED_RUNNER.md, the flakiness doc, bin/ci-integration-test.sh. - Mixed files combined: actionlint.yaml keeps performance+macOS labels; CHANGELOG.adoc keeps #69's Examples/ArchUnit entries with master's corrected self-hosted entry; inflight.md keeps 6:16 + #69's DONE unit-forking block, drops master's superseded "NEXT/not started" note. - Deleted laptop-sequential-poc.yml: the merge resurrected it (removed in #68; the squash made its deletion net-absent from the merge base). It stays gone. Net PR diff vs master is now only #69's own work (unit-suite forking, the TestConventionRules ArchUnit guard, StreamsApp/CoreApp DI, RunLengthEncoderTest speedup, real Awaitility/Hamcrest, pr-mac-fast-feedback.yml, docs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @astubbs's task in 4m 6s —— View job Claude's reviewReviewed the diff against The good
Worth a look
Not reviewed in depth
No blocking correctness or security issues found. LGTM pending a decision on the duplicate-code gate and the PIT run finishing green. |
astubbs
changed the title
astubbs
added a commit
that referenced
this pull request
Jul 28, 2026
…pd cap, regen README Follow-up to the @claude review on PR #69. - Dedupe the one clone this PR actually introduced: remove unused imports from MockConsumerTestWith{CommitTimeout,SaslAuthentication}Exception (SaslAuthenticationException and comment-only LongPollingMockConsumer) so the near-identical import blocks no longer register as a jscpd clone. Zero behaviour change (timing-sensitive test logic untouched). The other flagged clone (CoreAppTest<->VertxAppTest) is pre-existing on master - this PR doesn't touch those files - so it's left alone. - Govern the jackson-databind test dep: add jackson.version=2.17.2 + dependencyManagement entry in the root pom; drop the hardcoded version from parallel-consumer-example-metrics (was the only ungoverned direct Jackson pin). - Raise the jscpd absolute cap 4% -> 5% (matches PMD CPD). The repo baseline is already ~4.2%, so a 4% cap failed on every PR including the base branch; the real regression guard is the per-engine "max increase vs base" check. - Regenerate README.adoc from CHANGELOG (review finding #5): the Self-Hosted Tests bullet still described the old thread-parallel approach; it now matches the CHANGELOG's forked-per-broker wording. Verified: `mvn -Pci test-compile` green across all modules. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Addressed the review feedback in Duplicate Code gate (finding #3)
README/CHANGELOG drift (finding #5) - README.adoc regenerated from CHANGELOG; the Hardcoded Jackson (finding #4) - Stale PR description (finding #1) - rewritten; the "Still in flight (WIP)" section is gone (all four items landed), and the full scope is now listed. Scope creep (finding #2) - acknowledged in the description: the CI/perf changes (unit forking, mac fast-feedback, PIT retune, Kafka-4.x-compat disable) are grouped separately from the core test-hygiene work. @astubbs's inline question re Verified |
astubbs
added a commit
that referenced
this pull request
Jul 29, 2026
…fresh) Clean auto-merge (no conflicts). Brings in #73's non-major dependency + build-plugin bumps (junit 5.14.4, testcontainers 1.21.4, mockito 5.23.0, byte-buddy 1.17.7 pin, surefire/spotbugs/pitest plugin bumps). #69's unit-suite forking and the module-local jackson pin are preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Jul 29, 2026
…needs a plugin) #73 bumped pitest 1.17.4 -> 1.25.8. In 1.25.x the `-DwithHistory` shorthand requires the commercial arcmutate history plugin and hard-fails without it: "History has been enabled but no history plugin has been installed/activated" - which is why the PIT job started failing in ~48s (not the #69 excludedTestClasses change, which PIT accepted fine). Switch to the built-in file-based incremental history via explicit -DhistoryInputFile/-DhistoryOutputFile at a fixed /tmp path (no plugin needed), and cache that exact file. Keeps incremental analysis; drops the plugin dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Jul 29, 2026
…tructure) #74 removed the `== Unreleased` section and folds pending work into `== 0.6.0.0` (still -SNAPSHOT), and added a =Dependencies= entry for #73. Resolved the CHANGELOG conflict by following that structure: dropped Unreleased, folded #69's user-facing entries into 0.6.0.0 (StreamsApp DI under a new =Examples=; a concise forked-unit-suite + ArchUnit test-placement line under =Build & CI=), and dropped #69's now-duplicate Self-Hosted Tests entry (already in master's 0.6.0.0). Regenerated README.adoc from the merged CHANGELOG so the two stay consistent (also fixes the pre-existing README/CHANGELOG drift #74 left on master). #74 is docs-only; no code changed, so no test rerun needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…drop shaded Awaitility/Hamcrest
Container-based tests were landing in the surefire (unit) suite purely because they
weren't in an integrationTests package, making the "unit" run slow and Docker-dependent.
Enforce the boundary so it can't silently regress, and clean up a related classpath trap.
ArchUnit rule (TestConventionRules): a test that uses Testcontainers or extends
BrokerIntegrationTest MUST live in an integrationTests package (so failsafe runs it with
Docker, never surefire). Guards every module - core, vertx, reactor, mutiny, examples -
green in all of them, which *proves* no container test is left hiding in a unit suite.
Written red-first; it caught its own convention bugs. Two matcher subtleties worth keeping:
the package name is integrationTests (plural) so the rule matches plural+singular; and the
Testcontainers match is narrowed to org.testcontainers.containers../.junit.. so it doesn't
false-flag pure-mock tests that only touch the shaded Awaitility bundled in Testcontainers.
DRY: the rule logic lives ONCE in TestConventionRules (shipped in core's test-jar); each
module has a tiny TestConventionsArchTest that only points ArchUnit at its own packages via
ArchTests.in(...). This replaces the previously-duplicated per-module IntegrationTestPlacementArchTest
copies (the ~90% similarity the duplication bot flags across the shims is irreducible - a
module can only @AnalyzeClasses its own packages).
Unshade Awaitility/Hamcrest: some tests imported org.testcontainers.shaded.org.{awaitility,hamcrest}
while the rest used the real libraries. Two copies of a library on the classpath bit us - the
shaded Awaitility has its own default-timeout config, and a shaded Hamcrest Matcher passed into
Awaitility's untilAtomic(..) only compiled because both sides were shaded. Migrated all 11 shaded
usages (3 MockConsumerTest* unit + 8 integration) onto the real org.awaitility/org.hamcrest (already
on the classpath, identical API/defaults), and the ArchUnit rule now bans org.testcontainers.shaded..
so it can't come back. Also drops now-unused imports from the two MockConsumerTest* classes.
…and relocate their integration tests The two example "tests" that the new ArchUnit rule flagged were real integration tests (StreamsAppTest; CoreAppMetricsIntegrationTest with a real Prometheus Testcontainer) hiding in surefire only because they weren't in an integrationTests package. Fix the underlying DI smell rather than just the packaging: - StreamsApp / CoreApp now take their Kafka bootstrapServers (and, for metrics, the consumer) via the constructor - constructor injection, NOT widened visibility - replacing the subclass-and-override-package-private-hooks seam that stopped working once the test moved to another package. Nice side effect: the examples are now genuinely runnable. As shown they were previously non-functional - empty-Properties clients (new KafkaConsumer<>(new Properties())) and a literal "add your server here". run()/close()/topic constants are now public. - The residual CoreAppUnderTest subclass now overrides only the postSetup() lifecycle hook that simulates the mock consumer's rebalance/assignment - legitimate mock scaffolding, not production wiring. - StreamsAppTest and CoreAppMetricsIntegrationTest (+ its PrometheusContainer helper) move into their module's integrationTests package, so failsafe runs them with Docker (StreamsAppTest verified green, ~36s on a real broker); the assertions are unchanged. - example-metrics needs real jackson-databind (test scope) to parse the Prometheus metadata JSON - it had been using Testcontainers' shaded Jackson, now banned. Pinned MODULE-LOCALLY on purpose: jackson-databind is a transitive dependency of wiremock-jre8, so pinning it globally (root dependencyManagement) forced WireMock onto an incompatible Jackson and it returned HTTP 500, failing the vertx tests. Kept isolated here; the trap is documented in docs/inflight.md.
…test classes
The unit (surefire) suite ran fully sequential (~5:14 for core) and was the CI long pole
(~8.5 min). Make it fast with process-level parallelism plus two targeted test speedups.
Fork the unit suite (pom.xml): surefire forkCount is a property defaulting to 1 (unchanged
locally / opt-in), raised to 1C (one fork per core) in the ci profile, keeping
parallel-tests=false. Core unit: 5:14 -> 1:39 (259 tests, 0 failures on a 12-core box).
Forking is both faster than JUnit thread-parallel (2:32) AND reliable - separate JVM
processes don't share the static state that concurrent threads race on, so it doesn't
reintroduce thread-parallel flakiness. reuseForks=true amortises JVM startup.
- forkCount=1C is deliberately the shared value because it auto-scales: 2 forks on GitHub's
2-core gate, core-count on the self-hosted box. Measured: 2C (24 forks) beats 1C on the
12-core Mac (1:26 vs 1:38) but oversubscribes the 2-core gate and REGRESSES it (7:08 vs
5:49), and 3C over-forks even on the Mac. So 2C would only ever be a many-core job override,
never the shared profile.
- Follow-up (docs/inflight.md): jacoco writes ONE append-mode exec file, which can race under
forkCount>1 - give each fork its own destFile + jacoco:merge if coverage looks off.
RunLengthEncoderTest (~85s -> 0.087s for the v2/Integer case): vTwoIntegerOverflow drove the
run-length past Integer.MAX_VALUE by calling encodeCompletedOffset ~2.1 BILLION times. But the
encoder accumulates run-length from the DELTA between an offset and the previous one, not from
the call count - so one completed offset a huge distance past the previous overflows in a single
step, through the exact same Math.toIntExact path. Replaced the loop with a two-call delta jump;
the overflow assertion is unchanged. (Also removed a dead shadowed `versionsToTest = v2` local +
unused Range import, and added javadoc explaining the shapes.) The sibling
testSimultaneousWithOverflowErrors INT case stays ~59s because OffsetSimultaneousEncoder.invoke()
genuinely walks every offset and can't take the delta shortcut - a delta-aware invoke() is a
main-code optimisation, tracked in docs/inflight.md.
Mutiny - trim the two heaviest unit classes' input volume 50%: MutinyPCTest concurrency
100_000 -> 50_000, MutinyBatchTest averageBatchSize 10_000 -> 5_000. Assertions unchanged and
still bite (max-concurrency=999 enforced; average-batch-size > 19.98 held - batches still fill
to the target 20 regardless of total volume). Verified locally: mutiny module 13 tests, 0 fails.
…t-feedback + config cleanups Optional Mac fast-feedback (pr-mac-fast-feedback.yml): runs the forked unit/integration suites (and PIT) on the self-hosted mac-laptop for fast feedback on a multi-core box (forked unit ~1:39 vs GitHub's 2-core ~5:49). Non-gating (continue-on-error, not required) so the laptop being offline never blocks a merge; the required gate stays on GitHub-hosted infra. SECURITY: guarded to SAME-REPO pull requests (head.repo == this repo) so a fork PR's untrusted code never runs on the home machine (RCE); uses pull_request (no secrets), never pull_request_target. The suites are a matrix so they can run concurrently given 2+ runner instances. Declares the macOS runner label for actionlint. Mutation testing (PIT) moved off GitHub's 2-core runner onto the self-hosted Mac: - PIT is CPU-bound and process-parallel across minion JVMs, so it scales with cores. On the 2-core runner a full internal.* sweep was impractically slow (threads maxed the cores; 17+ min without finishing). New bin/ci-mutation-test.sh runs it with -Dthreads defaulting to the box's core count (override PIT_THREADS; RAM = threads x 2g), so ~12 threads on the Mac is ~5-6x faster. Removed the GitHub-hosted mutation-testing job entirely. - Scope: targetClasses stays internal.* (the engine); -DexcludedTestClasses=integrationTests.* stops per-mutant runs re-running the slow Docker integration tests (which had blown the 300-min cap). Note this restricts which TESTS run per mutant, NOT which classes are mutated - it's still a full sweep. Coverage-minion heap is -Xmx2g (1g OOM'd, 4g completes on the heavy PC classes). - No incremental history: #73 bumped pitest 1.17.4 -> 1.25.8, which dropped built-in file-based history entirely - -DwithHistory AND explicit -DhistoryInputFile now both hard-fail without the commercial arcmutate history plugin. Removed all history flags. The changed-classes-only speedup (arcmutate git plugin, free for OSS) is tracked as the escalation path in docs/inflight.md. Config: - jscpd duplicate-code absolute cap 4% -> 5% (matches PMD CPD). The repo baseline is already ~4.2%, so a 4% cap failed on every PR including the base branch; the real regression guard is the per-engine "max increase vs base" check. - Disable the experimental "Kafka Compat (experimental 4.x)" job (if: false) - it's continue-on-error so never gated, but showed a red X on every PR; re-enable when the Kafka 4.x migration begins.
…t-suite work - docs/solutions/test-flakiness/unit-tests-parallelise-by-forking-not-threading: the full fork-vs-thread rationale and measurements behind the unit-suite change. - docs/inflight.md: records the live follow-ups and shelved plans surfaced by this work - the RunLengthEncoder delta-aware invoke() main-code optimisation; the jacoco append-mode exec race under forkCount>1; the PIT changed-classes-only escalation via arcmutate's free OSS licence (why it's shelved: manual signup + a licence file at repo root + a commercial dep, vs. the current full-sweep staying under the cap); the MultiInstanceMetricsTest lock-timeout contention flake (hypothesis + "confirm before touching" per AGENTS.md); and the archunit-junit5 <-> JUnit 6 coupling (no archunit-junit6 yet, TNG#1556) to account for before the junit 6.0.3 bump. - CHANGELOG.adoc: user/operator-visible entries under 0.6.0.0 (StreamsApp example DI; the ArchUnit integration-test guard + forked unit suite). README.adoc regenerated to match - it is GENERATED from CHANGELOG.adoc via the asciidoc-template plugin (README_TEMPLATE.adoc + include::CHANGELOG.adoc), so it must never be hand-edited; regenerating keeps it in sync (and fixes pre-existing drift).
astubbs
force-pushed
the
test/parallel-unit-tests
branch
from
July 29, 2026 04:23
8a94d21 to
6d6db5d
Compare
4 tasks
astubbs
added a commit
that referenced
this pull request
Aug 4, 2026
…dy said otherwise The automated reviewer flagged this contradiction on five consecutive passes and it kept being deferred. It was right, and it was not a nitpick: the plan doc asserted "the basic history file is free in OSS pitest, confirm whether the free tier covers what we want", while docs/inflight.md records the opposite as an already-verified PR #69 finding - 1.25.x dropped file-based history when #73 bumped 1.17.4 -> 1.25.8. Settled by running it rather than by choosing which document to believe: [ERROR] History has been enabled but no history plugin has been installed/activated. [ERROR] If you are using https://www.arcmutate.com remember to activate the history plugin So there is no free tier to check. History lives entirely in arcmutate, and the work item is obtaining and wiring a licence - free for OSS, but needing maintainer signup and a licence file at the repo root, which on a public repo means a committed key or a CI secret. The existing shelved plan in inflight.md covers that; §4.2 now points at it rather than duplicating it. The correction is written in place, with the reproduction, rather than the section being quietly rewritten - the same treatment as the other two corrections in this doc. inflight.md gains the re-verification date so the next reader finds agreement instead of a contradiction to arbitrate. 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.
Keeps the fast unit (surefire) suite free of slow, Docker-dependent integration tests, enforces it so it can't silently regress, and parallelises the unit suite by forking.
What's here
Test hygiene (the core change)
TestConventionRules+ a thin per-moduleTestConventionsArchTestshim): any test that uses Testcontainers or extendsBrokerIntegrationTestmust live in anintegrationTest/integrationTestspackage (where failsafe runs it, with Docker) — never in a surefire-scanned package. Rule logic lives once in core's test-jar; each module only@AnalyzeClassesits own packages. Green across core, vertx, reactor, mutiny, and the example modules. Written red-first (it caught its own convention bugs). Also bans the shadedorg.testcontainers.shaded.*Awaitility/Hamcrest copies so they can't creep back.StreamsAppDI fix: the example now takesbootstrapServersvia its constructor instead of subclass-and-override test hooks, soStreamsAppTestmoves out to anintegrationTestspackage and the example is genuinely runnable.run()/close()/topic constants are nowpublic.example-metricsCoreAppDI: same constructor-injection treatment;CoreAppMetricsIntegrationTestrelocated tointegrationTests(with a realjackson-databindtest dep, version governed in the root pom).RunLengthEncoderTestspeedup: replace the ~2.1B-iteration overflow input with a single delta jump (the encoder accumulates run-length from the delta between offsets, not the call count) — v2 case ~85s → 0.087s, overflow assertion untouched.CI / perf riding along (documented in
docs/inflight.md+ CHANGELOG)These are separate in spirit from the test-hygiene work but landed here; noting them for reviewability.
surefire forkCount=1C,reuseForks=true) — one JVM per core, keepingparallel-tests=false. Core unit ~5:14 → ~1:39; auto-scales with cores.pr-mac-fast-feedback.yml) — same-repo-guarded (pull_request, notpull_request_target),continue-on-error, non-required.-Xmx2g,threads=2) and disable the known-broken experimental Kafka-4.x-compat job (if: false) until the Kafka 4 migration.Known follow-up (not blocking)
archunit-junit5(ArchUnit's JUnit 5 / Platform 1.x integration). It will not run on JUnit 6 (Platform 2 / Jupiter 6), and there is noarchunit-junit6artifact yet (latest ArchUnit 1.4.2 still ships only junit5; tracked in TNG/ArchUnit#1556). So whendependabot/maven/junit.version-6.0.3lands, either ArchUnit must have shipped JUnit 6 support, or these convention tests stay on the JUnit 5 engine (JUnit runs 5 + 6 side by side), or they're temporarily disabled. Flagging so the JUnit 6 bump doesn't merge without accounting for it.🤖 Generated with Claude Code
https://claude.ai/code/session_012ytagqk6daL4uTtbebQNNv