nick-boss-tech commented on PR #5211: URL: https://github.com/apache/solr/pull/5211#issuecomment-6388351069
🤖 *AI text below* 🤖 *(reviewed by Nick Shanin)* @gerlowskija, here is a concrete version of a test suggestion for this test. It builds on the idea of checking the leftover entries right after the check returns, and it comes with a run on this change. It is based on the current head of this PR, `6f72e0cac151`. **The problem.** In `testStartupWaitsForLeftoverEphemeralNodesToDisappear`, the test joins the deleter thread in a `finally` block, and only after that asserts that the leftover paths are gone. If `checkForExistingEphemeralNode()` returned too early, the join would still wait for the deleter to remove the leftovers, and the assertion afterwards would pass. So the test cannot tell an early return from a correct one. This is about test sensitivity. I have not found a problem in the production code. **The change.** The change is in `solr/core/src/test/org/apache/solr/cloud/ZkControllerTest.java` only. The absence assertion now runs right after `checkForExistingEphemeralNode()` returns, before the join. The join stays in `finally`, with a 30 second timeout so it cannot hang. The deleter thread's exception is captured and checked after the join, instead of being thrown inside the thread where the test would not see it. The test also checks that the deleter thread has finished. The production code is not touched. Diff of this change only: https://github.com/nick-boss-tech/solr/commit/f00c7bde83bf5d82ac4607ecf086a4397eca563b Compare from the PR head to the new commit: https://github.com/nick-boss-tech/solr/compare/6f72e0cac151d139626299a680f90a7c9c123ceb...f00c7bde83bf5d82ac4607ecf086a4397eca563b **What I ran.** Each run was on Linux (aarch64) in a local copy, with the same temporary change where noted. - At `6f72e0cac151` (the PR head), I temporarily emptied the body of `checkForExistingEphemeralNode()`, so it returns immediately. The test as it stands on the PR still passes. So it does not catch that early return. - At `f00c7bde83bf` (the new commit), with the same temporary change, the updated test fails at its new assertion, with the message `should be gone when the check returns` for `/node_roles/overseer/preferred/127.0.0.1:8983_solr`. That is the failure the test should produce. - At `f00c7bde83bf`, with no temporary change, `ZkControllerTest` runs 13 tests with 0 failures and 0 errors. Two tests are skipped by their existing `@Ignore` annotations: `testVersionCompatibilityFailsStartup` and `testMinorVersionCompatibilityFailsStartup`. **Limits.** All of this ran on one Linux machine, not on Windows or in CI. The one second delay before the deleter removes the leftovers still provides the timing margin, so the test depends on that margin, just as the original did. A tighter handshake would need a point of coordination inside the production wait, and I did not add one, since that would mean changing production code for the test. Would a test-only change like this be useful here, or would you rather keep the test as it is? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
