Convert most awaitBusy calls to assertBusy #45794
Conversation
pugnascotia
added
>test
|
Pinging @elastic/es-core-infra |
|
Let's leave the c/p of The comments reference a different refactor which got shelved, but I think this will be going away soon anyway because we have a different refactor in mind which would obviate the need for it :) |
|
@elasticmachine run elasticsearch-ci/2 |
2 similar comments
|
@elasticmachine run elasticsearch-ci/2 |
|
@elasticmachine run elasticsearch-ci/2 |
|
@elasticmachine update branch |
|
The labels indicate this is for |
|
@elasticmachine update branch |
|
@hendrikmuhs there's no plan to backport, but I can look into that once I finally get this PR merged. |
|
I think this should definitely be backported to 7.x (I took the liberty to add the v7.5.0 label). Having the codebase diverge between master and 7.x without good reasons will complicate any other backports in the future. Perhaps the lesson learned here is to keep these PRs smaller and doing this kind of work more incrementally 😄 |
|
A CI that wasn't flaky would help too 😉 |
Backport of elastic#45794 to 7.x. Convert most `awaitBusy` calls to `assertBusy`, and use asserts where possible. Follows on from elastic#28548 by @liketic. There were a small number of places where it didn't make sense to me to call `assertBusy`, so I kept the existing calls but renamed the method to `waitUntil`. This was partly to better reflect its usage, and partly so that anyone trying to add a new call to awaitBusy wouldn't be able to find it. I also didn't change the usage in `TransportStopRollupAction` as the comments state that the local awaitBusy method is a temporary copy-and-paste. Other changes: * Rework `waitForDocs` to scale its timeout. Instead of calling `assertBusy` in a loop, work out a reasonable overall timeout and await just once. * Some tests failed after switching to `assertBusy` and had to be fixed. * Correct the expect templates in AbstractUpgradeTestCase. The ES Security team confirmed that they don't use templates any more, so remove this from the expected templates. Also rewrite how the setup code checks for templates, in order to give more information. * Remove an expected ML template from XPackRestTestConstants The ML team advised that the ML tests shouldn't be waiting for any `.ml-notifications*` templates, since such checks should happen in the production code instead. * Also rework the template checking code in `XPackRestTestHelper` to give more helpful failure messages.
|
Backport PR is #47112. |
|
Thanks again to @liketic for the original PR! |
Backport of #45794 to 7.x. Convert most `awaitBusy` calls to `assertBusy`, and use asserts where possible. Follows on from #28548 by @liketic. There were a small number of places where it didn't make sense to me to call `assertBusy`, so I kept the existing calls but renamed the method to `waitUntil`. This was partly to better reflect its usage, and partly so that anyone trying to add a new call to awaitBusy wouldn't be able to find it. I also didn't change the usage in `TransportStopRollupAction` as the comments state that the local awaitBusy method is a temporary copy-and-paste. Other changes: * Rework `waitForDocs` to scale its timeout. Instead of calling `assertBusy` in a loop, work out a reasonable overall timeout and await just once. * Some tests failed after switching to `assertBusy` and had to be fixed. * Correct the expect templates in AbstractUpgradeTestCase. The ES Security team confirmed that they don't use templates any more, so remove this from the expected templates. Also rewrite how the setup code checks for templates, in order to give more information. * Remove an expected ML template from XPackRestTestConstants The ML team advised that the ML tests shouldn't be waiting for any `.ml-notifications*` templates, since such checks should happen in the production code instead. * Also rework the template checking code in `XPackRestTestHelper` to give more helpful failure messages. * Fix issue in `DataFrameSurvivesUpgradeIT` when upgrading from < 7.4
|
I've merged the backport to |
Retries in this method were lost in elastic#45794. This commit reinstates them.
Retries in this method were lost in #45794. This commit reinstates them.
Retries in this method were lost in #45794. This commit reinstates them.
Retries in this method were lost in #45794. This commit reinstates them.
Retries in this method were lost in #45794. This commit reinstates them.
11 participants
Closes #28450. Convert most
awaitBusycalls toassertBusy, and use asserts where possible.This PR follows on from #28548 by @liketic. However, given the time elapsed since that PR, I implemented the changes from scratch instead of trying to merge (which would probably have taken longer). The PR was still useful though - thanks!
There were a small number of places where it didn't make sense to me to call
assertBusy, so I kept the existing calls but renamed the method towaitUntil. This was partly to better reflect its usage, and partly so that anyone trying to add a new call toawaitBusywouldn't be able to find it.I also didn't change the usage in
TransportStopRollupActionas the comments state that the localawaitBusymethod is a temporary copy-and-paste. If @polyfractal can tell me whether and how it can be removed, I'm happy to do it.Tagging @javanna as he reviwed the original PR.