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

Improve E2E Tests & Stability #12439

Merged
merged 133 commits into from
Oct 21, 2022
Merged

Improve E2E Tests & Stability #12439

merged 133 commits into from
Oct 21, 2022

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Oct 7, 2022

Context

Summary

Relevant Technical Choices

  • Remove outdated focus-visible polyfill
    Browser support has much improved this year, with Safari adding support in version 15.4 in March 2022.
    Safari 15.4 ships with the iOS and iPadOS 15.4 and macOS 12.3.
  • Adjusted media recording to lazy-load selfie segmentation module (because of WASM)
    See Media Recording: Load @mediapipe/selfie_segmentation async #12383
  • Stopped prefetching colorthief script
  • Added retryTimes to every e2e test
  • Added retryTimes also to unit tests when using AMP validation
  • Rewrote a ton of e2e tests to make them more robust
  • Updated Puppeteer to latest version, patching dependencies where necessary
  • Fixes a bug in packages/element-library/src/media/util.js where undefined was being loaded as a CSS background image when there was no poster, causing some tests to fail and logs to be polluted
  • Un-skips all previously skipped e2e tests

To-do

User-facing changes

Testing Instructions

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

Reviews

Does this PR have a security-related impact?

No

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

No

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #12419
Fixes #7107
Fixes #12383
Fixes #12025
Fixes #11994
Fixes #11976
Fixes #11361
Fixes #11362
Fixes #11992
Fixes #11991
Fixes #11981
Fixes #11990
Fixes #6879
Fixes #11978
Fixes #9636
Fixes #11990
Fixes #11931
Fixes #11993
Fixes #9985
Fixes #11975
Fixes #6237
Fixes #12023
Fixes #11989
Fixes #11930
Fixes #9152
Fixes #11977
Fixes #7481
Fixes #7107
Fixes #11959
Fixes #8232
Fixes #9619
Fixes #11970

Browser support has much improved this year, with Safari adding support in version 15.4 in March 2022.

Safari 15.4 ships with the iOS and iPadOS 15.4 and macOS 12.3.
@spacedmonkey

This comment was marked as resolved.

@timarney

This comment was marked as resolved.

Copy link
Contributor

@timarney timarney left a comment

Choose a reason for hiding this comment

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

Approving

Would like to see a small summary per

#12439 (comment)

But that's not a blocker for merging.

@swissspidy
Copy link
Collaborator Author

It would be nice to see a brief summary of what you uncovered for future reference. Nothing exhaustive but something to surface some causes of the tests failing.

It basically boils down to many rounds of manually running and debugging tests.

What I usually did is to fix a broken/flaky test:

  • Remove the jest.retryTimes() config (just so there's only a single run)
  • Add await page.screenshot(..) calls after every single step to capture the page's state
  • Run only a single test, multiple times, observing the error messages and screenshots and identifying issues

Besides obvious quirks like missing awaits and incorrect selectors, most issues were some sort of race condition, i.e. trying to click on something when things are not fully loaded yet.

After a while I had all tests running mostly reliably locally, but especially on CI there was still some flakiness, for which jest.retryTimes basically acts as a last resort to catch these.

Specifically around things adding the status plugin files etc... having tried to debug these before I understand based on what I was seeing but would be nice to surface.

Screen Shot 2022-10-19 at 7 02 14 AM

For the status check tests in particular, I found request interception to not be super reliable, sometimes even getting intermittent errors saying that request interception was not enabled (when in fact it definitely was).
Using these plugins to mimic the behavior was more reliable and has the added benefit that you can also use them for manual testing as well.

@swissspidy
Copy link
Collaborator Author

On Percy, @miina pointed out some missing focus rings in some of the screenshots.

While I can reproduce some of these situations, I can't reproduce all of them. Overall I do think that the focus is working as expected.

:focus-visible means the browser determines via heuristics whether the focus should be made evident on the element or not.

I think the polyfill was just adding the focus more often than Chrome does.

@miina
Copy link
Contributor

miina commented Oct 20, 2022

While I can reproduce some of these situations, I can't reproduce all of them. Overall I do think that the focus is working as expected.

I'm also not able to reproduce the remaining differences currently in Percy when testing -- in the editor, the focus is showed correctly. In Percy screenshots, it's missing in some places for some reason, e.g. here. Any thoughts why it would differ in Percy?

In the editor, the focus seems to work correctly 👍

@miina
Copy link
Contributor

miina commented Oct 20, 2022

Is this Karma failure related?

Copy link
Contributor

@miina miina left a comment

Choose a reason for hiding this comment

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

Nice work, closes quite a lot of issues 😄

@swissspidy
Copy link
Collaborator Author

In Percy screenshots, it's missing in some places for some reason, e.g. here. Any thoughts why it would differ in Percy?

I'd assume this is simply because of how Percy works. It takes the page's current HTML + CSS, loads that in a browser and takes screenshots. This way, browser-supplied focus-visible styles get lost. Again, because of the browser's heuristics — the element is focus, but because of how the focus happens in this process, the browser decides not to show the focus ring.

Is this Karma failure related?

Looks like a flake. Restarted the test and it passed now.

@swissspidy swissspidy merged commit 5756117 into main Oct 21, 2022
@swissspidy swissspidy deleted the fix/e2e-tests-stability branch October 21, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: E2E Tests /packages/e2e-tests Type: Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
5 participants