-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Strengthen field-caps responses wire test #84596
Conversation
Pinging @elastic/es-search (Team:Search) |
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, this is good to have!
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
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.
left a couple of comments, thanks for working on this.
Couple of thoughts I left on #83494 still apply I think: do we also test cases where the mapping hash does not come back from an older node, yet we write the response to another node, which may or may not support the mapping hash? One more thing I do in these cases is test reading the response from an older version by storing the older format in base64 obtained from the previous version, rather than simulated from the code of the current version.
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
@jtibshirani @javanna Thanks for reviews.
I am not sure if we need this special test? Anyway, we have these scenarios covered in our tests: with and without mapping hash with different wire versions.
I can add this test, but I am not sure its benefit. Also, we prefer randomized tests for testing wire compact. |
Can you expand on why you find it a special test? I was thinking that in the context of CCS, the one I described is a real-life scenario that our tests may not cover, but I am happy to discuss this.
Agreed that randomized tests are good and what you are adding expands our current coverage, but we can only simulate what would be written if the stream output was set to a certain previous version, and there are no guarantees that that is what the previous version really writes? What I am suggesting is that we read from the real bits that the previous version writes. Do you see what I mean? |
@javanna I understand your comments, and I think there's disagreement on testing BWC here.
I think we should write BWC tests with cross clusters with mixed versions instead.
I don't see many of us doing this. I think we should write mixed cluster tests (e.g., #84455) instead. Anyway, let's move on. I added the tests as you suggested. Would you please take another look? Thank you! |
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.
besides the comments I left, one minor peculiarity of these tests is that we create a list of index responses. Should we rather test using the FieldCapabilitiesNodeResponse which holds what we are creating in these tests?
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
...er/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesIndexResponseTests.java
Outdated
Show resolved
Hide resolved
@jtibshirani @javanna Thanks for reviews. |
Relates #83494