Skip to content

Add BulkByScrollResponse Serialization Tests#142688

Merged
joshua-adams-1 merged 3 commits intoelastic:mainfrom
joshua-adams-1:spacetime-bulk-by-paginated-search-response
Feb 24, 2026
Merged

Add BulkByScrollResponse Serialization Tests#142688
joshua-adams-1 merged 3 commits intoelastic:mainfrom
joshua-adams-1:spacetime-bulk-by-paginated-search-response

Conversation

@joshua-adams-1
Copy link
Copy Markdown
Contributor

Adds BulkByScrollResponseWireSerializingTests and removes a redundant testRoundTrip() test from BulkByScrollResponseTests

Adds BulkByScrollResponseWireSerializingTests and removes a redundant
testRoundTrip() test from BulkByScrollResponseTests
@joshua-adams-1 joshua-adams-1 self-assigned this Feb 19, 2026
@joshua-adams-1 joshua-adams-1 added >test Issues or PRs that are addressing/adding tests :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. labels Feb 19, 2026
@joshua-adams-1 joshua-adams-1 changed the title Spacetime bulk by paginated search response Add BulkByScrollResponse Serialization Tests Feb 19, 2026
private boolean includeCreated;
private boolean testExceptions = randomBoolean();

public void testRountTrip() throws IOException {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is greater serialisation test coverage from BulkByScrollResponseserializationTests than this test previously provided

@joshua-adams-1 joshua-adams-1 marked this pull request as ready for review February 23, 2026 10:59
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Feb 23, 2026
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Copy link
Copy Markdown
Member

@PeteGillinElastic PeteGillinElastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confess that I'm a little doubtful about the utility of this test. It seems to me that the most common cause of regressions in the wire serialization code is someone adding a new field and forgetting to update the serialization. But if they've forgotten that, they're definitely not going to remember to update this test, which will continue to pass.

The only alternative to that would be to do something reflection-based, which seems overkill (and is, I assume, not something that we're doing elsewhere).

Still, it's probably going to do more good than harm, so LGTM.

@joshua-adams-1
Copy link
Copy Markdown
Contributor Author

I confess that I'm a little doubtful about the utility of this test. It seems to me that the most common cause of regressions in the wire serialization code is someone adding a new field and forgetting to update the serialization. But if they've forgotten that, they're definitely not going to remember to update this test, which will continue to pass.

I think this would only be true if someone added a new field but didn't change the writeTo method, although in that case we haven't technically changed the serialisation, and so this test should still be passing, right?

What this test will catch in future is:

  • Writing a field without a transport‑version guard
  • Reading a field unconditionally
  • Gating on the wrong version
  • Changing field order across versions
  • A field is moved, renamed, or type‑changed

etc, which is much better coverage than before. Plus it tests equality and hashcode, which the original roundTrip() method did not

@joshua-adams-1 joshua-adams-1 merged commit b21e05f into elastic:main Feb 24, 2026
35 checks passed
@joshua-adams-1 joshua-adams-1 deleted the spacetime-bulk-by-paginated-search-response branch February 24, 2026 13:57
sidosera pushed a commit to sidosera/elasticsearch that referenced this pull request Feb 24, 2026
Adds BulkByScrollResponseWireSerializingTests and removes a redundant
testRoundTrip() test from BulkByScrollResponseTests
szybia added a commit to szybia/elasticsearch that referenced this pull request Feb 24, 2026
…on-sliced-reindex

* upstream/main:
  Activity logging improvements (elastic#142901)
  Fix serialization of NodeGpuStatsResponse when no GPU is present (elastic#142937)
  Add doc on master elections in DistributedArchitectureGuide (elastic#142435)
  ESQL: Account for missing StubRelation due to SurrogateExpressions replacement (elastic#142882)
  Add BulkByScrollTask Serialization Tests (elastic#142697)
  Rebalance CI test partitions to reduce Part3 bottleneck (elastic#142930)
  Mute org.elasticsearch.xpack.esql.qa.multi_node.EsqlClientYamlIT test {p0=esql/40_tsdb/to_aggregate_metric_double with multi_values} elastic#142964
  Bump OpenTelemetry dependencies (elastic#142323)
  SQL: add support for API key to JDBC and CLI (elastic#142021)
  Ensure requested capability exists (elastic#142695)
  Warn and fall back to local branches.json (elastic#142606)
  [CI] Mute testWithFetchFailures, testAddCompletionListenerScheduleErr… (elastic#142926)
  ESQL: Add support for ORC file format (elastic#142900)
  Update wolfi (versioned) (elastic#142948)
  Add BulkByScrollResponse Serialization Tests (elastic#142688)
  Run 25_id_generation with and without synthetic id (elastic#142770)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed Meta label for distributed team. >test Issues or PRs that are addressing/adding tests v9.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants