feat: improve stability when recording by brian-mann · Pull Request #25837 · cypress-io/cypress · GitHub

brian-mann · 2023-02-16T01:33:00Z

Additional details

This PR improves various error messages around interactions with the Cypress cloud and fixes various bugs when recording to the cloud.

Steps to test

This can be tested by recording test runs to the cloud and ensuring there are no problems. The specific edge case error scenarios are thoroughly tested in the tests on this PR.

How has the user experience changed?

n/a

PR Tasks

Have tests been added/updated?
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress · 2023-02-16T02:02:49Z

3 flaky tests on run #44406 ↗︎

0	5116	74	0	3

Details:

added tests for failing after preflight, but valid decryption, always fail on 41...
Project: cypress	Commit: `45c8a33f1c`
Status: Passed	Duration: 13:41 💡
Started: Feb 24, 2023 9:54 PM	Ended: Feb 24, 2023 10:07 PM

cypress/e2e/cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

Test
... > correctly returns currentRetry
... > correctly returns currentRetry
... > correctly returns currentRetry

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

…ent requests

- made some errors clearer - moved up server response errors to be closer to the error, instead of pushed down by list items - renamed old usage of "Desktop App" to "Cypress app" - updated on links referring to the dashboard -> cloud

emilyrohrbough · 2023-02-16T14:16:43Z

  },

-  preflight (preflightInfo) {
+  postPreflight (preflightInfo) {


this was renamed to postPreflight but there are several places preflight is referenced in this file.

Seems like this should still be named preflight

where is it referenced? all of the other methods are verbs, so I just renamed it to make it consistent

On this diff view, there's

where postFlight is called, the preflightOptions are passed in.

in this method, theres preflightInfo and preflightBaseProxy and preflightResult,

resetPreflightResult

yeah i gotcha... but all that makes sense... the verb (action) uses postPreflight to act as a similar pattern to how we do all functions as verbs... whereas the object it takes (is basically the discrete payload/noun)... i see it like this

brian.sendsMessageTo(emily, emilyMsgInfo)

ohh ha, teaches me to read it a bit closer. I was reading it as postFlight

ignore my suggestions

emilyrohrbough · 2023-02-16T14:18:33Z

+        if (response.headers['x-cypress-encrypted'] || params.encrypt === 'always') {
          let decryptedBody

+          // TODO: if body is null/undefined throw a custom error


We should log issues for these TODO or they likely will get forgotten

i will address/remove them all by the time this PR is done... they are notes in the places that I still have work to finish... the PR isn't done yet, I moved it to draft

FWIW, this shouldn't ever be a valid scenario. Either we send a body or we don't send the response header indicating it needs to be processed. Preflight wouldn't have an empty body

preflight wouldnt but thats not the only situation it could occur in

…odes

tgriesser · 2023-02-16T15:04:40Z

+        if (response.headers['x-cypress-encrypted'] || params.encrypt === 'always') {
          let decryptedBody

+          // TODO: if body is null/undefined throw a custom error


FWIW, this shouldn't ever be a valid scenario. Either we send a body or we don't send the response header indicating it needs to be processed. Preflight wouldn't have an empty body

tgriesser · 2023-02-16T15:06:31Z

    }
  },

+  // TODO: i think we can remove this function


I don't think we can without reworking a lot of tests

i basically already did that, haha... but its nbd, just leaving myself a note if I get it for free at the end as a reminder

tgriesser · 2023-02-16T15:08:24Z

+            // we failed decrypting the response...
+
+            // if status code is >=500 or 404 just return body
+            if (statusCode >= 500 || statusCode === 404 || statusCode === 422) {


We don't ever respond to /preflight with a 404 so it doesn't seem like would ever happen in real world use.

i understand that, i'm solving for a different scenario

tgriesser · 2023-02-16T15:15:36Z


          // If we've hit an encrypted payload error case, we need to re-constitute the error
          // as it would happen normally, with the body as an error property
+          // TODO: need to look harder at this to better understand why its necessary


You'll see in the system tests if you remove this if block why it's needed. I believe it's because without it you'll get the original body in the StatusCodeError if you don't

yeah i worked my way through everything else, so that's the last system test to look at to verify that functionality/behavior

mschile · 2023-02-24T15:42:30Z

        resource_class: macos.x86.medium.gen2
        requires:
          - darwin-x64-build
+    - server-unit-tests-cloud-environment:


How come this job isn't run against darwin-arm64 and linux-arm64?

I suppose I could add it. I'm not sure there's much benefit as the logic is platform specific, not architecture specific

mschile · 2023-02-24T16:07:04Z

+        })
+      })
+
+      // TODO: finish implementing this test


Does this still need to be done?

mjhenkes · 2023-02-24T16:29:14Z

+      try {
+        const packageJsonPath = resolvePackagePath(dependency, projectRoot)
+
+        if (packageJsonPath) {
+          packageToJsonMapping[dependency] = packageJsonPath
+          checkProcessTree = true
+        }
+      } catch (error) {
+        errors.push({
+          dependency,
+          name: error.name,
+          message: error.message,
+          stack: error.stack,
+        })
+      }


seems like this callback is duplicated below? can we share a function between them? (or am i missing a difference?)

The difference is setting checkProcessTree to true vs. false. I can see if I can share some of this though.

