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: Improve CLOUD_PARALLEL_GROUP_PARAMS_MISMATCH error message #24799

Conversation

estrada9166
Copy link
Member

@estrada9166 estrada9166 commented Nov 23, 2022

User facing changelog

Using the returned payload by the cloud, we should be able to show an error message displaying the mismatch information when CLOUD_PARALLEL_GROUP_PARAMS_MISMATCH is thrown

image

image

Additional details

Steps to test

How has the user experience changed?

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?
  • Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 23, 2022

Thanks for taking the time to open a PR!

@estrada9166 estrada9166 marked this pull request as ready for review November 28, 2022 18:44
@cypress
Copy link

cypress bot commented Nov 28, 2022



Test summary

5194 0 350 0Flakiness 11


Run details

Project cypress
Status Passed
Commit fec70a3
Started Dec 12, 2022 5:06 PM
Ended Dec 12, 2022 5:20 PM
Duration 13:30 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

cypress-in-cypress-run-mode.cy.ts Flakiness
1 Cypress In Cypress - run mode > e2e run mode spec runner header is correct
2 Cypress In Cypress - run mode > hides reporter when NO_COMMAND_LOG is set in run mode
e2e/origin/commands/navigation.cy.ts Flakiness
1 cy.origin navigation > #consoleProps > .go()
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)
This comment includes only the first 5 flaky tests. See all 11 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

Comment on lines 208 to 210
"differentSpecs": [
"cypress/integration/foo_spec.js"
]
Copy link
Member

Choose a reason for hiding this comment

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

Curious how different specs come into play here. If this is starting the same parallel run, shouldn't this be a different set of specs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is possible that in the same way that the params can change, the --spec flag can be set in one machine, running a different set of specs?

CLOUD_PARALLEL_GROUP_PARAMS_MISMATCH: (arg1: {runUrl: string, parameters: any, payload: any }) => {
let params: any = arg1.parameters

if (arg1.payload?.differentParams) {
Copy link
Member

Choose a reason for hiding this comment

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

when would differentParams not exist if there is a mismatch?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is more a safeguard in case the cloud changes were released after the app

params = {}

_.map(arg1.parameters, (value, key) => {
if (key === 'specs') {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (key === 'specs') {
if (key === 'specs' && arg1.payload.differentSpecs) {

do we need to include differentSpecs at all if there is nothing to show the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

do we want to show different error messages depending on the payload? If there are no differentSpecs we show specs with all the specs ran but if there're different specs, we are showing it

Copy link
Member

Choose a reason for hiding this comment

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

If the payload is meant to show the differences on why this errored, I think providing: differentSpecs: [] is a little confusing / doesn't provide much but can go either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to remove that field in case it is empty! you're right! it can be confusing


_.map(arg1.parameters, (value, key) => {
if (key === 'specs') {
params['differentSpecs'] = arg1.payload.differentSpecs ?? []
Copy link
Member

Choose a reason for hiding this comment

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

why is show this attribute as the negative where as the? Are we able to differentiate if a new spec was sent or a specific spec was missing from the payload?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're able to get the different values in the cloud, keeping track of the specs sent by the TR

Copy link
Member

Choose a reason for hiding this comment

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

so diff specs is only tied to the current run that errored right?

How does this look if it sent 1 additional spec vs sent 1 spec less?

Copy link
Member Author

Choose a reason for hiding this comment

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

When there's one spec extra, its gonna show that spec extra, and the same when there's one less, it shows the spec missing

estrada9166 and others added 2 commits November 30, 2022 15:30
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
@emilyrohrbough emilyrohrbough linked an issue Dec 6, 2022 that may be closed by this pull request
@emilyrohrbough
Copy link
Member

@estrada9166 there's a failing system-test.

…ATCH-error' of github.com:cypress-io/cypress into alejandro/feat/improve-CLOUD_PARALLEL_GROUP_PARAMS_MISMATCH-error
…o/feat/improve-CLOUD_PARALLEL_GROUP_PARAMS_MISMATCH-error
@emilyrohrbough emilyrohrbough merged commit 7154fc8 into develop Dec 12, 2022
@emilyrohrbough emilyrohrbough deleted the alejandro/feat/improve-CLOUD_PARALLEL_GROUP_PARAMS_MISMATCH-error branch December 12, 2022 17:47
flotwig pushed a commit that referenced this pull request Dec 13, 2022
)

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
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.

Update PARALLEL_GROUP_PARAMS_MISMATCH error
3 participants