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

feat: enhance test error UI #24266

Merged
merged 8 commits into from Oct 18, 2022
Merged

feat: enhance test error UI #24266

merged 8 commits into from Oct 18, 2022

Conversation

emilyrohrbough
Copy link
Member

@emilyrohrbough emilyrohrbough commented Oct 14, 2022

User facing changelog

I wouldn't necessarily call these note-worthy changes for the changelog. Addressed in #24266.

  • Enhancements were made to improve the UI around reporter test errors in the Reporter.

User Facing Changing

Review Percy screenshots to see the full impact of changes.

(edit: latest push tweaked these a bit, but these are the general look. See Percy!)

Test Error:
Screen Shot 2022-10-14 at 2 41 47 PM

Recovered In-Test Error (currently only observed when using cy.session())
Screen Shot 2022-10-14 at 2 42 54 PM

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [] Has a PR for user-facing changes been opened in cypress-documentation?
  • [n/a] Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 14, 2022

Thanks for taking the time to open a PR!

{model.event && model.type !== 'system' ? `(${displayName(model)})` : displayName(model)}
</span>
</span>
{!!groupId && model.type === 'system' && model.state === 'failed' && <StateIcon aria-hidden className="failed-indicator" state={model.state}/>}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note for Monday -- this is where the fail-indicator class came from -- verify you don't need to re-add vertical-align: middle

@emilyrohrbough emilyrohrbough self-assigned this Oct 14, 2022
@cypress
Copy link

cypress bot commented Oct 14, 2022



Test summary

45914 0 4489 0Flakiness 6


Run details

Project cypress
Status Passed
Commit 56f4c2a
Started Oct 17, 2022 4:30 PM
Ended Oct 17, 2022 4:47 PM
Duration 17:41 💡
OS Linux Debian - 11.4
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/xhr.cy.js Flakiness
1 src/cy/commands/xhr > abort > aborts xhrs currently in flight
specs_list_latest_runs.cy.ts Flakiness
1 App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs
e2e/origin/config_env.cy.ts Flakiness
1 cy.origin- Cypress.config() > serializable > overwrites different values in secondary, even if the Cypress.config() value does not exist in the primary
e2e/origin/commands/navigation.cy.ts Flakiness
1 cy.origin navigation > #consoleProps > .go()
next.cy.ts Flakiness
1 Working with next-11 > should show compilation errors on src changes
This comment includes only the first 5 flaky tests. See all 6 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

@AtofStryker AtofStryker self-requested a review October 17, 2022 17:17
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.

Is the goal of isRecovered to eventually be used outside of cy.session? Does that mean session validation fails but the rest of the test continues?

@emilyrohrbough
Copy link
Member Author

Is the goal of isRecovered to eventually be used outside of cy.session?

Not necessarily, this is just cleaning up how this is currently implemented. I could see this potentially being used in the future by other "workflow-like" commands but there are no plans ATM that I am away of to create others commands of this style. Think of this as a try-catch-finally or try-catch-try_something_else.

Does that mean session validation fails but the rest of the test continues?

Yes, it does. In the context of cy.session() this only apply in this flow:
restore session -> validate session -> FAIL validation -> recreate session -> validate session -> if pass continue to next command OR if fail, fail test

@AtofStryker AtofStryker self-requested a review October 17, 2022 20:58
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.

wonder if we want to name the isRecovered to something session specific until it is used more generically later? Either way changes look good on my end!

@mjhenkes mjhenkes self-requested a review October 17, 2022 21:02
Comment on lines +152 to +159
if (props.err) {
if (!this.err) {
this.err = new Err(props.err)
} else {
this.err.update(props.err)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to share this logic with the attempt-model?

@emilyrohrbough emilyrohrbough merged commit fa4a19c into develop Oct 18, 2022
@emilyrohrbough emilyrohrbough deleted the reporter-error-ui branch October 18, 2022 13:15
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.

None yet

3 participants