-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fixes integration tests against latest 7.7.0-SNAPSHOT #4545
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
…roperties to NodeInfo. Fixes broken integration test against 7.7.0-SNAPSHOT
- Change date histogram aggregations to use minimum document = 0 in order to pass server validation. - Changes terms query to use a string value to pass server validation - Change search template API test to not tie to a specific server message substring
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.
The changes look OK.
With the move to MinimumDocumentCount = 0
, does this affect assertions on some tests run e.g. missing buckets in a series?
else | ||
response.ServerError.Error.Reason.Should().Contain("unknown query [atch]"); | ||
} | ||
protected override void ExpectResponse(ISearchResponse<Project> response) => response.ServerError.Should().NotBeNull(); |
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.
maybe
response.ServerError.Error.Reason.Should().Contain("query");
for both? It would provide a slightly stronger assertion for why there is an error
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.
I was just conscious that binding the test to the error message probably isn't a good idea as the error message can change.
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 fair point, historically verifiable! Is there some other way we can assert that the error is related to what we expect it to be?
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-master master
# Navigate to the new working tree
cd .worktrees/backport-master
# Create a new branch
git switch --create backport-4545-to-master
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 2a73a4e26fc731442d02650eeb2b77566a6450c4
# Push it to GitHub
git push --set-upstream origin backport-4545-to-master
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-master Then, create a pull request where the |
- Change date histogram aggregations to use minimum document = 0 in order to pass server validation. - Changes terms query to use a string value to pass server validation - Change search template API test to not tie to a specific server message substring Fixes #4521 Fixes #4508 Co-authored-by: Stuart Cam <stuart.cam@elastic.co>
- Change date histogram aggregations to use minimum document = 0 in order to pass server validation. - Changes terms query to use a string value to pass server validation - Change search template API test to not tie to a specific server message substring Fixes #4521 Fixes #4508 (cherry picked from commit 2a73a4e)
Fixes #4521
Fixes #4508