fix(regression): cy.pause() should not be ignored with `cypress run --headed --no-exit` #20877

davidmunechika · 2022-04-01T01:35:04Z

Closes pause() should not be ignored with cypress run --headed --no-exit #20745

User facing changelog

Fixes a regression that was introduced by this PR which prevented cy.pause() from correctly executing when run with cypress run --headed --no-exit.

Additional details

Original implementation PR: #18358

The cause of this regression was the addition of exit to ResolvedConfigOptions in this PR. As a result, when the args got merged with the config, config('exit') would always be true even when --no-exit was passed. When this change was made, we should have removed this line

However, it was decided that exit should be an arg that is not included as part of config. Therefore, this PR removes exit from the ResolvedConfigOptions and correctly passes exit through the options so no conflicts will occur with the config and --no-exit will still work as expected.

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)
[n/a] Has a PR for user-facing changes been opened in cypress-documentation?
Have API changes been updated in the type definitions?
[n/a] Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-04-01T01:35:06Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-04-01T01:52:23Z

Test summary

19391 • 0 • 218 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`42ef9ab`
Started	Apr 11, 2022 3:29 PM
Ended	Apr 11, 2022 3:41 PM
Duration	11:58 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/xhr_spec.js	1
1	... > no status when request isnt forced 404
	cypress/proxy-logging_spec.ts	1
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

lmiller1990 · 2022-04-06T03:40:36Z

packages/config/lib/options.ts

-    name: 'exit',
-    defaultValue: true,
-    validation: validate.isBoolean,
-    canUpdateDuringTestTime: false,


Why does removing this fix the issue? 🤔

Do you mind explaining a little more about this fixed the regression (not entirely clear what part of the regression PR linked broke this in the first place).

Is the bug actually caused by the unique combo of --headed and --no-exit, or is it just related to one of those flags? Looking at the snippet in the related issue:

if (!config('isInteractive') && (!config('browser').isHeaded || config('exit'))) {

Seems it's one or the other (which means we'd probably want two system tests, one with each condition).

@lmiller1990 I've updated this PR description with more details about the regression. The expected behavior is that Cypress will pause if it is in open mode or if it in run mode with --headed and --no-exit both passed as args. Therefore, if cypress is in run mode aka not interactive (!config('isInteractive')) and either --headed or --no-exit was not passed, then we do not want to pause. Hence, the logic in the snippet above

Right, I see! I hope we can have config('mode') in the future - config('mode') === 'run would be way more readable. Nice job hunting down this regression.

lmiller1990

Seems fine - I had my comment clarified, also left one final question but happy to drop the ✔️ since the regression is fixed w/ a test.

lmiller1990 · 2022-04-07T00:54:19Z

system-tests/test/pause_headed_exit_spec.ts

+    spec: 'pause_simple_spec.js',
+    snapshot: false,
+    headed: true,
+    noExit: true,


Somewhat surprised a test can have an exit code of 0 and have noExit as true - seems like these would conflict, but seems this isn't the case?

At least locally this seems to hang forever (on develop, was verifying it fails there)

Yes this test is still a wip- it will hang forever because --no-exit has been passed

Currently working on adding a callback to ExecOptions (something like onSpawn) that yields the ChildProcess so we can listen to the stdout and kill the ChildProcess within the system test itself when not exiting due to options.exit being false gets logged.

