Add remaining BWC tests for the ent-search module #98016
Conversation
kderusso
added
>non-issue
:EnterpriseSearch/Application
| && eventTime == that.eventTime | ||
| && Objects.equals(eventType, that.eventType) | ||
| && Objects.equals(xContentType, that.xContentType) | ||
| && Objects.equals(xContentType.canonical(), that.xContentType.canonical()) |
There was a problem hiding this comment.
The canonical() calls were added here to ensure that versioned XContentTypes still act as equivalent in BWC tests.
|
Pinging @elastic/ent-search-eng (Team:Enterprise Search) |
|
|
||
| static { | ||
| PARSER.declareStringOrNull(optionalConstructorArg(), NAME_FIELD); | ||
| PARSER.declareStringArray(constructorArg(), INDICES_FIELD); |
There was a problem hiding this comment.
just a note for both of us - this will not be compatible with #98036 since I removed the indices from the list search apps API - so whichever PR gets merged second, we need to make sure we get the latest changes
There was a problem hiding this comment.
That's a really good observation. I actually think it might be best to merge this one first, so we can ensure that BWC works after removing the indices. WDYT?
...main/java/org/elasticsearch/xpack/application/analytics/action/PostAnalyticsEventAction.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| protected PutSearchApplicationAction.Response mutateInstanceForVersion( |
There was a problem hiding this comment.
we have to implement this because we are now inheriting from AbstractBWCWireSerializationTestCase.
this is not related to your PR, but looking at the other (~70) implementation of this method most of them are the same where we just do return instance.
Is there a reason why this is an abstract method?
There was a problem hiding this comment.
That's a really good question.
My guess is that the assumption was, these tests wouldn't be written unless we had backwards compatibility changes to address. However, if we're going to be proactively including them (which I think is a good idea) then it would make sense to have a sane default to return the instance.
There was a problem hiding this comment.
I guess the intention is to make us think about that. If we're doing a BWC test, then something could have changed between version serializations. So probably it's let's not forget?
There was a problem hiding this comment.
(@kderusso - sorry again, I know this is not related to your PR and it's not a change I propose doing here)
I don't know if there's a clear intention behind making this method abstract or if all implementations just follow a pattern that's already in place.
This method was added as part of #69534 by @astefan.
@astefan - do you think it would make sense to provide a sane default here instead of having this be an abstract method and forcing an implementation in each subclass?
| } | ||
|
|
||
| @Override | ||
| protected PutSearchApplicationAction.Response mutateInstanceForVersion( |
There was a problem hiding this comment.
I guess the intention is to make us think about that. If we're doing a BWC test, then something could have changed between version serializations. So probably it's let's not forget?
4 participants
Adds remaining BWC tests to the ent-search module.