test(core) #40: one harness for the vanilla MockConsumer tests, not five copies - #206
test(core) #40: one harness for the vanilla MockConsumer tests, not five copies#206astubbs wants to merge 9 commits into
Conversation
…ive copies The six MockConsumer* test classes each carried their own copy of the same wiring: build a MockConsumer, hand-rebalance the partition in, tell PC about it separately, update beginning offsets, construct and subscribe PC, feed records, collect them out of the user function, then tear down. The file-similarity check flagged two of them at 70.7% on #34 and kept flagging them afterwards, which is the symptom; the cause is that the wiring was copied rather than shared, so it drifted - the daemon-thread comment about PIT attributing a stray addRecord() to the next test in the JVM appears verbatim in two files, and its cleanup only in those two. MockConsumerTestBase now owns that wiring. A scenario supplies the failure behaviour (createMockConsumer) and the options it needs (customiseOptions), and nothing else. It deliberately does NOT extend AbstractParallelEoSStreamProcessorTestBase: that base wires a Mockito-spied LongPollingMockConsumer, and the subject of these tests is what PC does when the consumer misbehaves in ways only a hand-written MockConsumer subclass can express. CommitRejectionTestBase, which had already extracted its own half of the same wiring, now sits on the harness too, so a third rejection reason is one method rather than another copy. Deliberately left duplicated: each scenario keeps its own Awaitility block, with its own timeout, in its own file. They are the point of the test, and the timeouts are scenario-specific - each has to clear that scenario's simulated outage window. Hoisting them would have made the base the only place the assertions live, and a reader would have to open two files to learn what a test checks. Teardown is now uniform and strictly stronger than what it replaced: the record feed is interrupted AND joined (previously interrupted only, and only in two of the classes), before PC is closed rather than after, and Awaitility.reset() runs first so it cannot be skipped by a throwing close. Two real defects found on the way: - @timeout(60000L) on three of these classes meant 60000 SECONDS - JUnit's default unit - i.e. no timeout at all. Replaced by @timeout(120) on the base (@timeout is @inherited), which is real headroom over the longest scenario (25s measured) and a genuine guard against a wedged MockConsumer test. - MockConsumerRebalanceInProgressTest's javadoc still linked MockConsumerTestWith{CommitTimeout,SaslAuthentication}Exception, renamed away in dc44e20. javac does not check {@link} without doclint, so it rotted silently. MockConsumerEarlyCloseTest gains an assertion it never had: it asserted only that close() returned. It now also asserts PC ended closed with no failure cause - "shut down cleanly" as the javadoc has always claimed, rather than "died in a way that also reports closed". Secondary audit (the second half of #40) done but not acted on: the remaining high-similarity pairs in the test tree are overwhelmingly cross-module clones (Mutiny/Reactor ~94%), which need a generified base in core's test-jar - a different and much larger job. Ranked with verdicts in docs/refactoring.md so it is not re-derived. The one within-module pair above the check's fail threshold (TransactionAndCommitModeTest/VeryLargeMessageVolumeTest, ~88%) is broker ITs and wants Docker to verify, not a desk refactor. Verification: the six tests pass, three consecutive runs, no intermittency; full unit suite (bin/ci-unit-test.sh, all modules) green. MockConsumerTestBase is an extraction from upstream-derived files, so it carries the Confluent header plus the modifications line and is registered in EXTRACTED_FROM_UPSTREAM. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 8m 52s —— View job Claude is reviewing this PR
|
✅ 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)
|
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
|
@claude review this Context for the reviewer: the scheduled Particular things worth your attention:
|
|
Claude finished @astubbs's task in 6m 27s —— View job Claude finished the reviewTodo list
SummaryThis is a clean, well-executed dedup. I read every changed file plus the full new 1. Teardown ordering (
2. 3. Other observations (non-blocking)
I did not find anything to flag as a real bug in the three areas you asked about, or elsewhere in the diff. |
…ng readers it blocks The in-flight note and the upstream-map entry both said fork #40 was queued behind this PR and should be unblocked "the moment #202 merges". That stopped being true: this branch edits none of the four MockConsumer* tests, so the collision never materialised and #40 went ahead on its own as PR #206, open now. Left as written, both files were a stale marker telling the next session to wait for something that already happened - exactly the failure mode docs/inflight/AGENTS.md warns about. Also trims the in-flight note down to what `gh pr view` and the diff cannot answer. The review flagged the package/SpotBugs reasoning as intentional triplication across javadoc, docs/refactoring.md and this file; the javadoc and refactoring.md are the right homes for it, so this file now states the decisions and points at them instead of restating them in full. While verifying the SpotBugs entry, noted that poll() and wakeup() are already synchronized methods that each open a further synchronized (this) block on the monitor they hold. Reentrant so harmless, but it is what makes the guard/notify pairing hard to read, and likely why the AtomicBoolean looked necessary - recorded with the deferred fix so it gets cleaned up in the same pass. Refs #159 · upstream confluentinc#526 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
… ones The PR predicted CommitRejectionTestBase <-> MockConsumerTestBase would land around 70% on the file-similarity check, reasoning that a whole-file token measure mostly sees a shared package declaration and import block. The first authoritative CI run falsifies that: neither file appears in the report at all, against anything, and the reporting floor is 30%. What the run does show is the result #40 wanted - the MockConsumer*Test scenarios pair at 34-37%, down from the 70.7% that #34 flagged. The prediction came from a stdlib stand-in written because the real tool would not build locally (gensim fails on py3.14; on py3.11 an nltk import shim blocks imports). Record the measured values, and the lesson: read the tool, do not model it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
Review noticed that CommitRejectionTestBase overrode closeParallelConsumer() to call closeDontDrainFirst(), which is exactly what the default did: DrainingCloseable.close() delegates to closeDontDrainFirst(). So the hook had no override that changed anything, and its javadoc - "override for a scenario where draining first is wrong" - asserted the opposite of the truth, implying the default drains. A future author trusting it would reason backwards about teardown. Hook and override both removed. The non-draining contract, and why teardown wants it (it runs on the failure path and must not hang waiting on in-flight work against a deliberately misbehaving consumer), is now stated once at the only place that closes. Six tests still green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Thanks - that's the first substantive review this PR has had, and it found something real. Point-by-point: The one finding I acted on: the redundant close hookYou flagged Verified your claim first - The problem is what the hook's javadoc said:
That asserts the default drains. It does not. A future author adding a seventh scenario and trusting that sentence would reason backwards about teardown - and the one override present appeared to confirm it, since "overrides to not drain" only makes sense if the default drains. The pair was self-reinforcingly wrong. Since the hook had no override that changed behaviour, it was also speculative generality. Both are gone; the non-draining contract and why teardown wants it (it runs on the failure path too, so it must not hang waiting on in-flight work against a consumer still being made to misbehave) is now stated once, at the only place that closes. A scenario that ever wants the opposite should call Your three answers - agreed, and independently confirmed
On the test evidence you couldn't gatherYou flagged that you couldn't run ( The similarity prediction in the description was wrong, and is now correctedThe description told you to expect What the run actually shows is the result #40 wanted: the Description and both docs corrected to the measured values in 762087f, including the lesson (read the tool, don't model it). Still standing, not changedNot extending |
The red
|
| Input | Citation position | Result |
|---|---|---|
| 37 bytes | present | Review posted by run 30979310097. exit 0 |
| 130,807 bytes (real comments) | comment #2 of 5 | exit 1 - false failure |
| ~100KB synthetic | last line | exit 0 |
Same needle, same script - only the position and the size change. It fails when the citation is found early in a stream larger than the 64KB pipe buffer, and passes when grep is forced to read to the end.
Why it triggered here
The similarity report comment is 127,686 bytes of the 130,807 total. The claude[bot] comment carrying the citation is #2; the giant report is #4. So grep matches ~1KB in and exits with ~128KB still unwritten - guaranteed EPIPE. Any PR with a large similarity report hits this.
Two separate things, both worth knowing
- The gate is wrong here. Run 30979310097 did post a comment citing itself, so a working gate would have gone green.
- But the review was substantively empty anyway. That run finished
"is_error": false, 52 turns, $2.27, 14 permission denials - and never wrote its findings into its comment, leaving two todo items unchecked and posting zero inline comments. That's exactly the "posts its progress comment and then produces an empty review would still pass" limit the script's own header calls out. Worth knowing that the failure mode is real and not hypothetical.
I got a real review by asking for one in a comment (#issuecomment-5188876914), as the script's error message suggests - it ran clean in 6m27s and found one genuine issue, now fixed.
Note on the fix, for whoever picks it up
bin/test-check-copyright-headers.sh already asserts ok: scanner has no SIGPIPE-prone pipes into grep -q / awk - the repo has met this bug class before and guards check-copyright-headers.sh against it. check-review-posted.sh shipped in the same commit without that guard, and has no self-test. The obvious candidates are dropping -q (grep -cE ... > /dev/null), a here-string (grep -qE ... <<< "$comment_bodies"), or a case/[[ ]] match with no pipe at all - plus a large-input regression test, since a small-input test passes today.
Update: the gate went green on the next push, and that confirms the diagnosis rather than clearing it
claude-review passed on run 30986389457. Nothing was fixed - the bug is still there, and the reason it passed is the smoking gun.
The gate greps for the run id in the comment stream, which GitHub returns in creation order. Compare where the citing comment sat:
| Run | Position of the comment citing that run | 127,696-byte similarity report | Result |
|---|---|---|---|
| 30979310097 | 2nd of 5 | after the match | grep exits early -> printf EPIPE -> false fail |
| 30986389457 | 10th of 11 | before the match | grep must read to the end -> no EPIPE -> pass |
Same script, same needle, same repo - only the match's position relative to the 64KB pipe buffer changed. A gate whose verdict depends on whether the thing it is looking for happens to appear before or after an unrelated bot's large comment is not measuring what it thinks it is.
So this is latent, not resolved. It will fail again on any PR where the reviewer's comment lands before a large one - which is the normal case early in a PR's life, before the reviewer has re-run. Worth fixing on master with a large-input regression test, since a small-input test passes today.
|
Claude finished @astubbs's task in 4m 24s —— View job Claude finished the reviewTodo list
SummaryThis update (commits 762087f, 2d1b2a4) responds directly to the prior review round, and everything in it checks out against the actual code and a live test run. 1. The close-hook removal (2d1b2a4) is correct and exactly as described. 2. The docs correction (762087f) matches the CI report, not just the PR body. 3. Live-ran all six affected tests ( All green, and the per-test timings land within ~0.1s of the numbers quoted in this thread — good evidence the 120s 4. 5. No compiler warnings on either On the three questions from the original review requestAlready answered in the prior round (teardown ordering safe for all six scenarios including the synchronized-monitor edge case in Nothing to flagI didn't find any new correctness, safety, or documentation-accuracy issues in this update. The one thing the prior review surfaced (the redundant/misleadingly-documented |
Chaos Pain Suite is red, and it is not this PR - it reproduced the open silent-stall bugDiagnosed rather than retried-to-green. Recording the evidence so nobody has to re-derive it. What actually failed
That is the silent-stall-after-rebalance class (confluentinc#857) - group STABLE, heartbeats flowing, backlog frozen. Known, root cause still open. The chaos suite did its job; this is a real product signal, not noise, and it should not be quarantined or bounded away. Why it cannot be this PR
Runner saturation is the aggravating factor, not the causeThe failing run logged the load signature throughout - That is context for why it surfaced now, not a reason to raise the bound. The probe is measuring the right thing. What I did and did not do
Whatever it reports, the analysis above stands: this suite's failure is independent of a test-only refactor with no main-code delta. If it goes green on re-run, that confirms intermittency under load; if it fails again, that is a stronger confluentinc#857 datapoint and worth attaching to that issue rather than to this PR. |
Scope note for whoever picks this upThis PR now closes #40 - the description was changed to carry a real closing link, so #40 will close on merge. Flagging it because the original wording said "closes the first half", and the second half is closed by verdict rather than by code. First half - done by doing it: Second half - done by documented decision, not by code. The audit ran. The remaining high-similarity pairs are overwhelmingly cross-module clones ( If you disagree that a documented won't-do discharges the second half, remove the One caveat on the duplication numbers: the real similarity tool could not be run locally ( |
master's new issue-ref gate (735b1d3) requires a reference below #1000 to name its repo: the fork numbers from 1 and confluentinc reaches confluentinc#922, so a bare number is a coin flip. The seven bare refs on this PR's added lines are all this fork - the harness issue, the similarity issue that motivated it, and two PR numbers - so they become astubbs#NN. Verified by running the gate against this branch's diff: 7 hits before, 0 after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 3m 39s —— View job Claude finished the reviewTodo list
SummaryThis PR has already been through two substantive review rounds in this thread, both of which read the harness and scenarios in full and traced the teardown ordering, I independently re-read all 10 changed files against
Nothing new to flagI did not find any correctness, safety, or documentation-accuracy issues beyond what the prior rounds already surfaced and fixed. The teardown ordering, |
Master's 735b1d3 mirrored all 78 confluentinc issues into this fork, which made a bare `#NN` ambiguous: the fork numbers from 1 and confluentinc reaches confluentinc#922, so the ranges overlap completely. The house convention is now `#159` / `confluentinc#526`, and `.github/scripts/issue-ref-gate.js` enforces it on added lines via the PR Checklist job. This branch predates that and had 15 unqualified refs across three files - `#159`, `#40`, `#202`, `#206`, `#162`, `confluentinc#861`, `confluentinc#526`, `confluentinc#271`. Every one is now qualified. Verified by running the gate module itself over this branch's diff rather than by eye: 15 findings -> 0. `upstream #NNN` is still accepted by the gate, but converted anyway - "upstream" names a role rather than a repo, and this repo is upstream to anyone who forks it, which is why master moved off it. No wording changed beyond the references themselves, except the in-flight note, which now also records that #159 IS the upstream-mirror issue for confluentinc#526 - that is where the fork<->upstream mapping lives now that the map entry is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
The pair I recorded as 37.5% came back 37.43% after merging master, which touched none of these files - the check is corpus-relative TF-IDF, so every number moves when anything in the repo does. Recording the decimals invites the same correction on every merge. Also fixes a figure I got wrong: overall duplication fell 0.39% on PMD CPD and 0.42% on jscpd, not 0.42% on both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 4m 36s —— View job Claude finished the reviewTodo list
SummaryThis thread has already been through three thorough review rounds covering the harness code itself (teardown ordering,
One finding: the new "Cross-module test clones" section in
|
| Pair | Doc says | Report actually says |
|---|---|---|
MutinyBatchTest ↔ ReactorBatchTest |
~94% | 78.94% |
MutinyPCTest ↔ ReactorPCTest |
~86% | 71.18% |
MutinyUnitTestBase ↔ ReactorUnitTestBase |
~80% | 31.93% |
TransactionAndCommitModeTest ↔ VeryLargeMessageVolumeTest |
~88% | 60.73% |
TestConventionsArchTest (all 6 pairs) |
~98% | 89.65-91.08% |
Every figure is overstated, by 7 to 48 points, all in the same direction. That's not corpus-relative drift (the kind this PR's own ba77e67 commit just corrected elsewhere was ~0.06 points) — it looks like the same stdlib-estimation-script problem the PR description already owns up to for the CommitRejectionTestBase/MockConsumerTestBase prediction ("the real tool won't build locally... the prediction came from a stdlib stand-in"), except that correction was applied to the MockConsumer-scenario numbers and the harness-pair prediction, but not to this cross-module table.
It's a bit pointed that the sentence immediately preceding these numbers in the same file is "Estimate similarity from the tool, not from a reading of the source." Given how carefully this PR otherwise re-derived and corrected every other similarity figure against the actual CI report (including a whole follow-up commit just for decimal drift), this block looks like it was missed rather than a deliberate choice — worth either correcting the five figures to match the linked report, or loosening them to bands the way the MockConsumer*Test figures were revised to (34-37% rather than fixed decimals) so they don't need re-verification on every merge either.
Nothing else to flag
No correctness, safety, or test-coverage issues beyond the doc figures above — the harness extraction itself, teardown ordering, @Timeout inheritance, and the strengthened getFailureCause() assertion all hold up under this independent re-verification, consistent with the prior review rounds in this thread.
(branch: refactor/40-mockconsumer-test-dedup)
#213 documented on DrainingCloseable#close() that a bare close() is the non-draining one. This comment was written before that existed and explained the delegation itself, which is now a second copy of a fact with a proper home. Keep only what the javadoc cannot carry: why teardown wants the non-draining close specifically - it runs on the failure path, so it must not be able to hang on in-flight work against a consumer still being made to misbehave. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RtNUsxokE9g2pSEjBHZqNA
|
Claude finished @astubbs's task in 2m 57s —— View job Claude finished the reviewTodo list
SummaryThis thread has already been through four review rounds covering the harness itself (teardown ordering,
Still open: the
|
| Pair | Doc says | Report actually says |
|---|---|---|
MutinyBatchTest ↔ ReactorBatchTest |
~94% | 78.94% |
MutinyPCTest ↔ ReactorPCTest |
~86% | 71.18% |
MutinyUnitTestBase ↔ ReactorUnitTestBase |
~80% | 31.93% |
TransactionAndCommitModeTest ↔ VeryLargeMessageVolumeTest |
~88% | 60.73% |
TestConventionsArchTest (6 pairs) |
~98% | 89.65–91.08% |
This was already raised in the 2026-08-06T03:48:01Z review round in this thread, with the same table. Nothing has changed in docs/refactoring.md at those lines since (git log -p -- docs/refactoring.md shows only the unrelated ba77e67c band-quoting edit, which touched the CommitRejectionTestBase ↔ MockConsumerTestBase paragraph a few lines below, not this one). The commits since then (1a2d8328, ba77e67c, 7942cdfe) don't touch these five figures.
It remains a bit pointed that the sentence immediately after this block, in the very next paragraph the PR added, says "Estimate similarity from the tool, not from a reading of the source" — while this block's own numbers are still the stdlib-estimate values rather than the measured ones, unlike every other similarity figure this PR corrected. Worth either fixing the five figures to match the linked report, or loosening them to a band (as was done for the MockConsumer*Test figures — 34-37% rather than fixed decimals) so they don't need re-verification on every future merge.
Nothing else to flag
No correctness, safety, or test-coverage issues beyond the doc figures above. The harness extraction itself — teardown ordering, @Timeout inheritance, the strengthened getFailureCause() assertion, and the close-hook removal — all hold up under this independent re-verification, consistent with every prior round in this thread. The only new commit since the last review is a comment-only trim that checks out against the javadoc it now points to.
|
Claude finished @astubbs's task in 3m 41s —— View job Claude finished the reviewTodo list
SummaryNothing has changed on this branch since the last review round in this thread —
Still open: the
|
| Pair | Doc says | Report actually says |
|---|---|---|
MutinyBatchTest ↔ ReactorBatchTest |
~94% | 78.94% |
MutinyPCTest ↔ ReactorPCTest |
~86% | 71.18% |
MutinyUnitTestBase ↔ ReactorUnitTestBase |
~80% | 31.93% |
TransactionAndCommitModeTest ↔ VeryLargeMessageVolumeTest |
~88% | 60.73% |
TestConventionsArchTest (6 pairs) |
~98% | 89.65–91.08% |
This was already raised twice in this thread (2026-08-06T03:48:01Z and 2026-08-06T04:10:35Z) with the same table, and no commit since has touched those lines (git log -p -- docs/refactoring.md confirms only the unrelated ba77e67c band-quoting edit landed, a few lines below, for the CommitRejectionTestBase/MockConsumerTestBase paragraph). It remains a bit pointed that the sentence right after this block, in the same paragraph this PR added, says "Estimate similarity from the tool, not from a reading of the source" — while this block's own five numbers are still the stdlib-estimate values rather than the measured ones, unlike every other similarity figure this PR corrected (including a whole follow-up commit, ba77e67c, just for decimal drift on the MockConsumer*Test figures).
Worth either correcting the five figures to match the linked CI report, or loosening them to a band the way the MockConsumer*Test figures were revised to (34-37% rather than fixed decimals), so they don't need re-verification on every future merge either.
Nothing else to flag
No correctness, safety, or test-coverage issues beyond the doc figures above. The harness extraction itself — teardown ordering, @Timeout inheritance, the strengthened getFailureCause() assertion, and the close-hook removal — all continue to hold up under this independent re-verification, consistent with every prior round in this thread.
(branch: refactor/40-mockconsumer-test-dedup)
1 participant
Closes #40 - the first half by extraction, the second by a documented verdict (see Secondary audit below).
Description
The six
MockConsumer*test classes each carried their own copy of the same wiring: build aMockConsumer, hand-rebalance the partition in, tell PC about it separately, update beginning offsets, construct and subscribe PC, feed records, collect them out of the user function, tear down. The similarity check flagging two of them at 70.7% on #34 is the symptom; the cause is that the wiring was copied rather than shared, so it drifted - the daemon-thread comment about PIT attributing a strayaddRecord()to whatever test runs next in the JVM appears verbatim in two files, and its cleanup exists only in those two.MockConsumerTestBasenow owns that wiring. A scenario supplies its failure behaviour (createMockConsumer()) and the options it needs (customiseOptions(..)), and nothing else.AbstractParallelEoSStreamProcessorTestBase. That base wires a Mockito-spiedLongPollingMockConsumerand aMockProducer; the whole subject of these tests is what PC does when the consumer misbehaves in ways only a hand-writtenMockConsumersubclass can express. The javadoc says so, so a future session does not "fix" it.CommitRejectionTestBase, which had already extracted its own half of the same wiring, now sits on the harness too - so a third rejection reason is one method rather than another copy.Deliberately left duplicated: every scenario keeps its own Awaitility block, with its own timeout, in its own file. Those are the point of each test, and the timeouts are scenario-specific - each has to clear that scenario's simulated outage window. Hoisting them would mean opening two files to learn what a test actually checks, which is a net loss even though it would cut more lines.
Teardown is now uniform and strictly stronger than what it replaced: the record feed is interrupted and joined (it was interrupted only, and only in two of the classes), before PC is closed rather than after, and
Awaitility.reset()runs first so a throwing close cannot skip it.Teardown closes PC with
close(), which is the non-draining close (DrainingCloseable.close()delegates tocloseDontDrainFirst()). Review flagged thatCommitRejectionTestBasewas overriding acloseParallelConsumer()hook to callcloseDontDrainFirst()explicitly - functionally identical to the default, so the hook had no real user and its javadoc ("override where draining first is wrong") stated the opposite of what the default did. Hook and override both removed; the fact is now a comment at the one place that closes.Two real defects found on the way
@Timeout(60000L)on three of these classes meant 60000 seconds - JUnit's default unit - i.e. no timeout at all. Replaced with@Timeout(120)on the base (@Timeoutis@Inherited), real headroom over the longest scenario (25s measured) and an actual guard against a wedgedMockConsumertest.MockConsumerRebalanceInProgressTest's javadoc still linkedMockConsumerTestWith{CommitTimeout,SaslAuthentication}Exception, renamed away in dc44e20.javacdoes not check{@link}without doclint, so it rotted silently.One assertion added, none weakened
MockConsumerEarlyCloseTestasserted only thatclose()returned. It now also asserts PC ended closed with no failure cause - "shut down cleanly", as its javadoc has always claimed, rather than "died in a way that also reports closed". Verified to hold.Secondary audit (#40's second half): done, deliberately not acted on
The remaining high-similarity pairs across
src/test/andsrc/test-integration/are overwhelmingly cross-module clones -MutinyBatchTest/ReactorBatchTest~94%,MutinyPCTest/ReactorPCTest~86%. Deduplicating those means a generified test base in core's test-jar that each module parameterises with its own processor type: a different and much larger job, and exactly the scope creep #40 says it does not want. The one within-module pair above the check'sfail_above: 80(TransactionAndCommitModeTest/VeryLargeMessageVolumeTest, ~88%) is broker ITs and wants Docker to verify, not a desk refactor. TheTestConventionsArchTestx4 at ~98% are already documented as irreducible.All of it is ranked with a verdict in
docs/refactoring.mdso the next reader does not re-derive the audit.ProducerManagerTest, named in the issue, turned out to have no cross-file duplication left: it already uses the sharedPCModuleTestEnv/ModelUtilsinfrastructure, and what it repeats is one line ofAwaitility.reset().Similarity check: the prediction was wrong, and the numbers are in. This PR expected the
extraction to push
CommitRejectionTestBase↔MockConsumerTestBaseto ~70% - two small abstractharnesses in one package sharing imports. It did not happen: neither file appears in the report at
all, against anything, and the check's reporting floor is 30%. What the run actually shows is the
thing #40 asked for - the
MockConsumer*Testscenarios now pair in the 34-37% band(
CommitTimeout↔EarlyCloseandCommitTimeout↔Saslaround 37%,EarlyClose↔Saslaround34.5%), down from the 70.7% that #34 flagged. The measure is corpus-relative, so the
decimals drift with unrelated changes - this pair moved 37.49 -> 37.43 across the master merge, which
touched none of these files; the band is the durable part. PMD CPD: no new clones, duplication -0.39%. jscpd: -0.42%, and its one "new clone" is the
8-line package + copyright + import block shared by
MockConsumerEarlyCloseTestandMockConsumerSaslAuthenticationTest, every line of which both files use. Docs corrected to themeasured values rather than the estimate.
Notes
MockConsumerTestBaseis an extraction from upstream-derived files, so it keeps the Confluent header plus the modifications line and is registered inEXTRACTED_FROM_UPSTREAMinbin/check-copyright-headers.sh.Verification
bin/ci-unit-test.sh, all modules): green.bin/check-copyright-headers.shand its self-test: green.Checklist
docs/inflight/test-mockconsumer-harness.md(how to add a scenario, and how to read a similarity comment about these files) anddocs/refactoring.md(the deferred cross-module audit)MockConsumerEarlyCloseTestN/A- no workflow or runner changes; the only non-test file is a path registration inbin/check-copyright-headers.sh