tests(e2e): improve login util stability #20223
Closed
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
innerdvations
force-pushed
the
tests/retry-flaky-login
branch
from
8254e73
to
5adf218
Compare
innerdvations
added
source: tooling
|
Size Change: 0 B Total Size: 2.58 MB ℹ️ View Unchanged
|
innerdvations
changed the title
innerdvations
commented
Apr 29, 2024
| page = await context.newPage(); | ||
| await page.goto('/admin'); | ||
| await expect(page).toHaveTitle('Strapi Admin'); | ||
| const nonPersistentPage = await context.newPage(); |
There was a problem hiding this comment.
Re-using the page object was rarely but sometimes causing a "page already closed" when attempting to navigate after
| await expect(nameCell).toBeVisible(); | ||
|
|
||
| // Locate the parent of nameCell and then search within it for the timestamp | ||
| const parentRow = nameCell.locator('xpath=..'); |
There was a problem hiding this comment.
adds check present on transfer tokens that was missing from api tokens
| @@ -96,8 +96,11 @@ test.describe('Sign Up', () => { | |||
| test('a user should be able to signup when the strapi instance starts fresh', async ({ | |||
| page, | |||
| }) => { | |||
| await page.getByRole('button', { name: "Let's start" }).click(); | |||
| await Promise.all([ | |||
There was a problem hiding this comment.
Using Promise.all with waitForLoadState and a click action works better to give clicks the time necessary to load the next page
Comment on lines
+66
to
+68
| // Locate the parent of nameCell and then search within it for the timestamp | ||
| const parentRow = nameCell.locator('xpath=..'); | ||
| await expect(parentRow).toContainText(/\d+ (second|minute)s? ago/); |
There was a problem hiding this comment.
Since we aren't clearing the db after each run anymore and have multiple tokens, we need to be more specific with what we're looking for
|
So far this has run four times locally and twice on the CI with only the content history failing, so I think this helps a lot compared to before |
innerdvations
commented
Apr 29, 2024
| @@ -41,9 +43,17 @@ const ALLOWED_CONTENT_TYPES = [ | |||
| const ADMIN_EMAIL_ADDRESS = '[email protected]'; | |||
| const ADMIN_PASSWORD = 'Testing123!'; | |||
|
|
|||
| const TITLE_LOGIN = 'Strapi Admin'; | |||
| const TITLE_HOME = 'Homepage | Strapi'; | |||
There was a problem hiding this comment.
This is so we stop having so many conflicts merging develop <-> v5/main
jhoward1994
reviewed
Apr 29, 2024
| @@ -96,8 +96,11 @@ test.describe('Sign Up', () => { | |||
| test('a user should be able to signup when the strapi instance starts fresh', async ({ | |||
| page, | |||
| }) => { | |||
| await page.getByRole('button', { name: "Let's start" }).click(); | |||
| await Promise.all([ | |||
| page.waitForLoadState('networkidle'), | |||
There was a problem hiding this comment.
Maybe we should make networkidle a const somewhere, it gets used quite a bit
There was a problem hiding this comment.
Since it's a typed value for waitForLoadState I'll leave it for now, but I actually think the next time I'm in here messing with things I'm going to add a util for navigating + waiting so you can just call one line instead of 2+, nobody should have to remember to do this to write a stable test
|
I haven't seen that content releases FE failure before. Maybe we could skip the test here if it keep failing to get the E2E CI to run? |
It just has to be run again sometimes, someone is working on that too and then we should have stable tests again everywhere! |
|
login.spec.ts just failed 😭 I'll take one more look at it, but at least it's the only e2e test that has failed in the last 10 runs or so. |
can you share a trace? |
Just the one in CI: https://github.com/strapi/strapi/actions/runs/8880591655/job/24382167567?pr=20223 Most of the fixing I've done in there is the same flaky login in general, compounded by creating multiple browsers. Whatever is going wrong here is probably another case I missed where we're not waiting for something. |
|
I've looked at your trace file, the issue is a bug in the FE which imo is EDIT: DX-1397 |
joshuaellis
reviewed
May 3, 2024
| @@ -26,12 +26,19 @@ const createTransferToken = async (page, tokenName, duration, type) => { | |||
| }; | |||
|
|
|||
| test.describe('Transfer Tokens', () => { | |||
| test.beforeEach(async ({ page }) => { | |||
| let page: Page; | |||
There was a problem hiding this comment.
Is this a pattern we want to promote? you effectively make the last test dependent on the previous because you're not starting with a "fresh" slate every time.
I'd be cautious about promoting this pattern as I don't think it's something we want to get into the habit of doing in the long term?
There was a problem hiding this comment.
Since we aren't clearing the db after each run anymore and have multiple tokens, we need to be more specific with what we're looking for
My point exactly.
There was a problem hiding this comment.
Unfortunately I think the bad practice is the core of the fix here. I will take a look and see if there's an alternative I can find, but otherwise I may just add a warning not to repeat that pattern in other tests.
|
I think I can do a much more minimalist PR, this one had too many guesses in it, so I'll close this for now and come back with a cleaner version. |
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.
What does it do?
Why is it needed?
For some reason, almost consistently near the end of a test run, the login will fail to load the page and gets an infinitely spinning loader instead. We really need to fix that, but until we can solve it, this will get our e2e CI working again since right now it's nearly impossible to get e2e to pass.
How to test it?
CI e2e tests should pass
Related issue(s)/PR(s)
Once I'm sure this passes consistently on the CI, I'll cherry pick this and do the same thing on develop
DX-1393
v4 version of this PR