voonhous commented on PR #19485:
URL: https://github.com/apache/hudi/pull/19485#issuecomment-5585869195
One more pass over `d74dd15`, this time on the two commits that had landed
after the last full read. Everything it found is fixed in `093458a` (the review
items) and `a9c354a` (a config mirror on two sibling tests, kept separate so it
can be dropped), and the description is updated to match. Nothing here is a
wrong result from the helpers: the first item was a flake in the new test
class, the rest are coverage and prose. Nine of the eleven commits are now
mine, so this needs an independent committer's approval rather than mine.
<details>
<summary>The 15 items and what changed</summary>
| | where | finding | fix |
|---|---|---|---|
| major | `TestDeltaStreamerTestHelpers.java:224` | `contains("0 evaluations
completed")` also matches 10/20/30 (18 measured locally), so the assertion
flaked on any multiple of ten | parses the count and checks it against the
condition's own tally, allowing one in-flight evaluation |
| major | `:366` | `stopThatHangsIsBounded...` never observed the bound:
widening `stop.get` to 20s left it green, 20s slower | the stop cases run under
a 5s `assertTimeoutPreemptively` ceiling (two bounds plus slack) |
| major | `TestHoodieDeltaStreamer.java:1863`, `:1850` |
`ingestionService.shutdown(true)` and the close-wait timeout were reached by no
test, since the mock's `getIngestionService()` is null | new
`stopThatOutlivesItsBoundIsForceStoppedAndLetRunOn`: stubbed service,
`verify(shutdown(true))`, lower-bound timing on the close-wait |
| minor | `HoodieDeltaStreamerTestBase.java:557,579,723,731` |
`assertRecordCount` and `assertDistanceCount` produced identical text and every
commit helper a bare `Got=N, exp >=M`, so the one-line report could not name
the assertion | every helper's message starts with its own name |
| minor | description | the sample output was the test fixture string; the
code formats `org.opentest4j.AssertionFailedError: assertAtleastNDeltaCommits:
Got=2, exp >=3 ==> expected: <true> but was: <false>` | description quotes what
the code formats |
| minor | `:1803` + description | "three times" the bound is unreachable: a
stop that times out skips the join and a stop that returns leaves nothing for
the close-wait, so at most two run in sequence | javadoc and description say
twice |
| minor | `:225` | two 60s bounds on top of the 360s wait left 120s of the
`@Timeout(600)` budget; once that blows, JUnit's `SameThreadTimeoutInvocation`
replaces the failure and demotes the diagnostic to a suppressed exception |
bound is 30s |
| minor | `TestDeltaStreamerTestHelpers.java:122`, `:142`, `:179` |
`pollingStopsOnce...` killed no mutant the other tests miss, and the `polls`
equality in both polling tests had a narrow race | the test is dropped; the
equality is dropped, `isAlive()` after `join` stays |
| minor | `:311`, `:330` | the `polling.await` result was discarded, so a
slow first poll would be reported as a broken interrupt path; the elapsed
assertion could not be what fails | latch result asserted first; elapsed check
dropped |
| minor | `TestHoodieDeltaStreamer.java:1832`, `:1850` | a caller arriving
interrupted made `awaitTermination` throw at once and skip the close-wait |
flag cleared for the wait and restored; new
`interruptedCallerStillGetsTheBoundedCloseWait` |
| minor | `HoodieDeltaStreamerTestBase.java:833` | the "future finished"
warn is false on the interrupt exit, which the direct-interrupt test drives |
branches on `dsFuture.isDone()` |
| nit | `TestJavaTestUtils.java:44` | `testNullThrowableReturnsFalse` had no
unique kill | dropped |
| nit | `TestDeltaStreamerTestHelpers.java:339-387` | the two stop tests
were 20-line twins | shared helper |
| minor | `TestHoodieDeltaStreamerWithMultiWriter.java:186`, `:263` |
`WithoutConflicts` and `testLatestCheckpointCarryOver...` wait on the same MOR
prep condition as `ForConflicts` but never got the compaction settings
f5d03a61749f (#9072) added there | `a9c354a`, separate commit |
Gates on JDK 11: checkstyle clean on both modules, `TestJavaTestUtils` 4/4,
`TestDeltaStreamerTestHelpers` 14/14 three times (~14.5s). Each new assertion
was checked against a mutation that flips only it.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
