Add mock-based unit test for NodeHealth cloud-mode logic - #4703
Conversation
Covers the ZK-failure branches (client closed/disconnected, node missing from live nodes), core-loading-incomplete and unhealthy-cores paths with plain mocks instead of a real cluster. Removes the two tests from NodeHealthSolrCloudTest that each booted an extra Jetty node just to break its ZK connection, keeping the happy-path tests that exercise the real HTTP/Jersey wiring.
dsmiley
left a comment
There was a problem hiding this comment.
I like that this increased code coverage (tested more scenarios) -- thanks.
Your opening premise seems backwards to me. Starting with a testing technique (mocking) and attempting to use it somewhere random seems backwards. Wouldn't we start with a slow test and see what that test needs to be better -- who knows what? Or target a known piece of poor infrastructure to change/elliminate. Any way, your random choice and the results turned out well here.
I confess I hate mocking; I use it very rarely. It lies to you; you create a sham world that you hope replicates reality, yet reality may be otherwise. Test passes by design; fools you into thinking bug-free, especially for something you explicitly test for. Sometimes creating the mocks is awkward; albeit wasn't too bad in this case, thankfully. It's also sad when an NPE or similar happen and we feel the need to add checks in non-test code to guard against provably impossible things. Didn't happen here, thankfully.
Addresses review feedback: CoreDescriptor is a simple POJO, and when constructed with a ZkController it builds its own CloudDescriptor.
This PR is backward on purpose. I did not attempt to delve into a formal process of assessing and improving the slowest tests (like I did a few months ago). Rather I decided to attempt LLM assisted test improvement along two axis - this one where mocks could help simplify complex failure scenarios (which is one good use of mocks). And the other where we spin up the entire sol(a)r system to test something that won't need a big cluster. So from a random set of 50 test classes I found two candidates. Which is a hint that we may have perhaps hundreds of such opportunities across the codebase. I believe we have grown numb to writing full integration tests for every single use case. Integration tests has their place and we should not stop writing them. But we should always go with lower level tests first. I'm hoping we can get to a place some day where Take ExtractingRequestHandler as an example. Currently we spin up a real TikaServer with Testcontainers to test the component. This is integration testing and should ideally be opt-in. For everyday test runs it would be sufficient to mock |
assertThat is inherited from LuceneTestCase, so the MatcherAssert static import was never bound.
(cherry picked from commit 45206ae)
I asked Claude to pick a unit test that can likely benefit from mocking instead of complex cluster setups, and it picked
NodeHealthSolrCloudTest. Not because it was the slowest or anything, just an example of potential for using more mocks:NodeHealthSolrCloudTesthad two failure-scenario tests that each booted an extra Jetty node just to break its ZK connection — while the code under test (NodeHealth.getClusterState()and the cloud-mode health checks) only reads state reachable throughCoreContainerand is trivially mockable.This PR:
NodeHealthTest— a plainSolrTestCasewith Mockito (no Jetty/ZK, runs in milliseconds), following the existing pattern ofGetNodeCommandStatusTestin the same package. It mocks theCoreContainer → ZkController → ZkStateReader → SolrZkClientchain and covers:CoreContainer→ 500 (previously untested)requireHealthyCoreswith core loading incomplete → 503 (previously untested)requireHealthyCoreswith all cores healthy → OKClusterStateMockUtil)NodeHealthSolrCloudTestby removing the two extra-Jetty failure tests (now covered by the mocks), keeping the happy-path tests that exercise the real HTTP/Jersey wiring on the shared 1-node cluster.Test-only change, no changelog entry.
Measured timings (3 local runs each, averaged)
NodeHealthSolrCloudTestNodeHealthTest(new)The two removed extra-Jetty tests accounted for ~1s (a third) of the cloud suite; their mock replacements run in milliseconds (the 0.6s suite time is almost entirely one-time Mockito/class-init overhead). Net: total time for the classes drops ~15% while going from 4 to 10 tests, including two previously untested error branches — and the ZK-connection-killing tests, the flakiest kind, no longer run against a real cluster.