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

fix: Recovered Error Handling and Session Recovered Errors #24511

Merged
merged 67 commits into from Nov 10, 2022

Conversation

emilyrohrbough
Copy link
Member

@emilyrohrbough emilyrohrbough commented Nov 2, 2022

There PR has a few set of changes going on:

  • Correct cy.session() error messaging for both created and restored/created with test coverage
    • updated messaging
    • proper code frames
    • handles the following failed validation scenarios: yield false, rejects, resolve false, thrown cy command, thrown err
    • REMOVED support for return false
  • Fix Session Instrument Panel Tag color
  • Enhance cy.session() console props output to include sessionStorage details and add step information to help with debuggin
  • Fix verifying upcoming assertions log handling (and associated tests) - this was need to prevent duplicate error logging in recreated sessions
  • Update command queue to be recursive to allow queue failures to always be collected when onQueueFailed (previously onCommandFailed) to ensure all failures can be collected on a 2nd session validation failure and/or when multiple session commands are used in a test.
  • Correctly end log groups when on:fail handlers to make additional assertions visible

User facing changelog

TO DO

Steps to test

It would be solid to pull this branch down and run various tests yourself to verify changes to logs and to ensure we don't hanging logs and that things appear to be correct.

How has the user experience changed?

Will comment screenshots here & throughout the code.

PR Tasks

emilyrohrbough and others added 30 commits September 16, 2022 11:07
# Conflicts:
#	packages/app/cypress/e2e/runner/sessions.ui.cy.ts
#	packages/driver/cypress/e2e/commands/sessions/sessions.cy.js
#	packages/driver/src/cy/commands/sessions/index.ts
#	packages/driver/src/cypress/command_queue.ts
#	packages/driver/src/cypress/state.ts
# Conflicts:
#	packages/driver/cypress/e2e/commands/sessions/sessions.cy.js
#	packages/driver/src/cypress/error_utils.ts
…-in-test-errors

# Conflicts:
#	packages/driver/cypress/e2e/commands/sessions/sessions.cy.js
#	packages/driver/src/cy/commands/sessions/index.ts
#	packages/driver/src/cypress/error_utils.ts
#	packages/driver/src/cypress/log.ts
#	packages/reporter/cypress/e2e/commands.cy.ts
#	packages/reporter/src/attempts/attempt-model.ts
#	packages/reporter/src/attempts/attempts.tsx
#	packages/reporter/src/commands/command-model.ts
#	packages/reporter/src/commands/command.tsx
#	packages/reporter/src/commands/commands.scss
#	packages/reporter/src/errors/err-model.ts
#	packages/reporter/src/errors/errors.scss
#	packages/reporter/src/errors/test-error.tsx
#	packages/reporter/src/lib/events.ts
#	packages/reporter/src/lib/variables.scss
…-in-test-errors

# Conflicts:
#	packages/reporter/src/commands/commands.scss
#	packages/reporter/src/errors/errors.scss
Copy link
Contributor

@AtofStryker AtofStryker left a comment

Choose a reason for hiding this comment

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

gave the app and driver tests a run and things didn't look to hang and appeared correctly. Just a few questions/suggestions on my end

@@ -368,7 +368,7 @@ describe('src/cy/commands/assertions', () => {

cy.on('log:added', (attrs, log) => {
if (log.get('name') === 'assert') {
logs.push(log)
logs?.push(log)
Copy link
Contributor

Choose a reason for hiding this comment

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

should we drop a comment there to explain this behavior incase someone runs into it again and thinks the optional chainer isn't necessary?


expect(this.logs[4].get('name')).to.eq('assert')
expect(this.logs[4].get('state')).to.eq('failed')
expect(this.logs[4].get('error').name).to.eq('AssertionError')
expect(this.logs[4].get('error').name).to.eq('CypressError')
expect(this.logs[4].get('error')).to.eq(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the assertion fails, I would expect an AssertionError to be thrown here and not a CypressError. What was the rason that changed? Or is it that the argument into the assertion is bad?

packages/driver/src/cy/commands/sessions/utils.ts Outdated Show resolved Hide resolved
@AtofStryker
Copy link
Contributor

actually have a few failures in the app tests that I scrolled past. Is that expected currently?

@AtofStryker
Copy link
Contributor

never mind I clicked something out of the frame causing a UI issue 😬

@cypress
Copy link

cypress bot commented Nov 4, 2022



Test summary

46495 6 4309 0Flakiness 18


Run details

Project cypress
Status Failed
Commit 919dd4a
Started Nov 8, 2022 7:03 PM
Ended Nov 8, 2022 7:22 PM
Duration 18:45 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️


Failures

cypress/e2e/commands/navigation.cy.js Failed
1 src/cy/commands/navigation > #visit > should eventually fail on assertion despite redirects
2 src/cy/commands/navigation > #page loading > emits 'page:loading' before and after initial visit
3 src/cy/commands/navigation > #page loading > emits during page navigation
4 src/cy/commands/navigation > #page loading > logs during page navigation
5 src/cy/commands/navigation > #page loading > logs during form submission and yields stale element
6 src/cy/commands/navigation > #form sub > logs 'form sub'

Flakiness

runner/reporter-ct-mount-hover.cy.ts Flakiness
1 CT Mount angular-14 > While hovering on Mount(), shows component on AUT for angular-14
e2e/origin/cookie_behavior.cy.ts Flakiness
1 ... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
2 ... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
3 ... > same site / cross origin > XMLHttpRequest > sets cookie on same-site request if withCredentials is true, and attaches to same-site request if withCredentials is true
4 ... > same site / cross origin > fetch > sets same-site cookies if "include" credentials option is specified from request, but does not attach same-site cookies to request by default (same-origin)
This comment includes only the first 5 flaky tests. See all 18 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

# Conflicts:
#	tooling/v8-snapshot/cache/dev-darwin/snapshot-meta.cache.json
@emilyrohrbough emilyrohrbough changed the title Fix Recovered Error Handling and Session Recovered Errors fix: Recovered Error Handling and Session Recovered Errors Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants