Refactor AsyncSearchErrorTraceIT to use assertBusy #131328
Conversation
benchaplin
added
>non-issue
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
cbuescher
left a comment
cbuescher
left a comment
There was a problem hiding this comment.
thanks for the cleanup, LGTM. I left one question about whether the first conditionals in all of the changed tests are still necessary, just a possible small adjustment though, nothing that needs another review round.
| createAsyncRequest.addParameter("keep_on_completion", "true"); | ||
| createAsyncRequest.addParameter("wait_for_completion_timeout", "0ms"); | ||
| Map<String, Object> createAsyncResponseEntity = performRequestAndGetResponseEntity(createAsyncRequest); | ||
| if (createAsyncResponseEntity.get("is_running").equals("true")) { |
There was a problem hiding this comment.
Is there a case where the the search might have already completed here? I guess that would be fine since we wouldn't need to wait any longer then, just wondering if that while conditional is needed any longer with the switch to busy waiting.
There was a problem hiding this comment.
I don't think the conditional is necessary, it's fine to always send the GET _async_search request. However I think it is possible that the search has already completed, so that check should speed up the test a bit.
…king * upstream/main: Mark watcher NotMultiProjectCapable and replace deprecated multi-project methods (elastic#131313) Enable force inference endpoint deleting for invalid models and after stopping model deployment fails (elastic#129090) [ML] Remove SageMaker Elastic updates (elastic#131301) Refactor AsyncSearchErrorTraceIT to use assertBusy (elastic#131328)
3 participants
Cleaning up this test suite. Its custom 1-second delay while-loop can be replaced with assertBusy, which is more consistent with other tests and implements exponential backoff under the hood, potentially speeding up the test runtime.
I also gave requests better names.