Skip to content
This repository has been archived by the owner on Mar 4, 2024. It is now read-only.

EVEREST-478 Flaky tests #207

Closed
wants to merge 12 commits into from
Closed

Conversation

oksana-grishchenko
Copy link
Contributor

@oksana-grishchenko oksana-grishchenko commented Oct 2, 2023

EVEREST-478 Powered by Pull Request Badge

Fix flaky tests

Problem:
EVEREST-478
In the backend repo there are several flaky tests for monitoring and backup-storages

Solution
Add random naming and cleanup

@oksana-grishchenko oksana-grishchenko marked this pull request as ready for review October 2, 2023 11:16
@oksana-grishchenko oksana-grishchenko requested a review from a user October 2, 2023 11:16
Comment on lines 188 to 199
let list = await response.json()

expect(list.filter((i) => i.name.startsWith(`${namePrefix}${testPrefix}`)).length).toBe(3)

response = await request.delete(`/v1/monitoring-instances/${name}`)
expect(response.ok()).toBeTruthy()

response = await request.get('/v1/monitoring-instances')
expect(response.ok()).toBeTruthy()
list = await response.json()

expect(list.filter((i) => i.name.startsWith(`${namePrefix}${testPrefix}`)).length).toBe(2)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic was in place intentionally.
I discovered we had a bug in the implementation where delete would delete the first instance, not the one selected by name.

I suggest we keep this in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that corner case out! The test is updated to handle it again. Also added a comment to avoid such confusions in the future.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a small comment about a potential issue with the tests.
Feel free to address if you think it's relevant.

})

test('get monitoring instance', async ({ request }) => {
const namePrefix = 'get-'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was in place for the same purpose - making sure we don't return the first entry instead of the correct one.

@oksana-grishchenko
Copy link
Contributor Author

not relevant anymore, fixed by #431

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants