Add Cluster Coordination Serialisation Tests#142755
Add Cluster Coordination Serialisation Tests#142755elasticsearchmachine merged 11 commits intoelastic:mainfrom
Conversation
Replaces MessagesTests, which contains a single hashcode test for a number of cluster/coordination classes, with individual serialisation test suites with greater coverage
|
As a note, these tests were added via LLM during spacetime, and since the tests heavily rely on randomisation, the newly added tests have been run a couple of thousand times to ensure there are no failure paths |
|
Pinging @elastic/es-distributed (Team:Distributed) |
pxsalehi
left a comment
There was a problem hiding this comment.
There are some inconsistencies and verbosity I guess due to this being LLM-generated. I've left a few suggestions but it would apply to the other classes too. Please address those before merging.
| DiscoveryNode sourceNode = create(); | ||
| long term = randomNonNegativeLong(); | ||
| long version = randomNonNegativeLong(); |
There was a problem hiding this comment.
you can probably just inline these in the call.
| switch (field) { | ||
| case 0 -> sourceNode = randomValueOtherThan(sourceNode, DiscoveryNodeUtils::create); | ||
| case 1 -> term = randomValueOtherThan(term, ESTestCase::randomNonNegativeLong); | ||
| default -> version = randomValueOtherThan(version, ESTestCase::randomNonNegativeLong); |
There was a problem hiding this comment.
typically we leave default for the invalid case and throw an assertion.
|
|
||
| public class DiscoveryNodeUtils { | ||
|
|
||
| public static DiscoveryNode create() { |
There was a problem hiding this comment.
maybe instead call this randomDiscoveryNode() ?
| DiscoveryNode masterCandidateNode = instance.getMasterCandidateNode(); | ||
| long term = instance.getTerm(); | ||
|
|
||
| int field = between(0, 1); |
There was a problem hiding this comment.
I'd prefer something like (w/ or w/o else):
if (randomBoolean()) {
return new ...
}
return new ...
…sticsearch into spacetime-join-status
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis pull request restructures test coverage for Elasticsearch cluster coordination messages. Eight new wire-serialization test classes are introduced, each extending ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
`MessagesTests` contained a number of serialisation tests for various classes in `cluster/coordination`. This commit introduces individual serialisation tests for each of them to provide greater test coverage. It also introduces `PublishRequestTests` since `PublishRequest` only had a single unit test before
`MessagesTests` contained a number of serialisation tests for various classes in `cluster/coordination`. This commit introduces individual serialisation tests for each of them to provide greater test coverage. It also introduces `PublishRequestTests` since `PublishRequest` only had a single unit test before
MessagesTestscontained a number of serialisation tests for various classes incluster/coordination. This commit introduces individual serialisation tests for each of them to provide greater test coverage. It also introducesPublishRequestTestssincePublishRequestonly had a single unit test before