Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E2E: reload the page after the log in process completes, but before the test steps take over. #77360

Closed
wants to merge 2 commits into from

Conversation

worldomonation
Copy link
Contributor

Related to #70674.

Proposed Changes

This PR adds a reload step after the log in process, but before the test steps take over, ostensibly to reset the state before the tests begin as a workaround to the page crash. This workaround was suggested in microsoft/playwright#18137.

The original issue describes a situation where the teardown code makes an attempt to screenshot the page, but is unable to do so because the page has already crashed. After some investigations, we find that a comment on microsoft/playwright closely (but not exactly) describes our issue.

During the login process for browsers controlled by Playwright:

  • the log in process is initiated, then the form is submitted.
  • Calypso authenticates and redirects the page multiple times.
  • finally, the page is redirected to the original endpoint they were supposed to land at.
  • test steps take over from here and redirect the page to the intended endpoint.

This all occurs with milliseconds to spare because Playwright is able to act on pages several orders of magnitude faster than humans can. The hypothesis is that this is what's causing the eventual page crash.

The proposed idea is to insert a state-resetting reload call between the third and fourth steps to "reset" the state. This way, the test steps are not attempt to execute code on a log in process that has not yet fully completed.

Testing Instructions

Ensure the following build configurations are passing:

  • Calypso E2E (desktop)
  • Calypso E2E (mobile)
  • Unit Tests
  • Code Style

I've also ran the proposed changes under a "stress test" of 100 iterations or 20 minutes of execution time, whichever comes first. The idea is to run most of the E2E specs in a single build, repeatedly, in the hopes that a problem either arises (in which case the fix has not worked) or does not arise (in which case the fix has high confidence of working).

For this PR, I ran multiple iterations, first with a limited set of specs and then gradually adding more.

First iteration - 15 test steps.
image

Second iteration - 15 test steps.
image

Third iteration - 48 test steps.
image

Fourth iteration - 89 test steps.
image

So far, through all iterations the crashing issue has not once cropped up.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

@github-actions
Copy link

github-actions bot commented May 25, 2023

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@worldomonation worldomonation changed the title E2E: reload the page after the log in process completes, but before the E2E: reload the page after the log in process completes, but before the test steps take over. May 25, 2023
@worldomonation worldomonation force-pushed the try/e2e-playwright-page-crash-70674 branch 2 times, most recently from 8d152ce to 53ab021 Compare May 25, 2023 14:18
- set up loop

packages/calypso-e2e/src/lib/test-account.ts
- remove default param for URL
@worldomonation worldomonation force-pushed the try/e2e-playwright-page-crash-70674 branch from 53ab021 to fd9a8d3 Compare May 26, 2023 06:26
@worldomonation worldomonation deleted the try/e2e-playwright-page-crash-70674 branch October 12, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants