Conversation
astubbs
commented
May 6, 2022
| </dependency> | ||
|
|
||
| <!-- Build dependency --> | ||
| <dependency> |
Owner
Author
There was a problem hiding this comment.
possible to use old path version instead?
astubbs
added a commit
that referenced
this pull request
Apr 16, 2026
Root-causes rather than excludes. Two categories of real test bugs that PIT's slower instrumented JVM and different test ordering surfaced: 1. Broken teardown override hiding base-class cleanup. ParallelEoSSStreamProcessorRebalancedTest overrode @AfterEach close() with an empty body. Each parameterised invocation created a new ParallelEoSStreamProcessor that was never closed and never triggered the base class's Awaitility.reset(). By invocation #2, accumulated resource pressure broke timing. Fix: delete the override so the base class's close() runs. 2. Standalone tests skipping base-class cleanup + tight margins. MockConsumerTestWithSaslAuthenticationException and ProducerManagerTest don't extend AbstractParallelEoSStreamProcessorTestBase, so they got no Awaitility.reset() or pc cleanup. The SASL test additionally mutated the global (shaded) Awaitility default timeout without resetting. Fixes: - MockConsumerTestWithSaslAuthenticationException: scope timeout locally (atMost(90s)) instead of setDefaultTimeout, and add @AfterEach closing parallelConsumer. - ProducerManagerTest: add @AfterEach with Awaitility.reset(); bump commitLockAcquisitionTimeout 2s→10s; give the failing test's three bare await() calls and BlockedThreadAsserter explicit 20s timeouts instead of the 10s default (too tight under PIT). 3. PCMetricsTest.metricsRegisterBinding - two bare await() calls (lines 94, 180) used Awaitility's 10s default, too tight for 1500 records through PC's pipeline under PIT instrumentation. Bumped to 120s to match the atMost budgets already used elsewhere in the same method. Also drops -DexcludedTestClasses from the PIT maven command now that all four tests pass their baseline. Base-class @AfterEach Awaitility.reset() stays as a general leakage guard. Verified all four classes' tests pass locally under Surefire (10 tests, 0 failures, 1m 19s). PIT baseline will be verified by CI. Closes #39 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Apr 20, 2026
Root-causes rather than excludes. Two categories of real test bugs that PIT's slower instrumented JVM and different test ordering surfaced: 1. Broken teardown override hiding base-class cleanup. ParallelEoSSStreamProcessorRebalancedTest overrode @AfterEach close() with an empty body. Each parameterised invocation created a new ParallelEoSStreamProcessor that was never closed and never triggered the base class's Awaitility.reset(). By invocation #2, accumulated resource pressure broke timing. Fix: delete the override so the base class's close() runs. 2. Standalone tests skipping base-class cleanup + tight margins. MockConsumerTestWithSaslAuthenticationException and ProducerManagerTest don't extend AbstractParallelEoSStreamProcessorTestBase, so they got no Awaitility.reset() or pc cleanup. The SASL test additionally mutated the global (shaded) Awaitility default timeout without resetting. Fixes: - MockConsumerTestWithSaslAuthenticationException: scope timeout locally (atMost(90s)) instead of setDefaultTimeout, and add @AfterEach closing parallelConsumer. - ProducerManagerTest: add @AfterEach with Awaitility.reset(); bump commitLockAcquisitionTimeout 2s→10s; give the failing test's three bare await() calls and BlockedThreadAsserter explicit 20s timeouts instead of the 10s default (too tight under PIT). 3. PCMetricsTest.metricsRegisterBinding - two bare await() calls (lines 94, 180) used Awaitility's 10s default, too tight for 1500 records through PC's pipeline under PIT instrumentation. Bumped to 120s to match the atMost budgets already used elsewhere in the same method. Also drops -DexcludedTestClasses from the PIT maven command now that all four tests pass their baseline. Base-class @AfterEach Awaitility.reset() stays as a general leakage guard. Verified all four classes' tests pass locally under Surefire (10 tests, 0 failures, 1m 19s). PIT baseline will be verified by CI. Closes #39 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 tasks
This was referenced Jul 28, 2026
astubbs
added a commit
that referenced
this pull request
Jul 29, 2026
The docstring claimed the test verified the #57 OffsetMapCodecManager caching as well as the confluentinc#859 Set de-dup, but it only guards the de-dup. Confirmed by experiment: reverting the caching (Set intact) leaves it green, while reverting Set->List makes it fail. The caching effect is masked - the Set already collapses the duplicate registrations, and PartitionState constructs its own per-assignment OffsetMapCodecManager registering the same fixed-id meter regardless. Rewrite the docstring to scope it to the confluentinc#859 de-dup and explain why the caching cannot be asserted here; record in docs/refactoring.md that the caching lacks isolated coverage (no construction seam; redundant for the leak). Doc-only; test still green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5 tasks
This was referenced Aug 3, 2026
4 tasks
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
The reference gate requires any #NNN below the threshold to name its repo, because the fork's numbering sits entirely inside upstream's range and a bare number is a coin flip. It checks added lines only, so it never fired on text nobody was editing - leaving the convention true of the files the original work touched and false of the rest of the tree. Two passes, on the same lines and so landing together: - 366 bare references gain their repo. All 77 distinct numbers were resolved against BOTH repos first: 62 of them exist in each, meaning different things, so the classification is per-occurrence rather than per-number. #188 and #195 are fork mirror issues in the release notes while confluentinc#188 and confluentinc#195 are the upstream bugs cited in test comments in the same tree. - 24 `upstream #NNN` uses become the owner form. That form passed the gate but names a relationship rather than a repository, and this fork is itself upstream to anyone who forks it. Three sets are deliberately NOT prefixed, because they are not references: author ordinals ("run #1", "produce #1/#2", "NUDGE #1/#2") annotating log excerpts, reworded to plain numbers; the changelog gate's fixture, which asserts that a *bare* #NN is not a citation and would have been destroyed by qualifying it, so it moves above the threshold as a fake #999104; and upstream-pr-analysis.adoc, which is exempt and written entirely in upstream terms. README link text keeps its qualifier even though the URL beside it already names the repo - the gate can see a link target, a reader cannot, and this fork has its own #12. Quoted upstream titles keep the quotation intact with the number appended rather than having the owner inserted mid-title. README is generated: the edit is in src/docs/README_TEMPLATE.adoc. @tag("confluentinc#355") becomes @tag("confluentinc#355"). Verified nothing selects on that tag - no pom, workflow or script filters it - and both classes still collect and pass. The 14 upstream-derived Java files gain the "Modifications Copyright" line the provenance-aware header check requires of any file changed since the fork point. docs/inflight/next-qualify-remaining-refs.md is deleted: this is everything it tracked, and in-flight files do not outlive their work. No behaviour change.
astubbs
added a commit
that referenced
this pull request
Aug 7, 2026
The reference gate requires any #NNN below the threshold to name its repo, because the fork's numbering sits entirely inside upstream's range and a bare number is a coin flip. It checks added lines only, so it never fired on text nobody was editing - leaving the convention true of the files the original work touched and false of the rest of the tree. Two passes, on the same lines and so landing together: - 366 bare references gain their repo. All 77 distinct numbers were resolved against BOTH repos first: 62 of them exist in each, meaning different things, so the classification is per-occurrence rather than per-number. #188 and #195 are fork mirror issues in the release notes while confluentinc#188 and confluentinc#195 are the upstream bugs cited in test comments in the same tree. - 24 `upstream #NNN` uses become the owner form. That form passed the gate but names a relationship rather than a repository, and this fork is itself upstream to anyone who forks it. Three sets are deliberately NOT prefixed, because they are not references: author ordinals ("run #1", "produce #1/#2", "NUDGE #1/#2") annotating log excerpts, reworded to plain numbers; the changelog gate's fixture, which asserts that a *bare* #NN is not a citation and would have been destroyed by qualifying it, so it moves above the threshold as a fake #999104; and upstream-pr-analysis.adoc, which is exempt and written entirely in upstream terms. README link text keeps its qualifier even though the URL beside it already names the repo - the gate can see a link target, a reader cannot, and this fork has its own #12. Quoted upstream titles keep the quotation intact with the number appended rather than having the owner inserted mid-title. README is generated: the edit is in src/docs/README_TEMPLATE.adoc. @tag("confluentinc#355") becomes @tag("confluentinc#355"). Verified nothing selects on that tag - no pom, workflow or script filters it - and both classes still collect and pass. The 14 upstream-derived Java files gain the "Modifications Copyright" line the provenance-aware header check requires of any file changed since the fork point. docs/inflight/next-qualify-remaining-refs.md is deleted: this is everything it tracked, and in-flight files do not outlive their work. No behaviour change.
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
1 participant
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.
No description provided.