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(regression): cy.pause() should not be ignored with cypress run --headed --no-exit #20877

Merged
merged 19 commits into from
Apr 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/config/__snapshots__/index_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ exports['src/index .getDefaultValues returns list of public config keys 1'] = {
"e2e": {},
"env": {},
"execTimeout": 60000,
"exit": true,
"experimentalFetchPolyfill": false,
"experimentalInteractiveRunEvents": false,
"experimentalSessionSupport": false,
Expand Down Expand Up @@ -100,7 +99,6 @@ exports['src/index .getPublicConfigKeys returns list of public config keys 1'] =
"e2e",
"env",
"execTimeout",
"exit",
"experimentalFetchPolyfill",
"experimentalInteractiveRunEvents",
"experimentalSessionSupport",
Expand Down
5 changes: 0 additions & 5 deletions packages/config/lib/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,6 @@ const resolvedOptions: Array<ResolvedConfigOption> = [
defaultValue: 60000,
validation: validate.isNumber,
canUpdateDuringTestTime: true,
}, {
name: 'exit',
defaultValue: true,
validation: validate.isBoolean,
canUpdateDuringTestTime: false,
Copy link
Contributor

@lmiller1990 lmiller1990 Apr 6, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}, {
name: 'experimentalFetchPolyfill',
defaultValue: false,
Expand Down
3 changes: 2 additions & 1 deletion packages/server/lib/modes/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -1459,7 +1459,7 @@ module.exports = {
compressedVideoName: videoRecordProps.compressedVideoName,
endVideoCapture: videoRecordProps.endVideoCapture,
startedVideoCapture: videoRecordProps.startedVideoCapture,
exit: config.exit,
exit: options.exit,
videoCompression: options.videoCompression,
videoUploadOnPasses: options.videoUploadOnPasses,
quiet: options.quiet,
Expand Down Expand Up @@ -1618,6 +1618,7 @@ module.exports = {
quiet: options.quiet,
outputPath: options.outputPath,
testingType: options.testingType,
exit: options.exit,
})
.tap((runSpecs) => {
if (!options.quiet) {
Expand Down
2 changes: 0 additions & 2 deletions packages/server/test/unit/config_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1482,7 +1482,6 @@ describe('lib/config', () => {
e2e: { from: 'default', value: {} },
env: {},
execTimeout: { value: 60000, from: 'default' },
exit: { value: true, from: 'default' },
experimentalFetchPolyfill: { value: false, from: 'default' },
experimentalInteractiveRunEvents: { value: false, from: 'default' },
experimentalSourceRewriting: { value: false, from: 'default' },
Expand Down Expand Up @@ -1572,7 +1571,6 @@ describe('lib/config', () => {
downloadsFolder: { value: 'cypress/downloads', from: 'default' },
e2e: { from: 'default', value: {} },
execTimeout: { value: 60000, from: 'default' },
exit: { value: true, from: 'default' },
experimentalFetchPolyfill: { value: false, from: 'default' },
experimentalInteractiveRunEvents: { value: false, from: 'default' },
experimentalSourceRewriting: { value: false, from: 'default' },
Expand Down
15 changes: 0 additions & 15 deletions packages/server/test/unit/modes/run_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -810,21 +810,6 @@ describe('lib/modes/run', () => {
})
})

it('passes exit from config to waitForTestsToFinishRunning', function () {
this.projectInstance.getConfig.restore()
sinon.stub(this.projectInstance, 'getConfig').resolves({
proxyUrl: 'http://localhost:12345',
exit: false,
})

return runMode.run()
.then(() => {
expect(runMode.waitForTestsToFinishRunning).to.be.calledWithMatch({
exit: false,
})
})
})

it('passes headed to openProject.launch', () => {
const browser = { name: 'electron', family: 'chromium' }

Expand Down
179 changes: 179 additions & 0 deletions system-tests/__snapshots__/pause_headed_exit_spec.ts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
exports['cy.pause() in run mode / pauses with --headed and --no-exit'] = `

====================================================================================================

(Run Starting)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Cypress: 1.2.3 │
│ Browser: FooBrowser 88 │
│ Specs: 1 found (pause_spec.js) │
│ Searched: cypress/integration/pause_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────

Running: pause_spec.js (1 of 1)


cy.pause()
✓ pauses


1 passing

not exiting due to options.exit being false

`

exports['cy.pause() in run mode / does not pause if headless'] = `

====================================================================================================

(Run Starting)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Cypress: 1.2.3 │
│ Browser: FooBrowser 88 │
│ Specs: 1 found (pause_spec.js) │
│ Searched: cypress/integration/pause_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────

Running: pause_spec.js (1 of 1)


cy.pause()
✓ pauses


1 passing

not exiting due to options.exit being false

`

exports['cy.pause() in run mode / does not pause without --no-exit'] = `

====================================================================================================

(Run Starting)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Cypress: 1.2.3 │
│ Browser: FooBrowser 88 │
│ Specs: 1 found (pause_spec.js) │
│ Searched: cypress/integration/pause_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────

Running: pause_spec.js (1 of 1)


cy.pause()
✓ pauses


1 passing


(Results)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 1 │
│ Passing: 1 │
│ Failing: 0 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Duration: X seconds │
│ Spec Ran: pause_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


(Video)

- Started processing: Compressing to 32 CRF
- Finished processing: /XXX/XXX/XXX/cypress/videos/pause_spec.js.mp4 (X second)


====================================================================================================

(Run Finished)


Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ pause_spec.js XX:XX 1 1 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✔ All specs passed! XX:XX 1 1 - - -


`

exports['cy.pause() in run mode / does not pause without --headed and --no-exit'] = `

====================================================================================================

(Run Starting)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Cypress: 1.2.3 │
│ Browser: FooBrowser 88 │
│ Specs: 1 found (pause_spec.js) │
│ Searched: cypress/integration/pause_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────

Running: pause_spec.js (1 of 1)


cy.pause()
✓ pauses


1 passing


(Results)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 1 │
│ Passing: 1 │
│ Failing: 0 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Duration: X seconds │
│ Spec Ran: pause_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


(Video)

- Started processing: Compressing to 32 CRF
- Finished processing: /XXX/XXX/XXX/cypress/videos/pause_spec.js.mp4 (X second)


====================================================================================================

(Run Finished)


Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ pause_spec.js XX:XX 1 1 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✔ All specs passed! XX:XX 1 1 - - -


`
4 changes: 4 additions & 0 deletions system-tests/lib/docker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ class DockerProcess extends EventEmitter implements SpawnerResult {
},
)
}

kill (): boolean {
throw new Error('.kill not implemented for DockerProcess.')
}
}

const checkBuiltBinary = async () => {
Expand Down
9 changes: 8 additions & 1 deletion system-tests/lib/system-tests.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const snapshot = require('snap-shot-it')

import type { SpawnOptions } from 'child_process'
import type { SpawnOptions, ChildProcess } from 'child_process'
import stream from 'stream'
import { expect } from './spec_helper'
import { dockerSpawner } from './docker'
Expand Down Expand Up @@ -135,6 +135,10 @@ type ExecOptions = {
* Pass a function to assert on and/or modify the stdout before snapshotting.
*/
onStdout?: (stdout: string) => string | void
/**
* Pass a function to receive the spawned process as an argument.
*/
onSpawn?: (sp: SpawnerResult) => void
/**
* User-supplied snapshot title. If unset, one will be autogenerated from the suite name.
*/
Expand Down Expand Up @@ -265,6 +269,7 @@ export type SpawnerResult = {
stderr: stream.Readable
on(event: 'error', cb: (err: Error) => void): void
on(event: 'exit', cb: (exitCode: number) => void): void
kill: ChildProcess['kill']
}

const cpSpawner: Spawner = (cmd, args, env, options) => {
Expand Down Expand Up @@ -1055,6 +1060,8 @@ const systemTests = {
const spawnerFn: Spawner = options.dockerImage ? dockerSpawner : cpSpawner
const sp: SpawnerResult = await spawnerFn(cmd, args, env, options)

options.onSpawn && options.onSpawn(sp)

const ColorOutput = function () {
const colorOutput = new stream.Transform()

Expand Down
30 changes: 30 additions & 0 deletions system-tests/projects/e2e/cypress/integration/pause_spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
describe('cy.pause()', () => {
it('pauses', () => {
let didPause = false

cy.visit('https://example.cypress.io')

cy.once('paused', (name) => {
cy.once('paused', (name) => {
didPause = true

// resume the rest of the commands so this
// test ends
Cypress.emit('resume:all')
})

Cypress.emit('resume:next')
})

cy.pause().wrap({}).should('deep.eq', {}).then(function () {
if (Cypress.env('SHOULD_PAUSE')) {
expect(didPause).to.be.true

// should no longer have onPaused
expect(cy.state('onPaused')).to.be.null
} else {
expect(didPause).to.be.false
}
})
})
})
Loading