Updated here with cleaner logic: 802f193 (#25837)

brian-mann · 2023-02-24T16:35:44Z

+        errors.push({
+          dependency,
+          name: error.name,
+          message: error.message,
+          stack: error.stack,
+        })


might want to refactor this into a common function since it does the same thing as lines 121-126 below

Updated here with cleaner logic: 802f193 (#25837)

-throw proper status code error - properly unroll transform errors - add tests

…into fix/preflight

… fail on 412 preflight - updated [W1] warning test case - updated unit test cases names

brian-mann added 2 commits February 15, 2023 20:32

cycle through preflight requests

48700f4

always replace apiUrl

c2b647a

brian-mann added 2 commits February 15, 2023 23:59

fix logic, add additional test cases

c080a11

add tests for default timeout on /preflight

ecbde1e

brian-mann changed the title ~~fix: recordings failing~~ fix: improve stability when recording Feb 16, 2023

brian-mann added 13 commits February 16, 2023 00:55

fix tests, rename function stub

29e6951

optimize only instantiating apiProd once

b2a047c

general cleanup

d01987b

general cleanup and refactoring of test helpers

f5bcc6e

add tests and logic for additional error scenarios

1462c86

cleanup tests, add tests for ensuring encrypt is respected on subsequ…

72d4b16

…ent requests

formatting

8c5817f

bugfix: prevent retryable API errors from retrying one too many times

60984eb

cleaned up various error messages

139eda5

- made some errors clearer - moved up server response errors to be closer to the error, instead of pushed down by list items - renamed old usage of "Desktop App" to "Cypress app" - updated on links referring to the dashboard -> cloud

added a bunch of tests covering all error cases except for F3

792ab36

snapshot cleanup and formatting

503e549

update error messages and html snapshots

3e64537

system test function name

f45cb3b

emilyrohrbough reviewed Feb 16, 2023

View reviewed changes

Comment thread packages/server/lib/cloud/api.ts

mjhenkes reviewed Feb 16, 2023

View reviewed changes

Comment thread packages/errors/src/errors.ts

emilyrohrbough reviewed Feb 16, 2023

View reviewed changes

Comment thread packages/errors/src/errors.ts

brian-mann marked this pull request as draft February 16, 2023 14:49

added tests for F3 and F4 failure states, handle 412 and 422 status c…

8ca8724

…odes

tgriesser reviewed Feb 16, 2023

View reviewed changes

env url resolution

d9142a6

ryanthemanuel and others added 8 commits February 23, 2023 16:52

fix tests

ea075e7

fix test - run ci

6780197

Updating logic for 422

4e7c7bf

updates to CYPRESS_ENV_DEPENDENCIES structure

a9e07e0

run ci

d2bda85

fix test for 422 error

c1b3357

Update CHANGELOG.md

d6681a5

Merge branch 'develop' into fix/preflight

26a0465

ryanthemanuel marked this pull request as ready for review February 24, 2023 15:19

mschile reviewed Feb 24, 2023

View reviewed changes

Comment thread packages/server/lib/cloud/environment.ts Outdated

mschile reviewed Feb 24, 2023

View reviewed changes

Comment thread packages/server/lib/cloud/environment.ts

Update CHANGELOG.md

878aa1e

mschile reviewed Feb 24, 2023

View reviewed changes

Comment thread packages/server/test/support/fixtures/cloud/environment/.gitignore Outdated

mschile reviewed Feb 24, 2023

View reviewed changes

Comment thread packages/server/test/support/fixtures/cloud/environment/test-project/package.json Outdated

mjhenkes reviewed Feb 24, 2023

View reviewed changes

brian-mann commented Feb 24, 2023

View reviewed changes

ryanthemanuel added 2 commits February 24, 2023 11:02

PR comments

802f193

Merge branch 'develop' into fix/preflight

458dc0b

mjhenkes approved these changes Feb 24, 2023

View reviewed changes

mschile approved these changes Feb 24, 2023

View reviewed changes

fix build

d6086a6

ryanthemanuel changed the title ~~fix: improve stability when recording~~ feat: improve stability when recording Feb 24, 2023

brian-mann added 4 commits February 24, 2023 14:34

retry on all 5xx errors

2c5094c

-throw proper status code error - properly unroll transform errors - add tests

Merge branch 'fix/preflight' of https://github.com/cypress-io/cypress …

245400f

…into fix/preflight

condensed down to 4 error cases, updated snapshots

5d38807

added tests for failing after preflight, but valid decryption, always…

45c8a33

… fail on 412 preflight - updated [W1] warning test case - updated unit test cases names

ryanthemanuel merged commit 2a17efa into develop Feb 24, 2023

ryanthemanuel deleted the fix/preflight branch February 24, 2023 22:32

Conversation

brian-mann commented Feb 16, 2023 • edited by ryanthemanuel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Additional details

Steps to test

How has the user experience changed?

PR Tasks

Uh oh!

cypress Bot commented Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

3 flaky tests on run #44406 ↗︎

Uh oh!

emilyrohrbough Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilyrohrbough Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-mann Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgriesser Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgriesser Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

brian-mann commented Feb 16, 2023 •

edited by ryanthemanuel

Loading

cypress Bot commented Feb 16, 2023 •

edited

Loading

emilyrohrbough Feb 16, 2023 •

edited

Loading

emilyrohrbough Feb 16, 2023 •

edited

Loading

brian-mann Feb 16, 2023 •

edited

Loading

tgriesser Feb 16, 2023 •

edited

Loading

tgriesser Feb 16, 2023 •

edited

Loading