Gotcha, can you mark it as draft? I can re-review when you are happy with it - I thought this was ready for review - click "Still in progress - mark as draft" under reviewers (kind of new feature on GH, some ppl haven't seen it).

system-tests/test/pause_headed_exit_spec.ts

emilyrohrbough

🎉 Nice!! Glad you got this figured out. At this point are the existing driver tests around this behavior helpful and/or should those test cases be updated as well?

flotwig

Nice work on the clean fix for this, and on the new test coverage for --no-exit. I was really surprised that we have no coverage considering it's part of our public API.

system-tests/lib/system-tests.ts

system-tests/projects/e2e/cypress/integration/no_pause_spec.js

system-tests/lib/system-tests.ts

system-tests/test/pause_headed_exit_spec.ts

Co-authored-by: Emily Rohrbough <[email protected]>

Co-authored-by: Zach Bloomquist <[email protected]>

system-tests/lib/docker.ts

* 10.0-release: (25 commits) fix: stop running spec when switching specs (#21038) fix: remove asset size warnings and enable nuxt e2e tests (#21074) feat: swap the #__cy_root id selector to become data-cy-root for component mounting (#20951) fix: Doc changes around vue2 (#21066) feat: Add vue2 package from npm/vue/v2 branch (#21026) skip failing test fix: add possible frameworks to object API config (#21056) fix snapshot spacing fix system test fix snapshot update snapshots rename spec files rename files update snapshots release 9.5.4 [skip ci] fix(regression): cy.pause() should not be ignored with `cypress run --headed --no-exit` (#20877) fix: add missing Cypress.Commands.addAll() types (#20894) chore: Don't store video and screenshot artifacts for runs (#20979) chore: Update Chrome (beta) to 101.0.4951.26 (#20957) chore: remove parallelism from test-binary-against-repo jobs (#21004) ...

fix: remove exit from new config

24b3933

davidmunechika added 4 commits April 4, 2022 12:35

add system test

744e327

add system test

bb99f01

fix merge

adba9bc

Merge branch 'develop' into issue-20745-pause-headed-regression

3e9bed6

davidmunechika marked this pull request as ready for review April 5, 2022 22:13

davidmunechika requested review from a team as code owners April 5, 2022 22:13

davidmunechika requested review from jennifer-shehane, flotwig and emilyrohrbough and removed request for a team and jennifer-shehane April 5, 2022 22:13

lmiller1990 reviewed Apr 6, 2022

View reviewed changes

davidmunechika added 2 commits April 6, 2022 13:00

fix --no-exit

2b37016

add system tests

f2b821a

lmiller1990 previously approved these changes Apr 7, 2022

View reviewed changes

davidmunechika marked this pull request as draft April 7, 2022 01:15

add onSpawn callback to system tests

27e6079

davidmunechika dismissed lmiller1990’s stale review via 27e6079 April 7, 2022 16:28

davidmunechika added 2 commits April 7, 2022 12:38

remove outdated test

a212cb1

fix types

1672a4d

davidmunechika self-assigned this Apr 7, 2022

fix tests

3ea8a31

davidmunechika marked this pull request as ready for review April 7, 2022 21:22

emilyrohrbough reviewed Apr 7, 2022

View reviewed changes

system-tests/test/pause_headed_exit_spec.ts Outdated Show resolved Hide resolved

emilyrohrbough previously approved these changes Apr 7, 2022

View reviewed changes

flotwig suggested changes Apr 7, 2022

View reviewed changes

generalize test name

48d106e

Co-authored-by: Emily Rohrbough <[email protected]>

davidmunechika dismissed emilyrohrbough’s stale review via 48d106e April 11, 2022 12:31

davidmunechika and others added 5 commits April 11, 2022 08:31

fix types

ecafaad

Co-authored-by: Zach Bloomquist <[email protected]>

Merge branch 'develop' into issue-20745-pause-headed-regression

e46c82d

fix types

6f47be2

fix tests

334e356

fix return type

9345bb0

emilyrohrbough requested review from flotwig and emilyrohrbough April 11, 2022 14:29

flotwig reviewed Apr 11, 2022

View reviewed changes

system-tests/lib/docker.ts Outdated Show resolved Hide resolved

davidmunechika added 2 commits April 11, 2022 11:17

combine spec files

3ddfe99

update snapshot

42ef9ab

davidmunechika requested a review from flotwig April 11, 2022 15:47

flotwig approved these changes Apr 11, 2022

View reviewed changes

davidmunechika requested a review from lmiller1990 April 11, 2022 17:21

emilyrohrbough approved these changes Apr 11, 2022

View reviewed changes

davidmunechika merged commit 6934c03 into develop Apr 11, 2022

davidmunechika deleted the issue-20745-pause-headed-regression branch April 11, 2022 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(regression): cy.pause() should not be ignored with `cypress run --headed --no-exit` #20877

fix(regression): cy.pause() should not be ignored with `cypress run --headed --no-exit` #20877

davidmunechika commented Apr 1, 2022 •

edited

Loading

cypress-bot bot commented Apr 1, 2022

cypress bot commented Apr 1, 2022 •

edited

Loading

lmiller1990 Apr 6, 2022 •

edited

Loading

davidmunechika Apr 6, 2022

lmiller1990 Apr 7, 2022

lmiller1990 left a comment

lmiller1990 Apr 7, 2022

lmiller1990 Apr 7, 2022

davidmunechika Apr 7, 2022

davidmunechika Apr 7, 2022 •

edited

Loading

lmiller1990 Apr 7, 2022 •

edited

Loading

emilyrohrbough left a comment

flotwig left a comment

fix(regression): cy.pause() should not be ignored with cypress run --headed --no-exit #20877

fix(regression): cy.pause() should not be ignored with cypress run --headed --no-exit #20877

Conversation

davidmunechika commented Apr 1, 2022 • edited Loading

User facing changelog

Additional details

PR Tasks

cypress-bot bot commented Apr 1, 2022

cypress bot commented Apr 1, 2022 • edited Loading

Test summary

Run details

Flakiness

lmiller1990 Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

davidmunechika Apr 6, 2022

Choose a reason for hiding this comment

lmiller1990 Apr 7, 2022

Choose a reason for hiding this comment

lmiller1990 left a comment

Choose a reason for hiding this comment

lmiller1990 Apr 7, 2022

Choose a reason for hiding this comment

lmiller1990 Apr 7, 2022

Choose a reason for hiding this comment

davidmunechika Apr 7, 2022

Choose a reason for hiding this comment

davidmunechika Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

lmiller1990 Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough left a comment

Choose a reason for hiding this comment

flotwig left a comment

Choose a reason for hiding this comment

fix(regression): cy.pause() should not be ignored with `cypress run --headed --no-exit` #20877

fix(regression): cy.pause() should not be ignored with `cypress run --headed --no-exit` #20877

davidmunechika commented Apr 1, 2022 •

edited

Loading

cypress bot commented Apr 1, 2022 •

edited

Loading

lmiller1990 Apr 6, 2022 •

edited

Loading

davidmunechika Apr 7, 2022 •

edited

Loading

lmiller1990 Apr 7, 2022 •

edited

Loading