Dry up testChunking #92530
Dry up testChunking #92530
Conversation
Most chunking implementations have a corresponding test regarding the number of chunks emitted, and they're all pretty much copies of each other. Extracting a test utility to capture this pattern is long overdue. Relates elastic#89838
DaveCTurner
added
>test
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| .getStatsResponses() | ||
| .size() + 2 | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This and couple of other tests are extending AbstractChunkedSerializingTestCase. In such cases test implementation looks identical except the function that calculates the chunk count. Is it worth moving testChunking into AbstractChunkedSerializingTestCase and adding a new abstract method to provide the chunk count for the test object? This way chunk count test would be enforced in all sub-classes instead of relying on remembering to add one
There was a problem hiding this comment.
I know what you mean, but AbstractChunkedSerializingTestCase (and indeed AbstractSerializationTestCase) derivatives are only appropriate for objects that we expect to be able to parse back from their XContent representations. That used to apply to many objects because of the HLRC, and there's still a load of legacy in this area, but these days it's less important and indeed almost all of these tests (including the one you link here) are not such tests. So in most cases I expect we still need an explicit test for chunk count.
This whole question probably deserves a bit more of an overhaul than would make sense in this PR.
DaveCTurner
added
the
auto-merge-without-approval
Most chunking implementations have a corresponding test regarding the number of chunks emitted, and they're all pretty much copies of each other. Extracting a test utility to capture this pattern is long overdue.
Relates #89838