meta: resolve e2e flakiness #4077

Murderlon · 2022-08-31T16:14:17Z

What do our flaky tests have in common? There is one common denominator between our stable and flaky tests, it's the fact that we use cy.window().then(({ uppy }) => {}) in combination with cy.wait() to eventually make an assertion. This led me to believe there is a possible race condition going on here.

To solve our flakiness (hopefully):

Make assertions in .then on wait() instead of after it
Only use one cy.window, with the rest of the test in there.

e2e/cypress/integration/dashboard-transloadit.spec.ts

Murderlon · 2022-09-01T13:49:58Z

Taking a look if I can further attempt to unflaky that test

aduh95 · 2022-09-01T13:51:37Z

Taking a look if I can further attempt to unflaky that test

FWIW it would also be OK to leave it marked as FLAKY, if we only have one test flaky instead of three, that's still a win IMO

e2e/cypress/integration/dashboard-transloadit.spec.ts

Murderlon · 2022-09-01T14:05:39Z

Failing on something unrelated?

Error: The log was not found. It may have been deleted based on retention settings.

aduh95 · 2022-09-01T14:10:17Z

It looks like the flakiness is still there...

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Murderlon · 2022-09-01T14:18:11Z

It looks like the flakiness is still there...

If it can't be fixed, I'll probably .skip the test. After this PR I simply want the suite to be 95% stable (AFAIK e2e is impossible to have without flaky test every now and then, especially if you're uploading to live servers).

aduh95 · 2022-09-01T14:20:59Z

It looks like the flakiness is still there...

If it can't be fixed, I'll probably .skip the test. After this PR I simply want the suite to be 95% stable (AFAIK e2e is impossible to have without flaky test every now and then, especially if you're uploading to live servers).

Hum I think we can simply revert f593251, if it's able to run successfully the stress test, let's call it a win for today. If that's not enough, I agree that skipping is appropriate for now.

Murderlon · 2022-09-01T14:28:20Z

I think the stress test is too much, we're running so many tests that I find it likely it will never do all of those successfully, due to what I mentioned:

AFAIK e2e is impossible to have without flaky test every now and then, especially if you're uploading to live servers

I propose to skip that test for now and continue with more certainty

aduh95 · 2022-09-01T14:31:12Z

I don't think the stress test is too much, it's only running 50 times the test suite, we open way more than 50 PRs in one month. If we are able to get it to turn green after running the test suite 50 times, that means we'll get very little false positive errors.

.github/workflows/e2e.yml

e2e/cypress/integration/dashboard-transloadit.spec.ts

* main: ci: add GHA to tryout bundling Uppy with popular bundlers (#4084) @uppy/core: Fix `Restrictor` counts ghost files against `maxNumberOfFiles` (#4078) uppy: add a decoy `Core` export to warn users about the renaming (#4085) meta: run CI when modifying workflow files (#4091) meta: limit the number of unnecessary CI runs (#4086) Update remote-sources.md (#4087) meta: remove all remaining occurrences of `Uppy.Core` (#4082) meta: fix typo in `e2e.yml` Restrict e2e CI runs (#4075) Set default videoConstraints (#4070)

aduh95 · 2022-09-09T13:26:13Z

e2e/cypress/integration/dashboard-transloadit.spec.ts

+  // as this is doing white box testing (testing internal state).
+  // But E2e is more about black box testing, you don’t care about the internals, only the result.
+  // May make more sense to turn this into a unit test.
+  it.skip('should emit one assembly-cancelled event when cancelled', () => {


Can we skip it only in CI? Since it works fine locally.

I don't think cypress offers that option. Only thing I can think of is having some sort of env var, but not sure about that approach

You may also skip at runtime using this.skip(). If a test needs an environment or configuration which cannot be detected beforehand, a runtime skip is appropriate.

I think we can check for 'GITHUB_REF' in process.env. If that turns out to be more difficult, let’s give up on this idea and let’s stick with .skip.

I think the better approach is to refactor this test to a unit test.

Murderlon · 2022-09-09T13:55:59Z

The test suite is now stable. Final reviews welcome.

e2e/cypress.config.mjs

| Package | Version | Package | Version | | ------------------------- | ------- | ------------------------- | ------- | | @uppy/angular | 0.4.2 | @uppy/onedrive | 3.0.1 | | @uppy/audio | 1.0.2 | @uppy/progress-bar | 3.0.1 | | @uppy/aws-s3 | 3.0.2 | @uppy/provider-views | 3.0.1 | | @uppy/aws-s3-multipart | 3.0.2 | @uppy/react | 3.0.2 | | @uppy/box | 2.0.1 | @uppy/redux-dev-tools | 3.0.1 | | @uppy/companion | 4.0.2 | @uppy/remote-sources | 1.0.2 | | @uppy/companion-client | 3.0.2 | @uppy/screen-capture | 3.0.1 | | @uppy/compressor | 1.0.1 | @uppy/status-bar | 3.0.1 | | @uppy/core | 3.0.2 | @uppy/store-default | 3.0.2 | | @uppy/dashboard | 3.1.0 | @uppy/store-redux | 3.0.2 | | @uppy/drag-drop | 3.0.1 | @uppy/svelte | 3.0.1 | | @uppy/drop-target | 2.0.1 | @uppy/thumbnail-generator | 3.0.2 | | @uppy/dropbox | 3.0.1 | @uppy/transloadit | 3.0.2 | | @uppy/facebook | 3.0.1 | @uppy/tus | 3.0.2 | | @uppy/file-input | 3.0.1 | @uppy/unsplash | 3.0.1 | | @uppy/form | 3.0.1 | @uppy/url | 3.0.1 | | @uppy/golden-retriever | 3.0.1 | @uppy/utils | 5.0.2 | | @uppy/google-drive | 3.0.1 | @uppy/vue | 1.0.1 | | @uppy/image-editor | 2.0.1 | @uppy/webcam | 3.2.0 | | @uppy/informer | 3.0.1 | @uppy/xhr-upload | 3.0.2 | | @uppy/instagram | 3.0.1 | @uppy/zoom | 2.0.1 | | @uppy/locales | 3.0.1 | uppy | 3.1.0 | - meta: Fix companion-deploy-yml (Mikael Finstad) - website: fix tag for Activity Feed (Livia Medeiros / #4118) - @uppy/golden-retriever: fix condition to load files from service worker (Merlijn Vos / #4115) - website: remove references to the deleted `disc.html` page (Antoine du Hamel / #4119) - @uppy/locales: Create uz_UZ (Ozodbek1405 / #4114) - @uppy/golden-retriever: Fix endless webcam re-render with Golden Retriever (Merlijn Vos / #4111) - @uppy/image-editor: image-editor: fix controls in small Dashboard (Livia Medeiros / #4113) - website: add “what is Uppy” to the blog post (Artur Paikin) - meta: fix Companion deploy (Antoine du Hamel / #4095) - @uppy/dashboard: add dashboard:show-panel event (Jon-Pierre Sanchez / #4108) - website: Small post fixes (Artur Paikin) - @uppy/companion: Companion throttle progress by time (Mikael Finstad / #4101) - meta: skip a few more unnecessary CI runs (Antoine du Hamel / #4106) - meta: resolve e2e flakiness (Merlijn Vos / #4077) - meta: run linters on almost every PRs (Antoine du Hamel / #4105) - website: 3.0 blog post tweaks (Merlijn Vos / #4104) - meta: Fix linter warnings in 3.0 post (Murderlon) - website: Add 3.0 blog post (Artur Paikin / #4046) - website: fix ESM import in example (Livia Medeiros / #4103) - doc: Update "Dashboard typo" (Laban / #4096) - @uppy/audio,@uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/box,@uppy/companion-client,@uppy/companion,@uppy/compressor,@uppy/core,@uppy/dashboard,@uppy/drag-drop,@uppy/drop-target,@uppy/dropbox,@uppy/facebook,@uppy/file-input,@uppy/form,@uppy/golden-retriever,@uppy/google-drive,@uppy/image-editor,@uppy/informer,@uppy/instagram,@uppy/locales,@uppy/onedrive,@uppy/progress-bar,@uppy/provider-views,@uppy/react,@uppy/redux-dev-tools,@uppy/remote-sources,@uppy/screen-capture,@uppy/status-bar,@uppy/store-default,@uppy/store-redux,@uppy/svelte,@uppy/thumbnail-generator,@uppy/transloadit,@uppy/tus,@uppy/unsplash,@uppy/url,@uppy/utils,@uppy/vue,@uppy/webcam,@uppy/xhr-upload,@uppy/zoom: add missing entries to changelog for individual packages (Antoine du Hamel / #4092) - meta: ci: add GHA to tryout bundling Uppy with popular bundlers (Antoine du Hamel / #4084) - @uppy/core: Fix `Restrictor` counts ghost files against `maxNumberOfFiles` (Andrew McIntee / #4078) - uppy: add a decoy `Core` export to warn users about the renaming (Antoine du Hamel / #4085) - meta: run CI when modifying workflow files (Antoine du Hamel / #4091) - meta: limit the number of unnecessary CI runs (Antoine du Hamel / #4086) - meta: Update remote-sources.md (heocoi / #4087) - uppy: remove all remaining occurrences of `Uppy.Core` (Antoine du Hamel / #4082) - meta: fix typo in `e2e.yml` (Antoine du Hamel) - meta: Restrict e2e CI runs (Merlijn Vos / #4075) - @uppy/webcam: Set default videoConstraints (Artur Paikin / #4070) - @uppy/angular: Fix angular build error (Murderlon) - website: add `Known issues` section on Migration Guide (Antoine du Hamel / #4066) - @uppy/core: fix types (Antoine du Hamel / #4072) - doc: remove use of deprecated `metaFields` option (Antoine du Hamel / #4073)

| Package | Version | Package | Version | | ------------------------- | ------- | ------------------------- | ------- | | @uppy/angular | 0.4.2 | @uppy/onedrive | 3.0.1 | | @uppy/audio | 1.0.2 | @uppy/progress-bar | 3.0.1 | | @uppy/aws-s3 | 3.0.2 | @uppy/provider-views | 3.0.1 | | @uppy/aws-s3-multipart | 3.0.2 | @uppy/react | 3.0.2 | | @uppy/box | 2.0.1 | @uppy/redux-dev-tools | 3.0.1 | | @uppy/companion | 4.0.2 | @uppy/remote-sources | 1.0.2 | | @uppy/companion-client | 3.0.2 | @uppy/screen-capture | 3.0.1 | | @uppy/compressor | 1.0.1 | @uppy/status-bar | 3.0.1 | | @uppy/core | 3.0.2 | @uppy/store-default | 3.0.2 | | @uppy/dashboard | 3.1.0 | @uppy/store-redux | 3.0.2 | | @uppy/drag-drop | 3.0.1 | @uppy/svelte | 3.0.1 | | @uppy/drop-target | 2.0.1 | @uppy/thumbnail-generator | 3.0.2 | | @uppy/dropbox | 3.0.1 | @uppy/transloadit | 3.0.2 | | @uppy/facebook | 3.0.1 | @uppy/tus | 3.0.2 | | @uppy/file-input | 3.0.1 | @uppy/unsplash | 3.0.1 | | @uppy/form | 3.0.1 | @uppy/url | 3.0.1 | | @uppy/golden-retriever | 3.0.1 | @uppy/utils | 5.0.2 | | @uppy/google-drive | 3.0.1 | @uppy/vue | 1.0.1 | | @uppy/image-editor | 2.0.1 | @uppy/webcam | 3.2.0 | | @uppy/informer | 3.0.1 | @uppy/xhr-upload | 3.0.2 | | @uppy/instagram | 3.0.1 | @uppy/zoom | 2.0.1 | | @uppy/locales | 3.0.1 | uppy | 3.1.0 | - meta: Fix companion-deploy-yml (Mikael Finstad) - website: fix tag for Activity Feed (Livia Medeiros / transloadit#4118) - @uppy/golden-retriever: fix condition to load files from service worker (Merlijn Vos / transloadit#4115) - website: remove references to the deleted `disc.html` page (Antoine du Hamel / transloadit#4119) - @uppy/locales: Create uz_UZ (Ozodbek1405 / transloadit#4114) - @uppy/golden-retriever: Fix endless webcam re-render with Golden Retriever (Merlijn Vos / transloadit#4111) - @uppy/image-editor: image-editor: fix controls in small Dashboard (Livia Medeiros / transloadit#4113) - website: add “what is Uppy” to the blog post (Artur Paikin) - meta: fix Companion deploy (Antoine du Hamel / transloadit#4095) - @uppy/dashboard: add dashboard:show-panel event (Jon-Pierre Sanchez / transloadit#4108) - website: Small post fixes (Artur Paikin) - @uppy/companion: Companion throttle progress by time (Mikael Finstad / transloadit#4101) - meta: skip a few more unnecessary CI runs (Antoine du Hamel / transloadit#4106) - meta: resolve e2e flakiness (Merlijn Vos / transloadit#4077) - meta: run linters on almost every PRs (Antoine du Hamel / transloadit#4105) - website: 3.0 blog post tweaks (Merlijn Vos / transloadit#4104) - meta: Fix linter warnings in 3.0 post (Murderlon) - website: Add 3.0 blog post (Artur Paikin / transloadit#4046) - website: fix ESM import in example (Livia Medeiros / transloadit#4103) - doc: Update "Dashboard typo" (Laban / transloadit#4096) - @uppy/audio,@uppy/aws-s3-multipart,@uppy/aws-s3,@uppy/box,@uppy/companion-client,@uppy/companion,@uppy/compressor,@uppy/core,@uppy/dashboard,@uppy/drag-drop,@uppy/drop-target,@uppy/dropbox,@uppy/facebook,@uppy/file-input,@uppy/form,@uppy/golden-retriever,@uppy/google-drive,@uppy/image-editor,@uppy/informer,@uppy/instagram,@uppy/locales,@uppy/onedrive,@uppy/progress-bar,@uppy/provider-views,@uppy/react,@uppy/redux-dev-tools,@uppy/remote-sources,@uppy/screen-capture,@uppy/status-bar,@uppy/store-default,@uppy/store-redux,@uppy/svelte,@uppy/thumbnail-generator,@uppy/transloadit,@uppy/tus,@uppy/unsplash,@uppy/url,@uppy/utils,@uppy/vue,@uppy/webcam,@uppy/xhr-upload,@uppy/zoom: add missing entries to changelog for individual packages (Antoine du Hamel / transloadit#4092) - meta: ci: add GHA to tryout bundling Uppy with popular bundlers (Antoine du Hamel / transloadit#4084) - @uppy/core: Fix `Restrictor` counts ghost files against `maxNumberOfFiles` (Andrew McIntee / transloadit#4078) - uppy: add a decoy `Core` export to warn users about the renaming (Antoine du Hamel / transloadit#4085) - meta: run CI when modifying workflow files (Antoine du Hamel / transloadit#4091) - meta: limit the number of unnecessary CI runs (Antoine du Hamel / transloadit#4086) - meta: Update remote-sources.md (heocoi / transloadit#4087) - uppy: remove all remaining occurrences of `Uppy.Core` (Antoine du Hamel / transloadit#4082) - meta: fix typo in `e2e.yml` (Antoine du Hamel) - meta: Restrict e2e CI runs (Merlijn Vos / transloadit#4075) - @uppy/webcam: Set default videoConstraints (Artur Paikin / transloadit#4070) - @uppy/angular: Fix angular build error (Murderlon) - website: add `Known issues` section on Migration Guide (Antoine du Hamel / transloadit#4066) - @uppy/core: fix types (Antoine du Hamel / transloadit#4072) - doc: remove use of deprecated `metaFields` option (Antoine du Hamel / transloadit#4073)

Resolve e2e flakiness (hopefully)

412e217

Murderlon requested review from mifi, arturi and aduh95 August 31, 2022 16:14

Remove .only and improve other test

bd8a4a3

Murderlon added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Aug 31, 2022

github-actions bot removed the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Aug 31, 2022

aduh95 reviewed Aug 31, 2022

View reviewed changes

e2e/cypress/integration/dashboard-transloadit.spec.ts Outdated Show resolved Hide resolved

Murderlon and others added 2 commits September 1, 2022 14:26

Remove FLAKY

f593251

drop! stress test

bf9956d

Truly unflaky it

3206052

aduh95 reviewed Sep 1, 2022

View reviewed changes

e2e/cypress/integration/dashboard-transloadit.spec.ts Outdated Show resolved Hide resolved

Update e2e/cypress/integration/dashboard-transloadit.spec.ts

9790cea

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

mifi reviewed Sep 1, 2022

View reviewed changes

.github/workflows/e2e.yml Outdated Show resolved Hide resolved

e2e/cypress/integration/dashboard-transloadit.spec.ts Outdated Show resolved Hide resolved

Skip flaky test

a2b2b94

mifi reviewed Sep 2, 2022

View reviewed changes

e2e/cypress/integration/dashboard-transloadit.spec.ts Show resolved Hide resolved

Murderlon added 6 commits September 9, 2022 11:33

Attempt

5d5dedf

Feeling good about this one

3bb6760

fixup! Feeling good about this one

7bde10b

Add default retries

3833344

Skip

008d4e2

aduh95 reviewed Sep 9, 2022

View reviewed changes

Revert to only running suite once

8c69ab6

Murderlon changed the title ~~Resolve e2e flakiness (hopefully)~~ Resolve e2e flakiness Sep 9, 2022

aduh95 changed the title ~~Resolve e2e flakiness~~ meta: skip e2e flaky tests Sep 12, 2022

aduh95 reviewed Sep 12, 2022

View reviewed changes

e2e/cypress.config.mjs Outdated Show resolved Hide resolved

Murderlon commented Sep 12, 2022

View reviewed changes

e2e/cypress.config.mjs Outdated Show resolved Hide resolved

Remove retries

859ef4f

Murderlon changed the title ~~meta: skip e2e flaky tests~~ meta: resolve e2e flakiness Sep 12, 2022

aduh95 approved these changes Sep 13, 2022

View reviewed changes

Murderlon merged commit 2005b18 into main Sep 13, 2022

Murderlon deleted the resolve-e2e-flakiness branch September 13, 2022 12:56

github-actions bot mentioned this pull request Sep 25, 2022

Release: uppy@3.1.0 #4121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

meta: resolve e2e flakiness #4077

meta: resolve e2e flakiness #4077

Murderlon commented Aug 31, 2022

Murderlon commented Sep 1, 2022

aduh95 commented Sep 1, 2022

Murderlon commented Sep 1, 2022

aduh95 commented Sep 1, 2022

Murderlon commented Sep 1, 2022 •

edited

aduh95 commented Sep 1, 2022

Murderlon commented Sep 1, 2022

aduh95 commented Sep 1, 2022

aduh95 Sep 9, 2022

Murderlon Sep 9, 2022

aduh95 Sep 9, 2022

Murderlon Sep 12, 2022

Murderlon commented Sep 9, 2022

meta: resolve e2e flakiness #4077

meta: resolve e2e flakiness #4077

Conversation

Murderlon commented Aug 31, 2022

Murderlon commented Sep 1, 2022

aduh95 commented Sep 1, 2022

Murderlon commented Sep 1, 2022

aduh95 commented Sep 1, 2022

Murderlon commented Sep 1, 2022 • edited

aduh95 commented Sep 1, 2022

Murderlon commented Sep 1, 2022

aduh95 commented Sep 1, 2022

aduh95 Sep 9, 2022

Choose a reason for hiding this comment

Murderlon Sep 9, 2022

Choose a reason for hiding this comment

aduh95 Sep 9, 2022

Choose a reason for hiding this comment

Murderlon Sep 12, 2022

Choose a reason for hiding this comment

Murderlon commented Sep 9, 2022

Murderlon commented Sep 1, 2022 •

edited