🐛(frontend) improve e2e tests avoiding race condition from mocks #641
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Morendil
force-pushed
the
lbo/fix-e2e-race-conditions
branch
2 times, most recently
from
b9bd04f to
3bcddeb
Compare
Morendil
force-pushed
the
lbo/fix-e2e-race-conditions
branch
from
3bcddeb to
0aa926d
Compare
sdemagny
reviewed
Jan 16, 2025
| @@ -164,6 +166,10 @@ test.describe('Mail domains', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| await page.goto('/'); | |||
| // See comment above | |||
There was a problem hiding this comment.
vaudrait mieux être explicite à chaque fois non ?
Reorder mock setup with page.route so that it takes place before nav.
Morendil
force-pushed
the
lbo/fix-e2e-race-conditions
branch
from
0aa926d to
7c34431
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Several E2E tests were suffering from a race condition problem. The race condition affected setting up mocked API calls with
page.routeand navigation to the front page of Desk. Specifically, the call topage.goto('/')was made before the call topage.route, without caring to synchronize the two. This meant that, at random, we could possibly have the API calls made by the front page complete before the mocks were in place, leading to a test failure.Proposal
This PR aims to eliminate this source of intermittent CI failures, which are always obnoxious and lead to a waste of time (when we investigate why the build failed), of resources (when we have to rerun jobs) and more importantly a loss of confidence in the test suite.
We systematically have the calls to
page.routetake place (synchronously, with anawait) before callingpage.goto('/'). (Unfortunately this means that we cannot make use oftest.beforeEachin these tests, a price I'm happy to pay as these clauses were only a couple lines each).(There is one exception to this "rule",
menu-spec.ts, because the login flow is triggered by a call to the/users/meendpoint, which must return a 401 on the first invocation; setting up the mock before the call topage.goto('/')interferes with that. We must trigger the login process by loading the home page, then prepare the mock, then proceed with login. Unfortunately I'm not able to find a fix for this that works both locally and in CI, so I'm leavingmenu-spec.tsunchanged for now.)In addition, fix some E2E calls that were not making use of
await, to be more in line with all other E2E tests.In addition, lower the default timeout for Playwright tests to 10 seconds, which is plenty.