[Uptime] Add delay in telemetry test #75162
Conversation
shahzad31
added
release_note:skip
|
Pinging @elastic/uptime (Team:uptime) |
| @@ -119,6 +122,8 @@ export default function ({ getService }: FtrProviderContext) { | |||
| }); | |||
|
|
|||
| it('should receive expected results after calling overview logging', async () => { | |||
| // wait few seconds to make sure data is refreshed, just to avoid flakiness | |||
| await delay(2000); | |||
| // call overview page | |||
| const { body: result } = await supertest | |||
There was a problem hiding this comment.
I think it might make sense to try wrapping this supertest call in a retry block instead of hardcoding delay values. In the past when I have attempted to implement similar solutions I have usually gotten pushback because it goes against the typical pattern used when trying to keep these tests maintainable. cc @spalger
There was a problem hiding this comment.
My concern isn't about consistency, it's about the fact that the delay is only necessary if there's something we should be waiting for but we're not waiting for it.
What should these tests be waiting for? Can we add a wait for that thing? If we can't, then we can try retrying.
Delay's seem to work today, but they age out as performance characteristics change and become very difficult to maintain over time.
There was a problem hiding this comment.
i agree, will do that
| @@ -84,6 +86,7 @@ export default function ({ getService }: FtrProviderContext) { | |||
| after('unload heartbeat index', () => getService('esArchiver').unload('uptime/blank')); | |||
|
|
|||
| beforeEach(async () => { | |||
| await delay(1000); | |||
There was a problem hiding this comment.
Is the delay still needed since we've removed the check for the API response and are now just checking the response code?
There was a problem hiding this comment.
I don't see anything wrong technically with this test, but I'll defer to @justinkambic to review the meaning of the change here. I worry that we're no longer testing that we "receive expected results"
@spalger yeah we are actually already unit testing that part, it was really hard to reliably test this query. |
|
@spalger we investigated and determined that we have unit tests covering how the server processes this input. |
|
Cool, then maybe we should rename the test |
|
@elasticmachine merge upstream |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: (71 commits) [Lens] Show 'No data for this field' for empty field in accordion (elastic#73772) Skip failing lens test Configure ScopedHistory consistenty regardless of URL used to mount app (elastic#75074) Fix returned payload by "search" usage collector (elastic#75340) [Security Solution] Fix missing key error (elastic#75576) Upgrade EUI to v27.4.1 (elastic#75240) Update datasets UI copy to data streams (elastic#75618) [Lens] Register saved object references (elastic#74523) [DOCS] Update links to Beats documentation (elastic#70380) [Enterprise Search] Convert our `public_url` route to `config_data` and collect initialAppData (elastic#75616) [Usage Collection Schemas] Remove Legacy entries (elastic#75652) [Dashboard First] Lens Originating App Breadcrumb (elastic#75470) Improve login UI error message. (elastic#75642) [Security Solution] modify circular deps checker to output images of circular deps graphs (elastic#75579) [Data Telemetry] Add index pattern to identify "meow" attacks (elastic#75163) Migrate CSP usage collector to `kibana_usage_collection` plugin (elastic#75536) [Console] Get ES Config from core (elastic#75406) [Uptime] Add delay in telemetry test (elastic#75162) [Lens] Use index pattern service instead saved object client (elastic#74654) Embeddable input (elastic#73033) ...
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Fixes: #72852
This test has been rarely flaky, this change should ensure that we will not see that flakiness anymore, it just add a delay before request is issued.