Remove dangling AwaitsFix #113941
Remove dangling AwaitsFix #113941
Conversation
The issue referenced seems to be closed, maybe this just wasn't removed properly.
cbuescher
added
>test
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
elasticsearchmachine
added
the
Team:Search Foundations
|
Something in these tests is wrong with the version randomization after the switch to 9.0. Probably didn't matter so far because the tests were muted so long. |
| public void testMinimumVersionSameAsNewVersion() throws Exception { | ||
| var newVersion = VersionInformation.CURRENT; | ||
| var oldVersion = new VersionInformation( | ||
| VersionUtils.randomVersionBetween(random(), Version.CURRENT.minimumCompatibilityVersion(), VersionUtils.getPreviousVersion()), | ||
| VersionUtils.randomCompatibleVersion(random(), VersionUtils.getPreviousVersion()), |
There was a problem hiding this comment.
So, the in the original test "old" was any random version between minimum compatibility and previous.
The new code defines "old" as any random version which is compatible with previous. "Compatible" is defined as the random version is on or after minimum compatibility of previous, and previous is on or after minimum compatibility of the random version.
Now this boils down to the same for intra-v8 as both the previous and the random version had the same minimum compatibility, while now v9 has a different (higher) minimum compatibility.
Is my reasoning sound? (I'm trying to understand why this is different and now works for any randomly generated version)
There was a problem hiding this comment.
This test hasn't been running on any major version bump as far as I can tell (it has been muted a long time). On the v8 series, Version.CURRENT.minimumCompatibilityVersion() was always 7.17 and the previous-to-Current has always been some 8.x minor version.
This doesn't work with the major version bump at the moment since Current is 9.0, previous-to-Current is 8.15.2 (!!!) due to 8.16 not having been released yet, and minimum compatible is 8.16. So currently this errors.
I believe what we want in this test is "newVersion" at whatever the main branch is currently on (9.x minors), and oldVersion anything from minimumCompatible up to previous. For now this would only be 8.16, but as soon as we move on to other 9x minor versions we draw from a larger range.
So I think we are good here, but please keep me honest.
There was a problem hiding this comment.
Yes, your explanation aligns with my understanding, thank you for double-checking this!
mark-vieira
added
auto-backport
|
@ldematte sorry this didn't move for a bit but I was running into issues with the LeakTracker when unmuting the tests that I also had to fix first. I left a note on why I think the version randomization is correct, the rest of the change now adapts to correct ref-counting on the QuerySearchResult that are now managed and tracked but haven't been adapted because the tests were muted. |
|
@cbuescher sitll looks good! Thanks! |
💚 Backport successful
|
The issue referenced seems to be closed, maybe this just wasn't removed properly.
The issue referenced seems to be closed, maybe this just wasn't removed properly.
The issue referenced seems to be closed, maybe this just wasn't removed properly.
The issue referenced seems to be closed, maybe this just wasn't removed properly.