-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Assert no partial results in integration tests #129293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This injects assertions in spec-based and YAML integration tests that the received results aren't partial. This only happens in case the tests themselves don't set `"allow_partial_results": true`
| Map<String, Object> answer = runEsql(builder.query(query), testCase.assertWarnings(deduplicateExactWarnings())); | ||
|
|
||
| var isPartial = answer.get("is_partial"); | ||
| assertTrue("unexpected partial results", isPartial == null || (boolean) isPartial == false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be easier to read with assertThat(..., anyOf(is(false), nullValue())). Dunno. Your call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, better, thanks.
| var apiCallSection = doSection.getApiCallSection(); | ||
| if (apiCallSection.getApi().equals("esql.query")) { | ||
| // If `allow_partial_results` is explicitly set to true, partial results are allowed. | ||
| // If it's missing, no partial results are expected, even if the parameter's default is true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of magic, I think it's a pretty reasonable sacrifice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, @bpintea. I wonder if we should also do this for other QA modules (multi-node, multi-nodes, mixed-cluster).
|
|
||
| var isPartial = answer.get("is_partial"); | ||
| assertTrue("unexpected partial results", isPartial == null || (boolean) isPartial == false); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we also extract the failures and throw them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I've extracted _clusters contents into the assertion message (we don't have more otherwise).
The changes apply to those suites too. |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Thanks Nik and Nhat. |
💔 Backport failed
You can use sqren/backport to manually backport by running |
This extends the check that no partial results are returned to the rest of the JSON-based REST ITs. Related: #129293
…0213) This extends the check that no partial results are returned to the rest of the JSON-based REST ITs. Related: elastic#129293
…0213) This extends the check that no partial results are returned to the rest of the JSON-based REST ITs. Related: elastic#129293
This injects assertions in spec-based and YAML integration tests that the received results aren't partial.
This only happens in case the tests themselves don't set
"allow_partial_results": true.Closes #129256