From 707b17a25a74b186ef036697194c6e9d6eb9a6d1 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Thu, 18 Nov 2021 17:49:58 -0600 Subject: [PATCH 01/25] Adding timeout option to writeFile --- cli/types/cypress.d.ts | 1 + .../integration/commands/files_spec.js | 121 ++++++++++++++++++ packages/driver/src/cy/commands/files.ts | 26 ++-- packages/server/lib/files.js | 31 +++-- 4 files changed, 155 insertions(+), 24 deletions(-) diff --git a/cli/types/cypress.d.ts b/cli/types/cypress.d.ts index 66e54176f63..defe6e581eb 100644 --- a/cli/types/cypress.d.ts +++ b/cli/types/cypress.d.ts @@ -3249,6 +3249,7 @@ declare namespace Cypress { interface WriteFileOptions extends Loggable { flag: string encoding: Encodings + timeout: number } // Kind of onerous, but has a nice auto-complete. diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index e5ee3834b11..a6bf3bfafc4 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -1,5 +1,6 @@ const { assertLogLength } = require('../../support/utils') const { stripIndent } = require('common-tags') +const Promise = require('bluebird') const { _ } = Cypress const okResponse = { @@ -336,6 +337,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'w', + timeout: 4000, }, ) }) @@ -352,6 +354,7 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', + timeout: 4000, }, ) }) @@ -369,6 +372,7 @@ describe('src/cy/commands/files', () => { { encoding: null, flag: 'w', + timeout: 4000, }, ) }) @@ -385,6 +389,24 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', + timeout: 4000, + }, + ) + }) + }) + + it('can take timeout as part of options', () => { + Cypress.backend.resolves(okResponse) + + cy.writeFile('foo.txt', 'contents', { timeout: 12345 }).then(() => { + expect(Cypress.backend).to.be.calledWith( + 'write:file', + 'foo.txt', + 'contents', + { + encoding: 'utf8', + flag: 'w', + timeout: 12345, }, ) }) @@ -436,6 +458,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'a+', + timeout: 4000, }, ) }) @@ -601,6 +624,104 @@ describe('src/cy/commands/files', () => { cy.writeFile('foo.txt', 'contents') }) + + it('throws when the driver timeout expires', function (done) { + const err = new Error('TimeoutError') + + err.name = 'TimeoutError' + + Cypress.backend.rejects(err) + + cy.on('fail', (err) => { + const { lastLog } = this + + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent` + \`cy.writeFile("foo.txt")\` timed out after waiting \`1000ms\`. + `) + + expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') + + done() + }) + + cy.writeFile('foo.txt', 'contents', { timeout: 1000 }) + }) + + it('throws when the server write is aborted', function (done) { + const err = new Error() + + err.aborted = true + + Cypress.backend.rejects(err) + + cy.on('fail', (err) => { + const { lastLog } = this + + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent` + \`cy.writeFile("foo.txt")\` timed out after waiting \`1000ms\`. + `) + + expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') + + done() + }) + + cy.writeFile('foo.txt', 'contents', { timeout: 1000 }) + }) }) }) + + describe('#writeFile-error', () => { + const bigString = JSON.stringify(Cypress._.times((10 ** 6), () => 'hehehehe'), null, 2) + const tooBigString = JSON.stringify(Cypress._.times(15000000, '😈'), null, 2) // 72MB + // afterEach(() => { + // return new Promise((resolve) => { + // setTimeout(() => { + // resolve(cy.writeFile('./fixtures/my-long-file.txt', bigString)) + // }, 5000) + // }) + // }) + + // afterEach(() => { + // return cy.writeFile('./fixtures/my-long-file.txt', bigString) + // }) + + // afterEach(() => { + // cy.writeFile('./fixtures/my-long-file.txt', bigString) + // }) + + // afterEach(() => { + // return new Promise((resolve) => { + // setTimeout(() => { + // resolve(cy.writeFile('./fixtures/my-long-file.txt', Cypress._.times(9000000, '😈'), { timeout: 1000 })) + // }, 5000) + // }) + // }) + + // afterEach(() => { + // cy.writeFile('./fixtures/my-long-file.txt', bigString, { timeout: 1 }) + // }) + + // it('writes until timeout', () => { + // cy.visit('/fixtures/dom.html') + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + // }) + }) }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 276ccfa766d..955697896ce 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -93,7 +93,7 @@ export default (Commands, Cypress, cy) => { return verifyAssertions() }, - writeFile (fileName, contents, encoding, options = {}) { + async writeFile (fileName, contents, encoding, options = {}) { let userOptions = options if (_.isObject(encoding)) { @@ -110,6 +110,7 @@ export default (Commands, Cypress, cy) => { encoding: encoding === undefined ? 'utf8' : encoding, flag: userOptions.flag ? userOptions.flag : 'w', log: true, + timeout: 4000, }) const consoleProps = {} @@ -142,24 +143,21 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } - return Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag'])) - .then(({ contents, filePath }) => { - consoleProps['File Path'] = filePath - consoleProps['Contents'] = contents + try { + await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag', 'timeout'])).timeout(options.timeout) + } catch (err) { + if (err.aborted || err.name === 'TimeoutError') { + return $errUtils.throwErrByPath('files.timed_out', { + onFail: options._log, + args: { cmd: 'writeFile', file: fileName, timeout: options.timeout }, + }) + } - return null - }).catch(Promise.TimeoutError, () => { - return $errUtils.throwErrByPath('files.timed_out', { - onFail: options._log, - args: { cmd: 'writeFile', file: fileName, timeout: options.timeout }, - }) - }) - .catch((err) => { return $errUtils.throwErrByPath('files.unexpected_error', { onFail: options._log, args: { cmd: 'writeFile', action: 'write', file: fileName, filePath: err.filePath, error: err.message }, }) - }) + } }, }) } diff --git a/packages/server/lib/files.js b/packages/server/lib/files.js index 3add13e81ed..412e7ad64f3 100644 --- a/packages/server/lib/files.js +++ b/packages/server/lib/files.js @@ -1,3 +1,4 @@ +const { clear } = require('console') const path = require('path') const { fs } = require('./util/fs') @@ -24,23 +25,33 @@ module.exports = { }) }, - writeFile (projectRoot, file, contents, options = {}) { + async writeFile (projectRoot, file, contents, options = {}) { const filePath = path.resolve(projectRoot, file) + const writeFileAbortController = new AbortController() const writeOptions = { encoding: options.encoding === undefined ? 'utf8' : options.encoding, flag: options.flag || 'w', + signal: writeFileAbortController.signal, } - return fs.outputFile(filePath, contents, writeOptions) - .then(() => { - return { - contents, - filePath, - } - }) - .catch((err) => { + let writeTimeout = setTimeout(() => { + writeFileAbortController.abort() + }, options.timeout === undefined ? 1e9 : options.timeout) + + try { + await fs.outputFile(filePath, contents, writeOptions) + + return { contents, filePath } + } catch (err) { err.filePath = filePath + + if (err.name === 'AbortError') { + err.aborted = true + } + throw err - }) + } finally { + clearTimeout(writeTimeout) + } }, } From 0a4fc369e8266ab5ca0ce3e00f35f6fbf2a01059 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 19 Nov 2021 14:09:34 -0600 Subject: [PATCH 02/25] Adding timeout to readFile as well --- cli/types/cypress.d.ts | 3 +- .../integration/commands/files_spec.js | 113 +++++++++++++----- packages/driver/src/cy/commands/files.ts | 90 +++++++------- packages/server/lib/files.js | 47 ++++++-- 4 files changed, 172 insertions(+), 81 deletions(-) diff --git a/cli/types/cypress.d.ts b/cli/types/cypress.d.ts index defe6e581eb..ab882263118 100644 --- a/cli/types/cypress.d.ts +++ b/cli/types/cypress.d.ts @@ -2228,7 +2228,7 @@ declare namespace Cypress { }) ``` */ - writeFile(filePath: string, contents: C, options?: Partial): Chainable + writeFile(filePath: string, contents: C, options?: Partial): Chainable /** * Write to a file with the specified encoding and contents. * @@ -3249,7 +3249,6 @@ declare namespace Cypress { interface WriteFileOptions extends Loggable { flag: string encoding: Encodings - timeout: number } // Kind of onerous, but has a nice auto-complete. diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index a6bf3bfafc4..3c071467a95 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -11,7 +11,7 @@ const okResponse = { describe('src/cy/commands/files', () => { beforeEach(() => { // call through normally on everything - cy.stub(Cypress, 'backend').callThrough() + cy.stub(Cypress, 'backend').callThrough().log(false) }) describe('#readFile', () => { @@ -22,7 +22,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'utf8' }, + { encoding: 'utf8', timeout: Cypress.config('defaultCommandTimeout') }, ) }) }) @@ -34,7 +34,19 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'ascii' }, + { encoding: 'ascii', timeout: Cypress.config('defaultCommandTimeout') }, + ) + }) + }) + + it('can take a timeout option', () => { + Cypress.backend.resolves(okResponse) + + cy.readFile('foo.json', { timeout: 12345 }).then(() => { + expect(Cypress.backend).to.be.calledWith( + 'read:file', + 'foo.json', + { encoding: 'utf8', timeout: 12345 }, ) }) }) @@ -50,7 +62,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: null }, + { encoding: null, timeout: Cypress.config('defaultCommandTimeout') }, ) }).should('eql', Buffer.from('\n')) }) @@ -322,6 +334,57 @@ describe('src/cy/commands/files', () => { cy.readFile('foo.json').should('equal', 'contents') }) + + it('throws when the read timeout expires', function (done) { + const err = new Error('TimeoutError: The Promise timed out') + + err.name = 'TimeoutError' + + Cypress.backend.rejects(err) + + cy.on('fail', (err) => { + const { lastLog } = this + + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent`\ + \`cy.readFile("foo")\` timed out after waiting \`50ms\`. + `) + + expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') + + done() + }) + + cy.readFile('foo') + }) + + it('throws when the server read aborts', function (done) { + const err = new Error('AbortError: The readFile operation was aborted on the server') + + err.name = 'AbortError' + err.aborted = true + + Cypress.backend.rejects(err) + + cy.on('fail', (err) => { + const { lastLog } = this + + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent`\ + \`cy.readFile("foo")\` timed out after waiting \`50ms\`. + `) + + expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') + + done() + }) + + cy.readFile('foo') + }) }) }) @@ -337,7 +400,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'w', - timeout: 4000, + timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -354,7 +417,7 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: 4000, + timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -372,7 +435,7 @@ describe('src/cy/commands/files', () => { { encoding: null, flag: 'w', - timeout: 4000, + timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -389,7 +452,7 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: 4000, + timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -458,7 +521,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'a+', - timeout: 4000, + timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -625,7 +688,7 @@ describe('src/cy/commands/files', () => { cy.writeFile('foo.txt', 'contents') }) - it('throws when the driver timeout expires', function (done) { + it('throws when the write timeout expires', function (done) { const err = new Error('TimeoutError') err.name = 'TimeoutError' @@ -677,7 +740,9 @@ describe('src/cy/commands/files', () => { }) }) - describe('#writeFile-error', () => { + describe('#writeFile-error', { + defaultCommandTimeout: 5000, + }, () => { const bigString = JSON.stringify(Cypress._.times((10 ** 6), () => 'hehehehe'), null, 2) const tooBigString = JSON.stringify(Cypress._.times(15000000, '😈'), null, 2) // 72MB // afterEach(() => { @@ -704,24 +769,16 @@ describe('src/cy/commands/files', () => { // }) // }) - // afterEach(() => { - // cy.writeFile('./fixtures/my-long-file.txt', bigString, { timeout: 1 }) - // }) - - // it('writes until timeout', () => { - // cy.visit('/fixtures/dom.html') - - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) + afterEach(() => { + cy.writeFile('./fixtures/my-long-file.txt', bigString, { timeout: 1000 }) + }) - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) + it('writes until timeout', () => { + cy.visit('/fixtures/dom.html') - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) - // }) + cy.get('body').then((bodyElement) => { + expect(true).to.equal(true) + }) + }) }) }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 955697896ce..5a34c367ec3 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -1,12 +1,11 @@ // @ts-nocheck import _ from 'lodash' -import Promise from 'bluebird' import $errUtils from '../../cypress/error_utils' export default (Commands, Cypress, cy) => { Commands.addAll({ - readFile (file, encoding, options = {}) { + async readFile (file, encoding, options = {}) { let userOptions = options if (_.isObject(encoding)) { @@ -22,6 +21,7 @@ export default (Commands, Cypress, cy) => { // to restore the default node behavior. encoding: encoding === undefined ? 'utf8' : encoding, log: true, + timeout: Cypress.config('defaultCommandTimeout'), }) const consoleProps = {} @@ -43,50 +43,60 @@ export default (Commands, Cypress, cy) => { }) } - const verifyAssertions = () => { - return Cypress.backend('read:file', file, _.pick(options, 'encoding')) - .catch((err) => { + const verifyAssertions = async () => { + let result + + try { + result = await Cypress.backend('read:file', file, _.pick(options, ['encoding', 'timeout'])).timeout(options.timeout) + } catch (err) { if (err.code === 'ENOENT') { - return { + result = { contents: null, filePath: err.filePath, } + } else if (err.aborted || err.name === 'TimeoutError') { + return $errUtils.throwErrByPath('files.timed_out', { + onFail: options._log, + args: { cmd: 'readFile', file, timeout: options.timeout }, + }) + } else { + return $errUtils.throwErrByPath('files.unexpected_error', { + onFail: options._log, + args: { cmd: 'readFile', action: 'read', file, filePath: err.filePath, error: err.message }, + }) } + } - return $errUtils.throwErrByPath('files.unexpected_error', { - onFail: options._log, - args: { cmd: 'readFile', action: 'read', file, filePath: err.filePath, error: err.message }, - }) - }).then(({ contents, filePath }) => { - // https://github.com/cypress-io/cypress/issues/1558 - // We invoke Buffer.from() in order to transform this from an ArrayBuffer - - // which socket.io uses to transfer the file over the websocket - into a - // `Buffer`, which webpack polyfills in the browser. - if (options.encoding === null) { - contents = Buffer.from(contents) - } + let { contents, filePath } = result - consoleProps['File Path'] = filePath - consoleProps['Contents'] = contents - - return cy.verifyUpcomingAssertions(contents, options, { - ensureExistenceFor: 'subject', - onFail (err) { - if (err.type !== 'existence') { - return - } - - // file exists but it shouldn't - or - file doesn't exist but it should - const errPath = contents ? 'files.existent' : 'files.nonexistent' - const { message, docsUrl } = $errUtils.cypressErrByPath(errPath, { - args: { file, filePath }, - }) - - err.message = message - err.docsUrl = docsUrl - }, - onRetry: verifyAssertions, - }) + // https://github.com/cypress-io/cypress/issues/1558 + // We invoke Buffer.from() in order to transform this from an ArrayBuffer - + // which socket.io uses to transfer the file over the websocket - into a + // `Buffer`, which webpack polyfills in the browser. + if (options.encoding === null) { + contents = Buffer.from(contents) + } + + consoleProps['File Path'] = filePath + consoleProps['Contents'] = contents + + return cy.verifyUpcomingAssertions(contents, options, { + ensureExistenceFor: 'subject', + onFail (err) { + if (err.type !== 'existence') { + return + } + + // file exists but it shouldn't - or - file doesn't exist but it should + const errPath = contents ? 'files.existent' : 'files.nonexistent' + const { message, docsUrl } = $errUtils.cypressErrByPath(errPath, { + args: { file, filePath }, + }) + + err.message = message + err.docsUrl = docsUrl + }, + onRetry: verifyAssertions, }) } @@ -110,7 +120,7 @@ export default (Commands, Cypress, cy) => { encoding: encoding === undefined ? 'utf8' : encoding, flag: userOptions.flag ? userOptions.flag : 'w', log: true, - timeout: 4000, + timeout: Cypress.config('defaultCommandTimeout'), }) const consoleProps = {} diff --git a/packages/server/lib/files.js b/packages/server/lib/files.js index 412e7ad64f3..a1a54eab49e 100644 --- a/packages/server/lib/files.js +++ b/packages/server/lib/files.js @@ -1,28 +1,49 @@ -const { clear } = require('console') const path = require('path') const { fs } = require('./util/fs') module.exports = { - readFile (projectRoot, file, options = {}) { + async readFile (projectRoot, file, options = {}) { const filePath = path.resolve(projectRoot, file) const readFn = (path.extname(filePath) === '.json' && options.encoding !== null) ? fs.readJsonAsync : fs.readFileAsync + const readFileAbortController = new AbortController() // https://github.com/cypress-io/cypress/issues/1558 // If no encoding is specified, then Cypress has historically defaulted // to `utf8`, because of it's focus on text files. This is in contrast to // NodeJs, which defaults to binary. We allow users to pass in `null` // to restore the default node behavior. - return readFn(filePath, options.encoding === undefined ? 'utf8' : options.encoding) - .then((contents) => { + + const readOptions = { + encoding: options.encoding === undefined ? 'utf8' : options.encoding, + signal: readFileAbortController.signal, + } + + let readFileTimeout + + if (options.timeout !== undefined) { + readFileTimeout = setTimeout(() => { + readFileAbortController.abort() + }, options.timeout) + } + + try { + const contents = await readFn(filePath, readOptions) + return { contents, filePath, } - }) - .catch((err) => { + } catch (err) { err.filePath = filePath + + if (err.name === 'AbortError') { + err.aborted = true + } + throw err - }) + } finally { + clearTimeout(readFileTimeout) + } }, async writeFile (projectRoot, file, contents, options = {}) { @@ -34,9 +55,13 @@ module.exports = { signal: writeFileAbortController.signal, } - let writeTimeout = setTimeout(() => { - writeFileAbortController.abort() - }, options.timeout === undefined ? 1e9 : options.timeout) + let writeFileTimeout + + if (options.timeout !== undefined) { + setTimeout(() => { + writeFileAbortController.abort() + }, options.timeout) + } try { await fs.outputFile(filePath, contents, writeOptions) @@ -51,7 +76,7 @@ module.exports = { throw err } finally { - clearTimeout(writeTimeout) + clearTimeout(writeFileTimeout) } }, } From 86917dad333cc0ea7438f2f02f45eb3b383aaec9 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 19 Nov 2021 17:07:19 -0600 Subject: [PATCH 03/25] Adding unit tests for server --- packages/server/lib/files.js | 16 +++++- packages/server/test/unit/files_spec.js | 69 +++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 2 deletions(-) diff --git a/packages/server/lib/files.js b/packages/server/lib/files.js index a1a54eab49e..8d86e246fd6 100644 --- a/packages/server/lib/files.js +++ b/packages/server/lib/files.js @@ -27,7 +27,19 @@ module.exports = { } try { - const contents = await readFn(filePath, readOptions) + // graceful-fs does not support the standard fs Promise API. + // Errors must be intercepted using a callback parameter. + const readPromise = new Promise((resolve, reject) => { + readFn(filePath, readOptions, (err, data) => { + if (err) { + reject(err) + } else { + resolve(data) + } + }) + }) + + const contents = await readPromise return { contents, @@ -58,7 +70,7 @@ module.exports = { let writeFileTimeout if (options.timeout !== undefined) { - setTimeout(() => { + writeFileTimeout = setTimeout(() => { writeFileAbortController.abort() }, options.timeout) } diff --git a/packages/server/test/unit/files_spec.js b/packages/server/test/unit/files_spec.js index ab0cc8bb9a1..8b0817cbce5 100644 --- a/packages/server/test/unit/files_spec.js +++ b/packages/server/test/unit/files_spec.js @@ -2,6 +2,7 @@ require('../spec_helper') const config = require(`${root}lib/config`) const files = require(`${root}lib/files`) +const { fs } = require(`${root}lib/util/fs`) const FixturesHelper = require('@tooling/system-tests/lib/fixtures') describe('lib/files', () => { @@ -61,6 +62,42 @@ describe('lib/files', () => { ]) }) }) + + it('aborts readFn execution if not complete within the specified timeout', function () { + const mockTimeoutId = 4567 + + sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { + // execute timeout function synchronously so that abort signal is aborted prior + // to outputFile execution + funcArg() + + return mockTimeoutId + }) + + sinon.stub(global, 'clearTimeout') + + return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt', { timeout: 100 }).catch((err) => { + expect(err.name).to.equal('AbortError') + expect(err.aborted).to.equal(true) + expect(err.filePath).to.include('tests/_fixtures/message.txt') + expect(global.clearTimeout).to.have.been.calledWith(mockTimeoutId) + }) + }) + + it('catches generic errors from readFn and appends filePath', function () { + sinon.stub(fs, 'readFileAsync').callsFake(function mockReadFile (path, options, callback) { + callback(new Error('UnexpectedError: How could this happen'), undefined) + }) + + sinon.stub(global, 'clearTimeout') + + return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt').catch((err) => { + expect(err.message).to.equal('UnexpectedError: How could this happen') + expect(err.aborted).to.equal(undefined) + expect(err.filePath).to.include('tests/_fixtures/message.txt') + expect(global.clearTimeout).to.have.been.calledWith(undefined) + }) + }) }) context('#writeFile', () => { @@ -118,5 +155,37 @@ describe('lib/files', () => { }) }) }) + + it('aborts outputFile execution if not complete within the specified timeout', function () { + sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { + // execute timeout function synchronously so that abort signal is aborted prior + // to outputFile execution + funcArg() + + // returned timeoutId is synchronized with clearTimeout assertion below + return 12345 + }) + + sinon.stub(global, 'clearTimeout') + + return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo', { timeout: 100 }).catch((err) => { + expect(err.name).to.equal('AbortError') + expect(err.aborted).to.equal(true) + expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') + expect(global.clearTimeout).to.have.been.calledWith(12345) + }) + }) + + it('catches generic errors from outputFile and appends filePath', function () { + sinon.stub(fs, 'outputFile').rejects(new Error('UnexpectedError: How could this happen')) + sinon.stub(global, 'clearTimeout') + + return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo').catch((err) => { + expect(err.message).to.equal('UnexpectedError: How could this happen') + expect(err.aborted).to.equal(undefined) + expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') + expect(global.clearTimeout).to.have.been.calledWith(undefined) + }) + }) }) }) From 2cc5b89647d2e30067cee5446149ca924d1c653e Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 19 Nov 2021 17:49:55 -0600 Subject: [PATCH 04/25] Updating some phrasing --- .../integration/commands/files_spec.js | 67 ++++++------------- 1 file changed, 22 insertions(+), 45 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index 3c071467a95..c99133cd479 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -1,6 +1,5 @@ const { assertLogLength } = require('../../support/utils') const { stripIndent } = require('common-tags') -const Promise = require('bluebird') const { _ } = Cypress const okResponse = { @@ -361,7 +360,7 @@ describe('src/cy/commands/files', () => { }) it('throws when the server read aborts', function (done) { - const err = new Error('AbortError: The readFile operation was aborted on the server') + const err = new Error('AbortError: The readFile operation was aborted by the server') err.name = 'AbortError' err.aborted = true @@ -713,8 +712,8 @@ describe('src/cy/commands/files', () => { cy.writeFile('foo.txt', 'contents', { timeout: 1000 }) }) - it('throws when the server write is aborted', function (done) { - const err = new Error() + it('throws when the server write aborts', function (done) { + const err = new Error('AbortError: The writeFile operation was aborted by the server') err.aborted = true @@ -740,45 +739,23 @@ describe('src/cy/commands/files', () => { }) }) - describe('#writeFile-error', { - defaultCommandTimeout: 5000, - }, () => { - const bigString = JSON.stringify(Cypress._.times((10 ** 6), () => 'hehehehe'), null, 2) - const tooBigString = JSON.stringify(Cypress._.times(15000000, '😈'), null, 2) // 72MB - // afterEach(() => { - // return new Promise((resolve) => { - // setTimeout(() => { - // resolve(cy.writeFile('./fixtures/my-long-file.txt', bigString)) - // }, 5000) - // }) - // }) - - // afterEach(() => { - // return cy.writeFile('./fixtures/my-long-file.txt', bigString) - // }) - - // afterEach(() => { - // cy.writeFile('./fixtures/my-long-file.txt', bigString) - // }) - - // afterEach(() => { - // return new Promise((resolve) => { - // setTimeout(() => { - // resolve(cy.writeFile('./fixtures/my-long-file.txt', Cypress._.times(9000000, '😈'), { timeout: 1000 })) - // }, 5000) - // }) - // }) - - afterEach(() => { - cy.writeFile('./fixtures/my-long-file.txt', bigString, { timeout: 1000 }) - }) - - it('writes until timeout', () => { - cy.visit('/fixtures/dom.html') - - cy.get('body').then((bodyElement) => { - expect(true).to.equal(true) - }) - }) - }) + // TODO remove repro test case + // describe('#writeFile-error', { + // defaultCommandTimeout: 5000, + // }, () => { + // const bigString = JSON.stringify(Cypress._.times((10 ** 6), () => 'hehehehe'), null, 2) + // const tooBigString = JSON.stringify(Cypress._.times(15000000, '😈'), null, 2) // 72MB + + // afterEach(() => { + // cy.writeFile('./fixtures/my-long-file.txt', bigString, { timeout: 1000 }) + // }) + + // it('writes until timeout', () => { + // cy.visit('/fixtures/dom.html') + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + // }) + // }) }) From 57b8c9fb408b7bbf809d6c2c5358d4dc52f24df3 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Mon, 22 Nov 2021 11:39:26 -0600 Subject: [PATCH 05/25] Clearing the default timeout to prevent race condition --- packages/driver/src/cy/commands/files.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 5a34c367ec3..03c37781542 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -43,6 +43,10 @@ export default (Commands, Cypress, cy) => { }) } + // We clear the default timeout because we are handling + // the timeout ourselves. + cy.clearTimeout() + const verifyAssertions = async () => { let result @@ -153,6 +157,10 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } + // We clear the default timeout because we are handling + // the timeout ourselves. + cy.clearTimeout() + try { await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag', 'timeout'])).timeout(options.timeout) } catch (err) { From 0ca8f0931841f8ea82fd4840e457208912d12561 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Tue, 23 Nov 2021 15:04:33 -0600 Subject: [PATCH 06/25] Increasing socket http buffer size. Using `responseTimeout` config value for read/write timeouts. Reverting server readFile/writeFile changes to remove explicit abort workflows. --- .../integration/commands/files_spec.js | 130 +++++++----------- packages/driver/src/cy/commands/files.ts | 12 +- packages/server/lib/files.js | 80 +++-------- packages/server/test/unit/files_spec.js | 69 ---------- packages/socket/lib/socket.ts | 13 +- 5 files changed, 78 insertions(+), 226 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index c99133cd479..aeab7addb34 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -21,7 +21,6 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'utf8', timeout: Cypress.config('defaultCommandTimeout') }, ) }) }) @@ -33,19 +32,6 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'ascii', timeout: Cypress.config('defaultCommandTimeout') }, - ) - }) - }) - - it('can take a timeout option', () => { - Cypress.backend.resolves(okResponse) - - cy.readFile('foo.json', { timeout: 12345 }).then(() => { - expect(Cypress.backend).to.be.calledWith( - 'read:file', - 'foo.json', - { encoding: 'utf8', timeout: 12345 }, ) }) }) @@ -61,7 +47,6 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: null, timeout: Cypress.config('defaultCommandTimeout') }, ) }).should('eql', Buffer.from('\n')) }) @@ -169,7 +154,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - defaultCommandTimeout: 50, + responseTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -335,11 +320,9 @@ describe('src/cy/commands/files', () => { }) it('throws when the read timeout expires', function (done) { - const err = new Error('TimeoutError: The Promise timed out') - - err.name = 'TimeoutError' - - Cypress.backend.rejects(err) + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -348,7 +331,7 @@ describe('src/cy/commands/files', () => { expect(lastLog.get('error')).to.eq(err) expect(lastLog.get('state')).to.eq('failed') expect(err.message).to.eq(stripIndent`\ - \`cy.readFile("foo")\` timed out after waiting \`50ms\`. + \`cy.readFile("foo")\` timed out after waiting \`10ms\`. `) expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') @@ -356,16 +339,15 @@ describe('src/cy/commands/files', () => { done() }) - cy.readFile('foo') + cy.readFile('foo', { timeout: 10 }) }) - it('throws when the server read aborts', function (done) { - const err = new Error('AbortError: The readFile operation was aborted by the server') - - err.name = 'AbortError' - err.aborted = true - - Cypress.backend.rejects(err) + it('uses responseTimeout config value if option not provided', { + responseTimeout: 42, + }, function (done) { + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -374,7 +356,7 @@ describe('src/cy/commands/files', () => { expect(lastLog.get('error')).to.eq(err) expect(lastLog.get('state')).to.eq('failed') expect(err.message).to.eq(stripIndent`\ - \`cy.readFile("foo")\` timed out after waiting \`50ms\`. + \`cy.readFile("foo")\` timed out after waiting \`42ms\`. `) expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') @@ -399,7 +381,6 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'w', - timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -416,7 +397,6 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -434,7 +414,6 @@ describe('src/cy/commands/files', () => { { encoding: null, flag: 'w', - timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -451,24 +430,6 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: Cypress.config('defaultCommandTimeout'), - }, - ) - }) - }) - - it('can take timeout as part of options', () => { - Cypress.backend.resolves(okResponse) - - cy.writeFile('foo.txt', 'contents', { timeout: 12345 }).then(() => { - expect(Cypress.backend).to.be.calledWith( - 'write:file', - 'foo.txt', - 'contents', - { - encoding: 'utf8', - flag: 'w', - timeout: 12345, }, ) }) @@ -520,7 +481,6 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'a+', - timeout: Cypress.config('defaultCommandTimeout'), }, ) }) @@ -578,7 +538,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - defaultCommandTimeout: 50, + responseTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -688,11 +648,9 @@ describe('src/cy/commands/files', () => { }) it('throws when the write timeout expires', function (done) { - const err = new Error('TimeoutError') - - err.name = 'TimeoutError' - - Cypress.backend.rejects(err) + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => {}) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -701,7 +659,7 @@ describe('src/cy/commands/files', () => { expect(lastLog.get('error')).to.eq(err) expect(lastLog.get('state')).to.eq('failed') expect(err.message).to.eq(stripIndent` - \`cy.writeFile("foo.txt")\` timed out after waiting \`1000ms\`. + \`cy.writeFile("foo.txt")\` timed out after waiting \`10ms\`. `) expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') @@ -709,15 +667,15 @@ describe('src/cy/commands/files', () => { done() }) - cy.writeFile('foo.txt', 'contents', { timeout: 1000 }) + cy.writeFile('foo.txt', 'contents', { timeout: 10 }) }) - it('throws when the server write aborts', function (done) { - const err = new Error('AbortError: The writeFile operation was aborted by the server') - - err.aborted = true - - Cypress.backend.rejects(err) + it('uses responseTimeout config value if option not provided', { + responseTimeout: 42, + }, function (done) { + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -726,7 +684,7 @@ describe('src/cy/commands/files', () => { expect(lastLog.get('error')).to.eq(err) expect(lastLog.get('state')).to.eq('failed') expect(err.message).to.eq(stripIndent` - \`cy.writeFile("foo.txt")\` timed out after waiting \`1000ms\`. + \`cy.writeFile("foo.txt")\` timed out after waiting \`42ms\`. `) expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') @@ -734,27 +692,37 @@ describe('src/cy/commands/files', () => { done() }) - cy.writeFile('foo.txt', 'contents', { timeout: 1000 }) + cy.writeFile('foo.txt', 'contents') }) }) }) // TODO remove repro test case - // describe('#writeFile-error', { - // defaultCommandTimeout: 5000, - // }, () => { - // const bigString = JSON.stringify(Cypress._.times((10 ** 6), () => 'hehehehe'), null, 2) - // const tooBigString = JSON.stringify(Cypress._.times(15000000, '😈'), null, 2) // 72MB - - // afterEach(() => { - // cy.writeFile('./fixtures/my-long-file.txt', bigString, { timeout: 1000 }) + // describe.only('#writeFile-error', () => { + // describe('error', () => { + // const bigString = JSON.stringify(Cypress._.times(10000000, '😈'), null, 2) // 72MB + // const tooBigString = JSON.stringify(Cypress._.times(15000000, '😈'), null, 2) // 72MB + + // afterEach(() => { + // cy.writeFile('./fixtures/my-long-file.txt', tooBigString, { timeout: 30000 }) + // }) + + // it('writes until timeout', () => { + // cy.visit('/fixtures/dom.html') + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + // }) // }) - // it('writes until timeout', () => { - // cy.visit('/fixtures/dom.html') + // describe('should pass', () => { + // it('writes until timeout', () => { + // cy.visit('/fixtures/dom.html') - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) // }) // }) // }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 03c37781542..00c6aba48c2 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -21,7 +21,7 @@ export default (Commands, Cypress, cy) => { // to restore the default node behavior. encoding: encoding === undefined ? 'utf8' : encoding, log: true, - timeout: Cypress.config('defaultCommandTimeout'), + timeout: Cypress.config('responseTimeout'), }) const consoleProps = {} @@ -51,14 +51,14 @@ export default (Commands, Cypress, cy) => { let result try { - result = await Cypress.backend('read:file', file, _.pick(options, ['encoding', 'timeout'])).timeout(options.timeout) + result = await Cypress.backend('read:file', file, _.pick(options, ['encoding'])).timeout(options.timeout) } catch (err) { if (err.code === 'ENOENT') { result = { contents: null, filePath: err.filePath, } - } else if (err.aborted || err.name === 'TimeoutError') { + } else if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, args: { cmd: 'readFile', file, timeout: options.timeout }, @@ -124,7 +124,7 @@ export default (Commands, Cypress, cy) => { encoding: encoding === undefined ? 'utf8' : encoding, flag: userOptions.flag ? userOptions.flag : 'w', log: true, - timeout: Cypress.config('defaultCommandTimeout'), + timeout: Cypress.config('responseTimeout'), }) const consoleProps = {} @@ -162,9 +162,9 @@ export default (Commands, Cypress, cy) => { cy.clearTimeout() try { - await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag', 'timeout'])).timeout(options.timeout) + await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag'])).timeout(options.timeout) } catch (err) { - if (err.aborted || err.name === 'TimeoutError') { + if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, args: { cmd: 'writeFile', file: fileName, timeout: options.timeout }, diff --git a/packages/server/lib/files.js b/packages/server/lib/files.js index 8d86e246fd6..3add13e81ed 100644 --- a/packages/server/lib/files.js +++ b/packages/server/lib/files.js @@ -2,93 +2,45 @@ const path = require('path') const { fs } = require('./util/fs') module.exports = { - async readFile (projectRoot, file, options = {}) { + readFile (projectRoot, file, options = {}) { const filePath = path.resolve(projectRoot, file) const readFn = (path.extname(filePath) === '.json' && options.encoding !== null) ? fs.readJsonAsync : fs.readFileAsync - const readFileAbortController = new AbortController() // https://github.com/cypress-io/cypress/issues/1558 // If no encoding is specified, then Cypress has historically defaulted // to `utf8`, because of it's focus on text files. This is in contrast to // NodeJs, which defaults to binary. We allow users to pass in `null` // to restore the default node behavior. - - const readOptions = { - encoding: options.encoding === undefined ? 'utf8' : options.encoding, - signal: readFileAbortController.signal, - } - - let readFileTimeout - - if (options.timeout !== undefined) { - readFileTimeout = setTimeout(() => { - readFileAbortController.abort() - }, options.timeout) - } - - try { - // graceful-fs does not support the standard fs Promise API. - // Errors must be intercepted using a callback parameter. - const readPromise = new Promise((resolve, reject) => { - readFn(filePath, readOptions, (err, data) => { - if (err) { - reject(err) - } else { - resolve(data) - } - }) - }) - - const contents = await readPromise - + return readFn(filePath, options.encoding === undefined ? 'utf8' : options.encoding) + .then((contents) => { return { contents, filePath, } - } catch (err) { + }) + .catch((err) => { err.filePath = filePath - - if (err.name === 'AbortError') { - err.aborted = true - } - throw err - } finally { - clearTimeout(readFileTimeout) - } + }) }, - async writeFile (projectRoot, file, contents, options = {}) { + writeFile (projectRoot, file, contents, options = {}) { const filePath = path.resolve(projectRoot, file) - const writeFileAbortController = new AbortController() const writeOptions = { encoding: options.encoding === undefined ? 'utf8' : options.encoding, flag: options.flag || 'w', - signal: writeFileAbortController.signal, } - let writeFileTimeout - - if (options.timeout !== undefined) { - writeFileTimeout = setTimeout(() => { - writeFileAbortController.abort() - }, options.timeout) - } - - try { - await fs.outputFile(filePath, contents, writeOptions) - - return { contents, filePath } - } catch (err) { - err.filePath = filePath - - if (err.name === 'AbortError') { - err.aborted = true + return fs.outputFile(filePath, contents, writeOptions) + .then(() => { + return { + contents, + filePath, } - + }) + .catch((err) => { + err.filePath = filePath throw err - } finally { - clearTimeout(writeFileTimeout) - } + }) }, } diff --git a/packages/server/test/unit/files_spec.js b/packages/server/test/unit/files_spec.js index 8b0817cbce5..ab0cc8bb9a1 100644 --- a/packages/server/test/unit/files_spec.js +++ b/packages/server/test/unit/files_spec.js @@ -2,7 +2,6 @@ require('../spec_helper') const config = require(`${root}lib/config`) const files = require(`${root}lib/files`) -const { fs } = require(`${root}lib/util/fs`) const FixturesHelper = require('@tooling/system-tests/lib/fixtures') describe('lib/files', () => { @@ -62,42 +61,6 @@ describe('lib/files', () => { ]) }) }) - - it('aborts readFn execution if not complete within the specified timeout', function () { - const mockTimeoutId = 4567 - - sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { - // execute timeout function synchronously so that abort signal is aborted prior - // to outputFile execution - funcArg() - - return mockTimeoutId - }) - - sinon.stub(global, 'clearTimeout') - - return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt', { timeout: 100 }).catch((err) => { - expect(err.name).to.equal('AbortError') - expect(err.aborted).to.equal(true) - expect(err.filePath).to.include('tests/_fixtures/message.txt') - expect(global.clearTimeout).to.have.been.calledWith(mockTimeoutId) - }) - }) - - it('catches generic errors from readFn and appends filePath', function () { - sinon.stub(fs, 'readFileAsync').callsFake(function mockReadFile (path, options, callback) { - callback(new Error('UnexpectedError: How could this happen'), undefined) - }) - - sinon.stub(global, 'clearTimeout') - - return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt').catch((err) => { - expect(err.message).to.equal('UnexpectedError: How could this happen') - expect(err.aborted).to.equal(undefined) - expect(err.filePath).to.include('tests/_fixtures/message.txt') - expect(global.clearTimeout).to.have.been.calledWith(undefined) - }) - }) }) context('#writeFile', () => { @@ -155,37 +118,5 @@ describe('lib/files', () => { }) }) }) - - it('aborts outputFile execution if not complete within the specified timeout', function () { - sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { - // execute timeout function synchronously so that abort signal is aborted prior - // to outputFile execution - funcArg() - - // returned timeoutId is synchronized with clearTimeout assertion below - return 12345 - }) - - sinon.stub(global, 'clearTimeout') - - return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo', { timeout: 100 }).catch((err) => { - expect(err.name).to.equal('AbortError') - expect(err.aborted).to.equal(true) - expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') - expect(global.clearTimeout).to.have.been.calledWith(12345) - }) - }) - - it('catches generic errors from outputFile and appends filePath', function () { - sinon.stub(fs, 'outputFile').rejects(new Error('UnexpectedError: How could this happen')) - sinon.stub(global, 'clearTimeout') - - return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo').catch((err) => { - expect(err.message).to.equal('UnexpectedError: How could this happen') - expect(err.aborted).to.equal(undefined) - expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') - expect(global.clearTimeout).to.have.been.calledWith(undefined) - }) - }) }) }) diff --git a/packages/socket/lib/socket.ts b/packages/socket/lib/socket.ts index e93099df99a..f31304f8a66 100644 --- a/packages/socket/lib/socket.ts +++ b/packages/socket/lib/socket.ts @@ -3,7 +3,7 @@ import type http from 'http' import server, { Server as SocketIOBaseServer, ServerOptions } from 'socket.io' import { client } from './browser' -const HUNDRED_MEGABYTES = 1e8 // 100000000 +const FIVE_HUNDRED_MEGABYTES = 5 * 1e8 // 500000000 const { version } = require('socket.io-client/package.json') const clientSource = require.resolve('socket.io-client/dist/socket.io.js') @@ -15,13 +15,14 @@ type PatchedServerOptions = ServerOptions & { cookie: { name: string | boolean } class SocketIOServer extends SocketIOBaseServer { constructor (srv: http.Server, opts?: Partial) { - // in socket.io v3, they reduced down the max buffer size - // from 100mb to 1mb, so we reset it back to the previous value + // in socket.io v4, the default maxHttpBufferSize is set to 1 MB to mitigate + // the potential of DoS attacks. given that our usage is limited to a local network, + // we can safely increase the buffer size to allow larger payloads over the + // socket. this is particularly helpful for large I/O processes (readFile/writeFile). // - // previous commit for reference: - // https://github.com/socketio/engine.io/blame/61b949259ed966ef6fc8bfd61f14d1a2ef06d319/lib/server.js#L29 + // related issue: https://github.com/cypress-io/cypress/issues/3350 opts = opts ?? {} - opts.maxHttpBufferSize = opts.maxHttpBufferSize ?? HUNDRED_MEGABYTES + opts.maxHttpBufferSize = opts.maxHttpBufferSize ?? FIVE_HUNDRED_MEGABYTES super(srv, opts) } From 840584248c71930fcaae66839845a0d65cee6756 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Wed, 24 Nov 2021 16:30:45 -0600 Subject: [PATCH 07/25] Reverting buffer increase to better scope changes --- packages/socket/lib/socket.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/socket/lib/socket.ts b/packages/socket/lib/socket.ts index f31304f8a66..e93099df99a 100644 --- a/packages/socket/lib/socket.ts +++ b/packages/socket/lib/socket.ts @@ -3,7 +3,7 @@ import type http from 'http' import server, { Server as SocketIOBaseServer, ServerOptions } from 'socket.io' import { client } from './browser' -const FIVE_HUNDRED_MEGABYTES = 5 * 1e8 // 500000000 +const HUNDRED_MEGABYTES = 1e8 // 100000000 const { version } = require('socket.io-client/package.json') const clientSource = require.resolve('socket.io-client/dist/socket.io.js') @@ -15,14 +15,13 @@ type PatchedServerOptions = ServerOptions & { cookie: { name: string | boolean } class SocketIOServer extends SocketIOBaseServer { constructor (srv: http.Server, opts?: Partial) { - // in socket.io v4, the default maxHttpBufferSize is set to 1 MB to mitigate - // the potential of DoS attacks. given that our usage is limited to a local network, - // we can safely increase the buffer size to allow larger payloads over the - // socket. this is particularly helpful for large I/O processes (readFile/writeFile). + // in socket.io v3, they reduced down the max buffer size + // from 100mb to 1mb, so we reset it back to the previous value // - // related issue: https://github.com/cypress-io/cypress/issues/3350 + // previous commit for reference: + // https://github.com/socketio/engine.io/blame/61b949259ed966ef6fc8bfd61f14d1a2ef06d319/lib/server.js#L29 opts = opts ?? {} - opts.maxHttpBufferSize = opts.maxHttpBufferSize ?? FIVE_HUNDRED_MEGABYTES + opts.maxHttpBufferSize = opts.maxHttpBufferSize ?? HUNDRED_MEGABYTES super(srv, opts) } From 2a619157449d8f683e78bc87dd781ef662673f9c Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Wed, 24 Nov 2021 16:39:35 -0600 Subject: [PATCH 08/25] Reverting inadvertent test changes. --- .../integration/commands/files_spec.js | 35 +++---------------- packages/driver/src/cy/commands/files.ts | 16 ++++----- 2 files changed, 12 insertions(+), 39 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index aeab7addb34..51b2e96d639 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -10,7 +10,7 @@ const okResponse = { describe('src/cy/commands/files', () => { beforeEach(() => { // call through normally on everything - cy.stub(Cypress, 'backend').callThrough().log(false) + cy.stub(Cypress, 'backend').callThrough() }) describe('#readFile', () => { @@ -21,6 +21,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', + { encoding: 'utf8' }, ) }) }) @@ -32,6 +33,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', + { encoding: 'ascii' }, ) }) }) @@ -47,6 +49,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', + { encoding: null }, ) }).should('eql', Buffer.from('\n')) }) @@ -696,34 +699,4 @@ describe('src/cy/commands/files', () => { }) }) }) - - // TODO remove repro test case - // describe.only('#writeFile-error', () => { - // describe('error', () => { - // const bigString = JSON.stringify(Cypress._.times(10000000, '😈'), null, 2) // 72MB - // const tooBigString = JSON.stringify(Cypress._.times(15000000, '😈'), null, 2) // 72MB - - // afterEach(() => { - // cy.writeFile('./fixtures/my-long-file.txt', tooBigString, { timeout: 30000 }) - // }) - - // it('writes until timeout', () => { - // cy.visit('/fixtures/dom.html') - - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) - // }) - // }) - - // describe('should pass', () => { - // it('writes until timeout', () => { - // cy.visit('/fixtures/dom.html') - - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) - // }) - // }) - // }) }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 00c6aba48c2..df3480f83b2 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -6,6 +6,10 @@ import $errUtils from '../../cypress/error_utils' export default (Commands, Cypress, cy) => { Commands.addAll({ async readFile (file, encoding, options = {}) { + // We clear the default timeout because we are handling + // the timeout ourselves. + cy.clearTimeout() + let userOptions = options if (_.isObject(encoding)) { @@ -43,10 +47,6 @@ export default (Commands, Cypress, cy) => { }) } - // We clear the default timeout because we are handling - // the timeout ourselves. - cy.clearTimeout() - const verifyAssertions = async () => { let result @@ -108,6 +108,10 @@ export default (Commands, Cypress, cy) => { }, async writeFile (fileName, contents, encoding, options = {}) { + // We clear the default timeout because we are handling + // the timeout ourselves. + cy.clearTimeout() + let userOptions = options if (_.isObject(encoding)) { @@ -157,10 +161,6 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } - // We clear the default timeout because we are handling - // the timeout ourselves. - cy.clearTimeout() - try { await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag'])).timeout(options.timeout) } catch (err) { From 7a5e813a616721ace6dc07e1fa57c95eef44465b Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Mon, 29 Nov 2021 17:28:11 -0600 Subject: [PATCH 09/25] Adding proper timeout to readFile log options, adding test coverage --- packages/driver/cypress/integration/commands/files_spec.js | 2 ++ packages/driver/src/cy/commands/files.ts | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index 51b2e96d639..342c7b81e75 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -145,6 +145,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'readFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.json') + expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) } }) @@ -529,6 +530,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'writeFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.txt', 'contents') + expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) } }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index df3480f83b2..926b6894cda 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -136,7 +136,7 @@ export default (Commands, Cypress, cy) => { if (options.log) { options._log = Cypress.log({ message: fileName, - timeout: 0, + timeout: options.timeout, consoleProps () { return consoleProps }, From 7f210f72e08f50a26d233fa9b1549b0ea517f2e6 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Mon, 29 Nov 2021 18:16:13 -0600 Subject: [PATCH 10/25] Explicitly returning null from writeFile again --- .../driver/cypress/integration/commands/files_spec.js | 2 +- packages/driver/src/cy/commands/files.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index 342c7b81e75..33f0e821355 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -443,7 +443,7 @@ describe('src/cy/commands/files', () => { Cypress.backend.resolves(okResponse) cy.writeFile('foo.txt', 'contents').then((subject) => { - expect(subject).to.not.exist + expect(subject).to.eq(null) }) }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 926b6894cda..db91d6bb407 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -161,8 +161,10 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } + let result + try { - await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag'])).timeout(options.timeout) + result = await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag'])).timeout(options.timeout) } catch (err) { if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { @@ -176,6 +178,11 @@ export default (Commands, Cypress, cy) => { args: { cmd: 'writeFile', action: 'write', file: fileName, filePath: err.filePath, error: err.message }, }) } + + consoleProps['File Path'] = result?.filePath + consoleProps['Contents'] = result?.contents + + return null }, }) } From 2e4af3365dc2f53d7442b81521620a1a100c1ff8 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Tue, 30 Nov 2021 17:53:37 -0600 Subject: [PATCH 11/25] Updating writeFile function definitions --- cli/types/cypress.d.ts | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/cli/types/cypress.d.ts b/cli/types/cypress.d.ts index ab882263118..8a04619ad56 100644 --- a/cli/types/cypress.d.ts +++ b/cli/types/cypress.d.ts @@ -2209,12 +2209,9 @@ declare namespace Cypress { * @see https://on.cypress.io/writefile ``` cy.writeFile('path/to/message.txt', 'Hello World') - .then((text) => { - expect(text).to.equal('Hello World') // true - }) ``` */ - writeFile(filePath: string, contents: C, encoding: Encodings): Chainable + writeFile(filePath: string, contents: FileContents, encoding: Encodings): Chainable /** * Write to a file with the specified encoding and contents. * @@ -2223,12 +2220,10 @@ declare namespace Cypress { cy.writeFile('path/to/ascii.txt', 'Hello World', { flag: 'a+', encoding: 'ascii' - }).then((text) => { - expect(text).to.equal('Hello World') // true }) ``` */ - writeFile(filePath: string, contents: C, options?: Partial): Chainable + writeFile(filePath: string, contents: FileContents, options?: Partial): Chainable /** * Write to a file with the specified encoding and contents. * @@ -2238,12 +2233,10 @@ declare namespace Cypress { ``` cy.writeFile('path/to/ascii.txt', 'Hello World', 'utf8', { flag: 'a+', - }).then((text) => { - expect(text).to.equal('Hello World') // true }) ``` */ - writeFile(filePath: string, contents: C, encoding: Encodings, options?: Partial): Chainable + writeFile(filePath: string, contents: FileContents, encoding: Encodings, options?: Partial): Chainable /** * jQuery library bound to the AUT From 67f0f0c9593cdb414d29c97e918a7cc92facf549 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Wed, 1 Dec 2021 13:51:36 -0600 Subject: [PATCH 12/25] Restructuring readFile error handling --- packages/driver/src/cy/commands/files.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index db91d6bb407..65e118506c1 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -53,22 +53,25 @@ export default (Commands, Cypress, cy) => { try { result = await Cypress.backend('read:file', file, _.pick(options, ['encoding'])).timeout(options.timeout) } catch (err) { - if (err.code === 'ENOENT') { - result = { - contents: null, - filePath: err.filePath, - } - } else if (err.name === 'TimeoutError') { + if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, args: { cmd: 'readFile', file, timeout: options.timeout }, }) - } else { + } + + // Non-ENOENT errors are not retried + if (err.code !== 'ENOENT') { return $errUtils.throwErrByPath('files.unexpected_error', { onFail: options._log, args: { cmd: 'readFile', action: 'read', file, filePath: err.filePath, error: err.message }, }) } + + result = { + contents: null, + filePath: err.filePath, + } } let { contents, filePath } = result From f053a9cc09f4f91f6f022eba198b055dc60d122c Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Wed, 1 Dec 2021 14:38:14 -0600 Subject: [PATCH 13/25] Updating default values to defaultCommandTimeout to account for passivity concerns --- .../cypress/integration/commands/files_spec.js | 16 ++++++++-------- packages/driver/src/cy/commands/files.ts | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index 33f0e821355..d1404357909 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -145,7 +145,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'readFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.json') - expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) + expect(log.get('timeout')).to.eq(Cypress.config('defaultCommandTimeout')) } }) @@ -158,7 +158,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - responseTimeout: 50, + defaultCommandTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -346,8 +346,8 @@ describe('src/cy/commands/files', () => { cy.readFile('foo', { timeout: 10 }) }) - it('uses responseTimeout config value if option not provided', { - responseTimeout: 42, + it('uses defaultCommandTimeout config value if option not provided', { + defaultCommandTimeout: 42, }, function (done) { Cypress.backend.callsFake(() => { return new Cypress.Promise(() => { /* Broken promise for timeout */ }) @@ -530,7 +530,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'writeFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.txt', 'contents') - expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) + expect(log.get('timeout')).to.eq(Cypress.config('defaultCommandTimeout')) } }) @@ -543,7 +543,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - responseTimeout: 50, + defaultCommandTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -675,8 +675,8 @@ describe('src/cy/commands/files', () => { cy.writeFile('foo.txt', 'contents', { timeout: 10 }) }) - it('uses responseTimeout config value if option not provided', { - responseTimeout: 42, + it('uses defaultCommandTimeout config value if option not provided', { + defaultCommandTimeout: 42, }, function (done) { Cypress.backend.callsFake(() => { return new Cypress.Promise(() => { /* Broken promise for timeout */ }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 65e118506c1..315a2492df7 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -25,7 +25,7 @@ export default (Commands, Cypress, cy) => { // to restore the default node behavior. encoding: encoding === undefined ? 'utf8' : encoding, log: true, - timeout: Cypress.config('responseTimeout'), + timeout: Cypress.config('defaultCommandTimeout'), }) const consoleProps = {} @@ -131,7 +131,7 @@ export default (Commands, Cypress, cy) => { encoding: encoding === undefined ? 'utf8' : encoding, flag: userOptions.flag ? userOptions.flag : 'w', log: true, - timeout: Cypress.config('responseTimeout'), + timeout: Cypress.config('defaultCommandTimeout'), }) const consoleProps = {} From 152a121f6a7cfea7a61d9433b01ea652b2e86cc0 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Thu, 2 Dec 2021 16:15:39 -0600 Subject: [PATCH 14/25] Detecting socket disconnections that occur during message send. Increasing HTTP buffer max to node Buffer max. --- packages/runner-shared/src/event-manager.js | 15 ++++++++++++++- packages/socket/lib/socket.ts | 5 ++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/packages/runner-shared/src/event-manager.js b/packages/runner-shared/src/event-manager.js index 714e691ccd6..6da675aca9c 100644 --- a/packages/runner-shared/src/event-manager.js +++ b/packages/runner-shared/src/event-manager.js @@ -403,7 +403,20 @@ export const eventManager = { _.each(driverToSocketEvents, (event) => { Cypress.on(event, (...args) => { - return ws.emit(event, ...args) + const cb = args.pop() + + const handleDisconnect = function (disconnectReason) { + cb({ + error: new Error(disconnectReason), + }) + } + + ws.once('disconnect', handleDisconnect) + + return ws.emit(event, ...args, (...cbArgs) => { + ws.off('disconnect', handleDisconnect) + cb(...cbArgs) + }) }) }) diff --git a/packages/socket/lib/socket.ts b/packages/socket/lib/socket.ts index e93099df99a..1248c09c8a4 100644 --- a/packages/socket/lib/socket.ts +++ b/packages/socket/lib/socket.ts @@ -1,10 +1,9 @@ import fs from 'fs' +import buffer from 'buffer' import type http from 'http' import server, { Server as SocketIOBaseServer, ServerOptions } from 'socket.io' import { client } from './browser' -const HUNDRED_MEGABYTES = 1e8 // 100000000 - const { version } = require('socket.io-client/package.json') const clientSource = require.resolve('socket.io-client/dist/socket.io.js') @@ -21,7 +20,7 @@ class SocketIOServer extends SocketIOBaseServer { // previous commit for reference: // https://github.com/socketio/engine.io/blame/61b949259ed966ef6fc8bfd61f14d1a2ef06d319/lib/server.js#L29 opts = opts ?? {} - opts.maxHttpBufferSize = opts.maxHttpBufferSize ?? HUNDRED_MEGABYTES + opts.maxHttpBufferSize = opts.maxHttpBufferSize ?? buffer.constants.MAX_LENGTH super(srv, opts) } From e928959480d07543709a8c4ec6ed018eeea5c147 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Thu, 2 Dec 2021 17:02:43 -0600 Subject: [PATCH 15/25] Reverting previous changes to validate socket disconnect detection --- .../integration/commands/files_spec.js | 248 ++++++++++++------ packages/driver/src/cy/commands/files.ts | 8 +- 2 files changed, 176 insertions(+), 80 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index d1404357909..fa7b72e6753 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -10,7 +10,7 @@ const okResponse = { describe('src/cy/commands/files', () => { beforeEach(() => { // call through normally on everything - cy.stub(Cypress, 'backend').callThrough() + cy.stub(Cypress, 'backend').callThrough().log(false) }) describe('#readFile', () => { @@ -21,7 +21,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'utf8' }, + { encoding: 'utf8', timeout: 4000 }, ) }) }) @@ -33,7 +33,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'ascii' }, + { encoding: 'ascii', timeout: 4000 }, ) }) }) @@ -49,7 +49,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: null }, + { encoding: null, timeout: 4000 }, ) }).should('eql', Buffer.from('\n')) }) @@ -323,53 +323,53 @@ describe('src/cy/commands/files', () => { cy.readFile('foo.json').should('equal', 'contents') }) - it('throws when the read timeout expires', function (done) { - Cypress.backend.callsFake(() => { - return new Cypress.Promise(() => { /* Broken promise for timeout */ }) - }) + // it('throws when the read timeout expires', function (done) { + // Cypress.backend.callsFake(() => { + // return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + // }) - cy.on('fail', (err) => { - const { lastLog } = this + // cy.on('fail', (err) => { + // const { lastLog } = this - assertLogLength(this.logs, 1) - expect(lastLog.get('error')).to.eq(err) - expect(lastLog.get('state')).to.eq('failed') - expect(err.message).to.eq(stripIndent`\ - \`cy.readFile("foo")\` timed out after waiting \`10ms\`. - `) + // assertLogLength(this.logs, 1) + // expect(lastLog.get('error')).to.eq(err) + // expect(lastLog.get('state')).to.eq('failed') + // expect(err.message).to.eq(stripIndent`\ + // \`cy.readFile("foo")\` timed out after waiting \`10ms\`. + // `) - expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') + // expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') - done() - }) + // done() + // }) - cy.readFile('foo', { timeout: 10 }) - }) + // cy.readFile('foo', { timeout: 10 }) + // }) - it('uses defaultCommandTimeout config value if option not provided', { - defaultCommandTimeout: 42, - }, function (done) { - Cypress.backend.callsFake(() => { - return new Cypress.Promise(() => { /* Broken promise for timeout */ }) - }) + // it('uses defaultCommandTimeout config value if option not provided', { + // defaultCommandTimeout: 42, + // }, function (done) { + // Cypress.backend.callsFake(() => { + // return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + // }) - cy.on('fail', (err) => { - const { lastLog } = this + // cy.on('fail', (err) => { + // const { lastLog } = this - assertLogLength(this.logs, 1) - expect(lastLog.get('error')).to.eq(err) - expect(lastLog.get('state')).to.eq('failed') - expect(err.message).to.eq(stripIndent`\ - \`cy.readFile("foo")\` timed out after waiting \`42ms\`. - `) + // assertLogLength(this.logs, 1) + // expect(lastLog.get('error')).to.eq(err) + // expect(lastLog.get('state')).to.eq('failed') + // expect(err.message).to.eq(stripIndent`\ + // \`cy.readFile("foo")\` timed out after waiting \`42ms\`. + // `) - expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') + // expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') - done() - }) + // done() + // }) - cy.readFile('foo') - }) + // cy.readFile('foo') + // }) }) }) @@ -385,6 +385,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'w', + timeout: 4000, }, ) }) @@ -401,6 +402,7 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', + timeout: 4000, }, ) }) @@ -418,6 +420,7 @@ describe('src/cy/commands/files', () => { { encoding: null, flag: 'w', + timeout: 4000, }, ) }) @@ -434,6 +437,7 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', + timeout: 4000, }, ) }) @@ -485,6 +489,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'a+', + timeout: 4000, }, ) }) @@ -652,53 +657,144 @@ describe('src/cy/commands/files', () => { cy.writeFile('foo.txt', 'contents') }) - it('throws when the write timeout expires', function (done) { - Cypress.backend.callsFake(() => { - return new Cypress.Promise(() => {}) - }) + // it('throws when the write timeout expires', function (done) { + // Cypress.backend.callsFake(() => { + // return new Cypress.Promise(() => {}) + // }) - cy.on('fail', (err) => { - const { lastLog } = this + // cy.on('fail', (err) => { + // const { lastLog } = this - assertLogLength(this.logs, 1) - expect(lastLog.get('error')).to.eq(err) - expect(lastLog.get('state')).to.eq('failed') - expect(err.message).to.eq(stripIndent` - \`cy.writeFile("foo.txt")\` timed out after waiting \`10ms\`. - `) + // assertLogLength(this.logs, 1) + // expect(lastLog.get('error')).to.eq(err) + // expect(lastLog.get('state')).to.eq('failed') + // expect(err.message).to.eq(stripIndent` + // \`cy.writeFile("foo.txt")\` timed out after waiting \`10ms\`. + // `) - expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') + // expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') - done() - }) + // done() + // }) - cy.writeFile('foo.txt', 'contents', { timeout: 10 }) - }) + // cy.writeFile('foo.txt', 'contents', { timeout: 10 }) + // }) - it('uses defaultCommandTimeout config value if option not provided', { - defaultCommandTimeout: 42, - }, function (done) { - Cypress.backend.callsFake(() => { - return new Cypress.Promise(() => { /* Broken promise for timeout */ }) - }) + // it('uses defaultCommandTimeout config value if option not provided', { + // defaultCommandTimeout: 42, + // }, function (done) { + // Cypress.backend.callsFake(() => { + // return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + // }) - cy.on('fail', (err) => { - const { lastLog } = this + // cy.on('fail', (err) => { + // const { lastLog } = this - assertLogLength(this.logs, 1) - expect(lastLog.get('error')).to.eq(err) - expect(lastLog.get('state')).to.eq('failed') - expect(err.message).to.eq(stripIndent` - \`cy.writeFile("foo.txt")\` timed out after waiting \`42ms\`. - `) + // assertLogLength(this.logs, 1) + // expect(lastLog.get('error')).to.eq(err) + // expect(lastLog.get('state')).to.eq('failed') + // expect(err.message).to.eq(stripIndent` + // \`cy.writeFile("foo.txt")\` timed out after waiting \`42ms\`. + // `) - expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') + // expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') - done() - }) + // done() + // }) - cy.writeFile('foo.txt', 'contents') - }) + // cy.writeFile('foo.txt', 'contents') + // }) }) }) + + // describe.only('#readFile-error', () => { + // describe('error case', () => { + // // const bigString = JSON.stringify(Cypress._.times(1000000, '😈'), null, 2) // 72MB + // // const tooBigString = JSON.stringify(Cypress._.times(20000000, '😈'), null, 2) // 72MB + + // afterEach(() => { + // debugger + // // cy.readFile('./fixtures/my-long-file.txt').should('eq', 'foobar') + // cy.readFile('./fixtures/my-short-file.txt').then((content) => { + // expect(content).to.eq('foobar') + // }) + // }) + + // it('writes until timeout', () => { + // cy.visit('/fixtures/dom.html') + // }) + // }) + + // describe('should pass', () => { + // it('writes successfully', () => { + // cy.visit('/fixtures/dom.html') + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + // }) + // }) + // }) + + // describe.only('#writeFile-error', { + // defaultCommandTimeout: 4000, + // }, () => { + // describe('error case', () => { + // const bigString = JSON.stringify(Cypress._.times(1000000, '😈'), null, 2) // 72MB + // const tooBigString = JSON.stringify(Cypress._.times(20000000, '😈'), null, 2) // 72MB + + // afterEach(() => { + // debugger + // // cy.readFile('./fixtures/my-long-file.txt').should('eq', 'foobar') + // cy.writeFile('./fixtures/my-long-file.txt', tooBigString) + // }) + + // it('writes until timeout', () => { + // cy.visit('/fixtures/dom.html') + // }) + // }) + + // describe('read after disconnect', () => { + // it('read successfully', () => { + // cy.visit('/fixtures/dom.html') + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + + // cy.readFile('./fixtures/my-short-file.txt').then((content) => { + // expect(content).to.exist + // }) + // }) + // }) + + // describe('error case repeat', () => { + // const bigString = JSON.stringify(Cypress._.times(1000000, '😈'), null, 2) // 72MB + // const tooBigString = JSON.stringify(Cypress._.times(20000000, '😈'), null, 2) // 72MB + + // afterEach(() => { + // debugger + // // cy.readFile('./fixtures/my-long-file.txt').should('eq', 'foobar') + // cy.writeFile('./fixtures/my-long-file.txt', tooBigString) + // }) + + // it('writes until timeout', () => { + // cy.visit('/fixtures/dom.html') + // }) + // }) + + // describe('read again', () => { + // it('read successfully', () => { + // cy.visit('/fixtures/dom.html') + + // cy.get('body').then((bodyElement) => { + // expect(true).to.equal(true) + // }) + + // cy.readFile('./fixtures/my-short-file.txt').then((content) => { + // expect(content).to.exist + // }) + // }) + // }) + // }) }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 315a2492df7..c1a67930ce4 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -8,7 +8,7 @@ export default (Commands, Cypress, cy) => { async readFile (file, encoding, options = {}) { // We clear the default timeout because we are handling // the timeout ourselves. - cy.clearTimeout() + // cy.clearTimeout() let userOptions = options @@ -51,7 +51,7 @@ export default (Commands, Cypress, cy) => { let result try { - result = await Cypress.backend('read:file', file, _.pick(options, ['encoding'])).timeout(options.timeout) + result = await Cypress.backend('read:file', file, _.pick(options, ['encoding', 'timeout'])) } catch (err) { if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { @@ -113,7 +113,7 @@ export default (Commands, Cypress, cy) => { async writeFile (fileName, contents, encoding, options = {}) { // We clear the default timeout because we are handling // the timeout ourselves. - cy.clearTimeout() + // cy.clearTimeout() let userOptions = options @@ -167,7 +167,7 @@ export default (Commands, Cypress, cy) => { let result try { - result = await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag'])).timeout(options.timeout) + result = await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag', 'timeout'])) } catch (err) { if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { From 84e92f1486f5b6dfd56168dd66c1f0f040537c0e Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Thu, 2 Dec 2021 17:12:06 -0600 Subject: [PATCH 16/25] Linter got me --- .../integration/commands/files_spec.js | 91 ------------------- 1 file changed, 91 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index fa7b72e6753..335fff9034a 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -706,95 +706,4 @@ describe('src/cy/commands/files', () => { // }) }) }) - - // describe.only('#readFile-error', () => { - // describe('error case', () => { - // // const bigString = JSON.stringify(Cypress._.times(1000000, '😈'), null, 2) // 72MB - // // const tooBigString = JSON.stringify(Cypress._.times(20000000, '😈'), null, 2) // 72MB - - // afterEach(() => { - // debugger - // // cy.readFile('./fixtures/my-long-file.txt').should('eq', 'foobar') - // cy.readFile('./fixtures/my-short-file.txt').then((content) => { - // expect(content).to.eq('foobar') - // }) - // }) - - // it('writes until timeout', () => { - // cy.visit('/fixtures/dom.html') - // }) - // }) - - // describe('should pass', () => { - // it('writes successfully', () => { - // cy.visit('/fixtures/dom.html') - - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) - // }) - // }) - // }) - - // describe.only('#writeFile-error', { - // defaultCommandTimeout: 4000, - // }, () => { - // describe('error case', () => { - // const bigString = JSON.stringify(Cypress._.times(1000000, '😈'), null, 2) // 72MB - // const tooBigString = JSON.stringify(Cypress._.times(20000000, '😈'), null, 2) // 72MB - - // afterEach(() => { - // debugger - // // cy.readFile('./fixtures/my-long-file.txt').should('eq', 'foobar') - // cy.writeFile('./fixtures/my-long-file.txt', tooBigString) - // }) - - // it('writes until timeout', () => { - // cy.visit('/fixtures/dom.html') - // }) - // }) - - // describe('read after disconnect', () => { - // it('read successfully', () => { - // cy.visit('/fixtures/dom.html') - - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) - - // cy.readFile('./fixtures/my-short-file.txt').then((content) => { - // expect(content).to.exist - // }) - // }) - // }) - - // describe('error case repeat', () => { - // const bigString = JSON.stringify(Cypress._.times(1000000, '😈'), null, 2) // 72MB - // const tooBigString = JSON.stringify(Cypress._.times(20000000, '😈'), null, 2) // 72MB - - // afterEach(() => { - // debugger - // // cy.readFile('./fixtures/my-long-file.txt').should('eq', 'foobar') - // cy.writeFile('./fixtures/my-long-file.txt', tooBigString) - // }) - - // it('writes until timeout', () => { - // cy.visit('/fixtures/dom.html') - // }) - // }) - - // describe('read again', () => { - // it('read successfully', () => { - // cy.visit('/fixtures/dom.html') - - // cy.get('body').then((bodyElement) => { - // expect(true).to.equal(true) - // }) - - // cy.readFile('./fixtures/my-short-file.txt').then((content) => { - // expect(content).to.exist - // }) - // }) - // }) - // }) }) From 58f247511133e74c12763839bc036e975ba33116 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Thu, 2 Dec 2021 17:38:41 -0600 Subject: [PATCH 17/25] Utilizing server-side timeout for read/writeFile commands. --- packages/driver/src/cy/commands/files.ts | 4 +- packages/server/lib/files.js | 44 ++++++++++++++- packages/server/test/unit/files_spec.js | 69 ++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 4 deletions(-) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index c1a67930ce4..7a77e353c7c 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -8,7 +8,7 @@ export default (Commands, Cypress, cy) => { async readFile (file, encoding, options = {}) { // We clear the default timeout because we are handling // the timeout ourselves. - // cy.clearTimeout() + cy.clearTimeout() let userOptions = options @@ -113,7 +113,7 @@ export default (Commands, Cypress, cy) => { async writeFile (fileName, contents, encoding, options = {}) { // We clear the default timeout because we are handling // the timeout ourselves. - // cy.clearTimeout() + cy.clearTimeout() let userOptions = options diff --git a/packages/server/lib/files.js b/packages/server/lib/files.js index 3add13e81ed..6ae75507926 100644 --- a/packages/server/lib/files.js +++ b/packages/server/lib/files.js @@ -5,13 +5,28 @@ module.exports = { readFile (projectRoot, file, options = {}) { const filePath = path.resolve(projectRoot, file) const readFn = (path.extname(filePath) === '.json' && options.encoding !== null) ? fs.readJsonAsync : fs.readFileAsync + const readFileAbortController = new AbortController() // https://github.com/cypress-io/cypress/issues/1558 // If no encoding is specified, then Cypress has historically defaulted // to `utf8`, because of it's focus on text files. This is in contrast to // NodeJs, which defaults to binary. We allow users to pass in `null` // to restore the default node behavior. - return readFn(filePath, options.encoding === undefined ? 'utf8' : options.encoding) + + const readOptions = { + encoding: options.encoding === undefined ? 'utf8' : options.encoding, + signal: readFileAbortController.signal, + } + + let readFileTimeout + + if (options.timeout !== undefined) { + readFileTimeout = setTimeout(() => { + readFileAbortController.abort() + }, options.timeout) + } + + return readFn(filePath, readOptions) .then((contents) => { return { contents, @@ -20,27 +35,52 @@ module.exports = { }) .catch((err) => { err.filePath = filePath + + if (err.name === 'AbortError') { + err.name = 'TimeoutError' + } + throw err }) + .finally(() => { + clearTimeout(readFileTimeout) + }) }, writeFile (projectRoot, file, contents, options = {}) { const filePath = path.resolve(projectRoot, file) + const writeFileAbortController = new AbortController() const writeOptions = { encoding: options.encoding === undefined ? 'utf8' : options.encoding, flag: options.flag || 'w', + signal: writeFileAbortController.signal, + } + + let writeFileTimeout + + if (options.timeout !== undefined) { + writeFileTimeout = setTimeout(() => { + writeFileAbortController.abort() + }, options.timeout) } return fs.outputFile(filePath, contents, writeOptions) .then(() => { return { - contents, filePath, } }) .catch((err) => { err.filePath = filePath + + if (err.name === 'AbortError') { + err.name = 'TimeoutError' + } + throw err }) + .finally(() => { + clearTimeout(writeFileTimeout) + }) }, } diff --git a/packages/server/test/unit/files_spec.js b/packages/server/test/unit/files_spec.js index af563f79e7f..0ca52df05e6 100644 --- a/packages/server/test/unit/files_spec.js +++ b/packages/server/test/unit/files_spec.js @@ -2,6 +2,7 @@ require('../spec_helper') const config = require(`${root}lib/config`) const files = require(`${root}lib/files`) +const { fs } = require(`${root}lib/util/fs`) const FixturesHelper = require('@tooling/system-tests/lib/fixtures') describe('lib/files', () => { @@ -61,6 +62,42 @@ describe('lib/files', () => { ]) }) }) + + it('aborts readFn execution if not complete within the specified timeout', function () { + const mockTimeoutId = 4567 + + sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { + // execute timeout function synchronously so that abort signal is aborted prior + // to outputFile execution + funcArg() + + return mockTimeoutId + }) + + sinon.stub(global, 'clearTimeout') + + return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt', { timeout: 100 }).catch((err) => { + expect(err.name).to.equal('AbortError') + expect(err.aborted).to.equal(true) + expect(err.filePath).to.include('tests/_fixtures/message.txt') + expect(global.clearTimeout).to.have.been.calledWith(mockTimeoutId) + }) + }) + + it('catches generic errors from readFn and appends filePath', function () { + sinon.stub(fs, 'readFileAsync').callsFake(function mockReadFile (path, options, callback) { + callback(new Error('UnexpectedError: How could this happen'), undefined) + }) + + sinon.stub(global, 'clearTimeout') + + return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt').catch((err) => { + expect(err.message).to.equal('UnexpectedError: How could this happen') + expect(err.aborted).to.equal(undefined) + expect(err.filePath).to.include('tests/_fixtures/message.txt') + expect(global.clearTimeout).to.have.been.calledWith(undefined) + }) + }) }) context('#writeFile', () => { @@ -118,5 +155,37 @@ describe('lib/files', () => { }) }) }) + + it('aborts outputFile execution if not complete within the specified timeout', function () { + sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { + // execute timeout function synchronously so that abort signal is aborted prior + // to outputFile execution + funcArg() + + // returned timeoutId is synchronized with clearTimeout assertion below + return 12345 + }) + + sinon.stub(global, 'clearTimeout') + + return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo', { timeout: 100 }).catch((err) => { + expect(err.name).to.equal('AbortError') + expect(err.aborted).to.equal(true) + expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') + expect(global.clearTimeout).to.have.been.calledWith(12345) + }) + }) + + it('catches generic errors from outputFile and appends filePath', function () { + sinon.stub(fs, 'outputFile').rejects(new Error('UnexpectedError: How could this happen')) + sinon.stub(global, 'clearTimeout') + + return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo').catch((err) => { + expect(err.message).to.equal('UnexpectedError: How could this happen') + expect(err.aborted).to.equal(undefined) + expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') + expect(global.clearTimeout).to.have.been.calledWith(undefined) + }) + }) }) }) From ab7b52dfd802776b9eb30ee50897fc13db668e63 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Thu, 2 Dec 2021 20:27:58 -0600 Subject: [PATCH 18/25] Reverting async/await usage for commands, as async/await introduces potentially impactful overhead --- packages/driver/src/cy/commands/files.ts | 93 +++++++++++------------- 1 file changed, 43 insertions(+), 50 deletions(-) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 7a77e353c7c..d01b898eba7 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -5,7 +5,7 @@ import $errUtils from '../../cypress/error_utils' export default (Commands, Cypress, cy) => { Commands.addAll({ - async readFile (file, encoding, options = {}) { + readFile (file, encoding, options = {}) { // We clear the default timeout because we are handling // the timeout ourselves. cy.clearTimeout() @@ -47,12 +47,9 @@ export default (Commands, Cypress, cy) => { }) } - const verifyAssertions = async () => { - let result - - try { - result = await Cypress.backend('read:file', file, _.pick(options, ['encoding', 'timeout'])) - } catch (err) { + const verifyAssertions = () => { + return Cypress.backend('read:file', file, _.pick(options, 'encoding', 'timeout')) + .catch((err) => { if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, @@ -68,49 +65,47 @@ export default (Commands, Cypress, cy) => { }) } - result = { + return { contents: null, filePath: err.filePath, } - } - - let { contents, filePath } = result - - // https://github.com/cypress-io/cypress/issues/1558 - // We invoke Buffer.from() in order to transform this from an ArrayBuffer - - // which socket.io uses to transfer the file over the websocket - into a - // `Buffer`, which webpack polyfills in the browser. - if (options.encoding === null) { - contents = Buffer.from(contents) - } - - consoleProps['File Path'] = filePath - consoleProps['Contents'] = contents - - return cy.verifyUpcomingAssertions(contents, options, { - ensureExistenceFor: 'subject', - onFail (err) { - if (err.type !== 'existence') { - return - } - - // file exists but it shouldn't - or - file doesn't exist but it should - const errPath = contents ? 'files.existent' : 'files.nonexistent' - const { message, docsUrl } = $errUtils.cypressErrByPath(errPath, { - args: { file, filePath }, - }) + }).then(({ contents, filePath }) => { + // https://github.com/cypress-io/cypress/issues/1558 + // We invoke Buffer.from() in order to transform this from an ArrayBuffer - + // which socket.io uses to transfer the file over the websocket - into a + // `Buffer`, which webpack polyfills in the browser. + if (options.encoding === null) { + contents = Buffer.from(contents) + } - err.message = message - err.docsUrl = docsUrl - }, - onRetry: verifyAssertions, + consoleProps['File Path'] = filePath + consoleProps['Contents'] = contents + + return cy.verifyUpcomingAssertions(contents, options, { + ensureExistenceFor: 'subject', + onFail (err) { + if (err.type !== 'existence') { + return + } + + // file exists but it shouldn't - or - file doesn't exist but it should + const errPath = contents ? 'files.existent' : 'files.nonexistent' + const { message, docsUrl } = $errUtils.cypressErrByPath(errPath, { + args: { file, filePath }, + }) + + err.message = message + err.docsUrl = docsUrl + }, + onRetry: verifyAssertions, + }) }) } return verifyAssertions() }, - async writeFile (fileName, contents, encoding, options = {}) { + writeFile (fileName, contents, encoding, options = {}) { // We clear the default timeout because we are handling // the timeout ourselves. cy.clearTimeout() @@ -164,11 +159,14 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } - let result + return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag', 'timeout')) + .then(({ contents, filePath }) => { + consoleProps['File Path'] = filePath + consoleProps['Contents'] = contents - try { - result = await Cypress.backend('write:file', fileName, contents, _.pick(options, ['encoding', 'flag', 'timeout'])) - } catch (err) { + return null + }) + .catch((err) => { if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, @@ -180,12 +178,7 @@ export default (Commands, Cypress, cy) => { onFail: options._log, args: { cmd: 'writeFile', action: 'write', file: fileName, filePath: err.filePath, error: err.message }, }) - } - - consoleProps['File Path'] = result?.filePath - consoleProps['Contents'] = result?.contents - - return null + }) }, }) } From f6bc87f1964bbee393ebddd56194e060791164eb Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 3 Dec 2021 02:15:41 -0600 Subject: [PATCH 19/25] Adding new error message for disconnections --- packages/driver/src/cy/commands/files.ts | 22 +++++++++++++++---- packages/driver/src/cypress/error_messages.ts | 6 +++++ 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index d01b898eba7..a8f51dc2a3f 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -50,13 +50,20 @@ export default (Commands, Cypress, cy) => { const verifyAssertions = () => { return Cypress.backend('read:file', file, _.pick(options, 'encoding', 'timeout')) .catch((err) => { - if (err.name === 'TimeoutError') { + if (err.name === 'AbortError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, args: { cmd: 'readFile', file, timeout: options.timeout }, }) } + if (err.name === 'SocketDisconnected') { + return $errUtils.throwErrByPath('files.socket_disconnected', { + onFail: options._log, + args: { cmd: 'readFile', file, message: err.message }, + }) + } + // Non-ENOENT errors are not retried if (err.code !== 'ENOENT') { return $errUtils.throwErrByPath('files.unexpected_error', { @@ -69,7 +76,7 @@ export default (Commands, Cypress, cy) => { contents: null, filePath: err.filePath, } - }).then(({ contents, filePath }) => { + }).then(({ filePath, contents }) => { // https://github.com/cypress-io/cypress/issues/1558 // We invoke Buffer.from() in order to transform this from an ArrayBuffer - // which socket.io uses to transfer the file over the websocket - into a @@ -160,20 +167,27 @@ export default (Commands, Cypress, cy) => { } return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag', 'timeout')) - .then(({ contents, filePath }) => { + .then(({ filePath, contents }) => { consoleProps['File Path'] = filePath consoleProps['Contents'] = contents return null }) .catch((err) => { - if (err.name === 'TimeoutError') { + if (err.name === 'AbortError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, args: { cmd: 'writeFile', file: fileName, timeout: options.timeout }, }) } + if (err.name === 'SocketDisconnected') { + return $errUtils.throwErrByPath('files.socket_disconnected', { + onFail: options._log, + args: { cmd: 'writeFile', file: fileName, message: err.message }, + }) + } + return $errUtils.throwErrByPath('files.unexpected_error', { onFail: options._log, args: { cmd: 'writeFile', action: 'write', file: fileName, filePath: err.filePath, error: err.message }, diff --git a/packages/driver/src/cypress/error_messages.ts b/packages/driver/src/cypress/error_messages.ts index 36bc19ee475..61f7b8fed07 100644 --- a/packages/driver/src/cypress/error_messages.ts +++ b/packages/driver/src/cypress/error_messages.ts @@ -493,6 +493,12 @@ export default { docsUrl: `https://on.cypress.io/${_.toLower(obj.cmd)}`, } }, + socket_disconnected (obj) { + return { + message: `${cmd('{{cmd}}', '"{{file}}"')} failed because the socket disconnected due to: \`{{message}}\`.`, + docsUrl: `https://on.cypress.io/${_.toLower(obj.cmd)}`, + } + }, }, fixture: { From 7137e5e31ef89ab4aefa7c3e2d540862e24097b0 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 3 Dec 2021 09:05:13 -0600 Subject: [PATCH 20/25] Stopped overriding AbortError. Updated clearTimeout reason. --- packages/driver/src/cy/commands/files.ts | 18 ++++++++++-------- packages/server/lib/files.js | 8 -------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index a8f51dc2a3f..80d7c0e50ad 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -6,10 +6,6 @@ import $errUtils from '../../cypress/error_utils' export default (Commands, Cypress, cy) => { Commands.addAll({ readFile (file, encoding, options = {}) { - // We clear the default timeout because we are handling - // the timeout ourselves. - cy.clearTimeout() - let userOptions = options if (_.isObject(encoding)) { @@ -47,6 +43,11 @@ export default (Commands, Cypress, cy) => { }) } + // We clear the default timeout because the read:file command will + // perform its own timeout in the server. If the server command times out, + // it will throw an AbortError. + cy.clearTimeout() + const verifyAssertions = () => { return Cypress.backend('read:file', file, _.pick(options, 'encoding', 'timeout')) .catch((err) => { @@ -113,10 +114,6 @@ export default (Commands, Cypress, cy) => { }, writeFile (fileName, contents, encoding, options = {}) { - // We clear the default timeout because we are handling - // the timeout ourselves. - cy.clearTimeout() - let userOptions = options if (_.isObject(encoding)) { @@ -166,6 +163,11 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } + // We clear the default timeout because the read:file command will + // perform its own timeout in the server. If the server command times out, + // it will throw an AbortError. + cy.clearTimeout() + return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag', 'timeout')) .then(({ filePath, contents }) => { consoleProps['File Path'] = filePath diff --git a/packages/server/lib/files.js b/packages/server/lib/files.js index 6ae75507926..ecdaa96e132 100644 --- a/packages/server/lib/files.js +++ b/packages/server/lib/files.js @@ -36,10 +36,6 @@ module.exports = { .catch((err) => { err.filePath = filePath - if (err.name === 'AbortError') { - err.name = 'TimeoutError' - } - throw err }) .finally(() => { @@ -73,10 +69,6 @@ module.exports = { .catch((err) => { err.filePath = filePath - if (err.name === 'AbortError') { - err.name = 'TimeoutError' - } - throw err }) .finally(() => { From bdb06940a8088b11a29fdac0d0beb2f78a8e21cd Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 3 Dec 2021 09:25:06 -0600 Subject: [PATCH 21/25] Using responseTimeout again. Updating tests. --- .../integration/commands/files_spec.js | 172 +++++++++--------- packages/driver/src/cy/commands/files.ts | 4 +- packages/runner-shared/src/event-manager.js | 5 +- 3 files changed, 94 insertions(+), 87 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index 335fff9034a..5d0f7a475ca 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -21,7 +21,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'utf8', timeout: 4000 }, + { encoding: 'utf8', timeout: Cypress.config('responseTimeout') }, ) }) }) @@ -33,7 +33,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'ascii', timeout: 4000 }, + { encoding: 'ascii', timeout: Cypress.config('responseTimeout') }, ) }) }) @@ -49,7 +49,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: null, timeout: 4000 }, + { encoding: null, timeout: Cypress.config('responseTimeout') }, ) }).should('eql', Buffer.from('\n')) }) @@ -145,7 +145,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'readFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.json') - expect(log.get('timeout')).to.eq(Cypress.config('defaultCommandTimeout')) + expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) } }) @@ -158,7 +158,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - defaultCommandTimeout: 50, + responseTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -323,53 +323,55 @@ describe('src/cy/commands/files', () => { cy.readFile('foo.json').should('equal', 'contents') }) - // it('throws when the read timeout expires', function (done) { - // Cypress.backend.callsFake(() => { - // return new Cypress.Promise(() => { /* Broken promise for timeout */ }) - // }) + it('throws when the read timeout expires', function (done) { + const abortError = new Error('read timed out in server') - // cy.on('fail', (err) => { - // const { lastLog } = this + abortError.name = 'AbortError' + Cypress.backend.rejects(abortError) - // assertLogLength(this.logs, 1) - // expect(lastLog.get('error')).to.eq(err) - // expect(lastLog.get('state')).to.eq('failed') - // expect(err.message).to.eq(stripIndent`\ - // \`cy.readFile("foo")\` timed out after waiting \`10ms\`. - // `) + cy.on('fail', (err) => { + const { lastLog } = this - // expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent`\ + \`cy.readFile("foo")\` timed out after waiting \`10ms\`. + `) - // done() - // }) + expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') - // cy.readFile('foo', { timeout: 10 }) - // }) + done() + }) - // it('uses defaultCommandTimeout config value if option not provided', { - // defaultCommandTimeout: 42, - // }, function (done) { - // Cypress.backend.callsFake(() => { - // return new Cypress.Promise(() => { /* Broken promise for timeout */ }) - // }) + cy.readFile('foo', { timeout: 10 }) + }) - // cy.on('fail', (err) => { - // const { lastLog } = this + it('uses responseTimeout config value if option not provided', { + responseTimeout: 42, + }, function (done) { + const abortError = new Error('read timed out in server') - // assertLogLength(this.logs, 1) - // expect(lastLog.get('error')).to.eq(err) - // expect(lastLog.get('state')).to.eq('failed') - // expect(err.message).to.eq(stripIndent`\ - // \`cy.readFile("foo")\` timed out after waiting \`42ms\`. - // `) + abortError.name = 'AbortError' + Cypress.backend.rejects(abortError) - // expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') + cy.on('fail', (err) => { + const { lastLog } = this - // done() - // }) + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent`\ + \`cy.readFile("foo")\` timed out after waiting \`42ms\`. + `) + + expect(err.docsUrl).to.eq('https://on.cypress.io/readfile') - // cy.readFile('foo') - // }) + done() + }) + + cy.readFile('foo') + }) }) }) @@ -385,7 +387,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'w', - timeout: 4000, + timeout: Cypress.config('responseTimeout'), }, ) }) @@ -402,7 +404,7 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: 4000, + timeout: Cypress.config('responseTimeout'), }, ) }) @@ -420,7 +422,7 @@ describe('src/cy/commands/files', () => { { encoding: null, flag: 'w', - timeout: 4000, + timeout: Cypress.config('responseTimeout'), }, ) }) @@ -437,7 +439,7 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: 4000, + timeout: Cypress.config('responseTimeout'), }, ) }) @@ -489,7 +491,7 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'a+', - timeout: 4000, + timeout: Cypress.config('responseTimeout'), }, ) }) @@ -535,7 +537,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'writeFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.txt', 'contents') - expect(log.get('timeout')).to.eq(Cypress.config('defaultCommandTimeout')) + expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) } }) @@ -548,7 +550,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - defaultCommandTimeout: 50, + responseTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -657,53 +659,55 @@ describe('src/cy/commands/files', () => { cy.writeFile('foo.txt', 'contents') }) - // it('throws when the write timeout expires', function (done) { - // Cypress.backend.callsFake(() => { - // return new Cypress.Promise(() => {}) - // }) + it('throws when the write timeout expires', function (done) { + const abortError = new Error('write timed out in server') - // cy.on('fail', (err) => { - // const { lastLog } = this + abortError.name = 'AbortError' + Cypress.backend.rejects(abortError) - // assertLogLength(this.logs, 1) - // expect(lastLog.get('error')).to.eq(err) - // expect(lastLog.get('state')).to.eq('failed') - // expect(err.message).to.eq(stripIndent` - // \`cy.writeFile("foo.txt")\` timed out after waiting \`10ms\`. - // `) + cy.on('fail', (err) => { + const { lastLog } = this - // expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent` + \`cy.writeFile("foo.txt")\` timed out after waiting \`11ms\`. + `) - // done() - // }) + expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') - // cy.writeFile('foo.txt', 'contents', { timeout: 10 }) - // }) + done() + }) - // it('uses defaultCommandTimeout config value if option not provided', { - // defaultCommandTimeout: 42, - // }, function (done) { - // Cypress.backend.callsFake(() => { - // return new Cypress.Promise(() => { /* Broken promise for timeout */ }) - // }) + cy.writeFile('foo.txt', 'contents', { timeout: 11 }) + }) - // cy.on('fail', (err) => { - // const { lastLog } = this + it('uses responseTimeout config value if option not provided', { + responseTimeout: 43, + }, function (done) { + const abortError = new Error('write timed out in server') - // assertLogLength(this.logs, 1) - // expect(lastLog.get('error')).to.eq(err) - // expect(lastLog.get('state')).to.eq('failed') - // expect(err.message).to.eq(stripIndent` - // \`cy.writeFile("foo.txt")\` timed out after waiting \`42ms\`. - // `) + abortError.name = 'AbortError' + Cypress.backend.rejects(abortError) - // expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') + cy.on('fail', (err) => { + const { lastLog } = this - // done() - // }) + assertLogLength(this.logs, 1) + expect(lastLog.get('error')).to.eq(err) + expect(lastLog.get('state')).to.eq('failed') + expect(err.message).to.eq(stripIndent` + \`cy.writeFile("foo.txt")\` timed out after waiting \`43ms\`. + `) + + expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') - // cy.writeFile('foo.txt', 'contents') - // }) + done() + }) + + cy.writeFile('foo.txt', 'contents') + }) }) }) }) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index 80d7c0e50ad..cec7c95b0be 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -21,7 +21,7 @@ export default (Commands, Cypress, cy) => { // to restore the default node behavior. encoding: encoding === undefined ? 'utf8' : encoding, log: true, - timeout: Cypress.config('defaultCommandTimeout'), + timeout: Cypress.config('responseTimeout'), }) const consoleProps = {} @@ -130,7 +130,7 @@ export default (Commands, Cypress, cy) => { encoding: encoding === undefined ? 'utf8' : encoding, flag: userOptions.flag ? userOptions.flag : 'w', log: true, - timeout: Cypress.config('defaultCommandTimeout'), + timeout: Cypress.config('responseTimeout'), }) const consoleProps = {} diff --git a/packages/runner-shared/src/event-manager.js b/packages/runner-shared/src/event-manager.js index 6da675aca9c..103641bd653 100644 --- a/packages/runner-shared/src/event-manager.js +++ b/packages/runner-shared/src/event-manager.js @@ -406,8 +406,11 @@ export const eventManager = { const cb = args.pop() const handleDisconnect = function (disconnectReason) { + const disconnectError = new Error(disconnectReason) + + disconnectError.name = 'SocketDisconnected' cb({ - error: new Error(disconnectReason), + error: disconnectError, }) } From 58d8b2b572c9d9eeb67135ffb6ecd32077aaf957 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 3 Dec 2021 10:04:23 -0600 Subject: [PATCH 22/25] Updating max buffer comment --- packages/server/test/unit/files_spec.js | 4 ---- packages/socket/lib/socket.ts | 11 ++++++----- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/packages/server/test/unit/files_spec.js b/packages/server/test/unit/files_spec.js index 0ca52df05e6..ab95e8e6b79 100644 --- a/packages/server/test/unit/files_spec.js +++ b/packages/server/test/unit/files_spec.js @@ -78,7 +78,6 @@ describe('lib/files', () => { return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt', { timeout: 100 }).catch((err) => { expect(err.name).to.equal('AbortError') - expect(err.aborted).to.equal(true) expect(err.filePath).to.include('tests/_fixtures/message.txt') expect(global.clearTimeout).to.have.been.calledWith(mockTimeoutId) }) @@ -93,7 +92,6 @@ describe('lib/files', () => { return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt').catch((err) => { expect(err.message).to.equal('UnexpectedError: How could this happen') - expect(err.aborted).to.equal(undefined) expect(err.filePath).to.include('tests/_fixtures/message.txt') expect(global.clearTimeout).to.have.been.calledWith(undefined) }) @@ -170,7 +168,6 @@ describe('lib/files', () => { return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo', { timeout: 100 }).catch((err) => { expect(err.name).to.equal('AbortError') - expect(err.aborted).to.equal(true) expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') expect(global.clearTimeout).to.have.been.calledWith(12345) }) @@ -182,7 +179,6 @@ describe('lib/files', () => { return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo').catch((err) => { expect(err.message).to.equal('UnexpectedError: How could this happen') - expect(err.aborted).to.equal(undefined) expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') expect(global.clearTimeout).to.have.been.calledWith(undefined) }) diff --git a/packages/socket/lib/socket.ts b/packages/socket/lib/socket.ts index 1248c09c8a4..e0c6a08a9d3 100644 --- a/packages/socket/lib/socket.ts +++ b/packages/socket/lib/socket.ts @@ -14,12 +14,13 @@ type PatchedServerOptions = ServerOptions & { cookie: { name: string | boolean } class SocketIOServer extends SocketIOBaseServer { constructor (srv: http.Server, opts?: Partial) { - // in socket.io v3, they reduced down the max buffer size - // from 100mb to 1mb, so we reset it back to the previous value - // - // previous commit for reference: - // https://github.com/socketio/engine.io/blame/61b949259ed966ef6fc8bfd61f14d1a2ef06d319/lib/server.js#L29 opts = opts ?? {} + + // the maxHttpBufferSize is used to limit the message size sent over + // the socket. Small values can be used to mitigate exposure to + // denial of service attacks; the default as of v3.0 is 1MB. + // because our server is local, we do not need to arbitrarily limit + // the message size and can use the theoretical maximum value. opts.maxHttpBufferSize = opts.maxHttpBufferSize ?? buffer.constants.MAX_LENGTH super(srv, opts) From 9d149ecfc7be0f42e1b6589a98b7d832cb7a6329 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 3 Dec 2021 16:00:48 -0600 Subject: [PATCH 23/25] Adding global disconnect handler. Updating command_queue error reporting for commands with active logs. --- packages/driver/src/cy/commands/files.ts | 24 +++---------- packages/driver/src/cypress/command_queue.ts | 14 ++++++++ packages/runner-shared/src/event-manager.js | 36 +++++++++++--------- 3 files changed, 38 insertions(+), 36 deletions(-) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index cec7c95b0be..a45603b1abe 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -49,22 +49,15 @@ export default (Commands, Cypress, cy) => { cy.clearTimeout() const verifyAssertions = () => { - return Cypress.backend('read:file', file, _.pick(options, 'encoding', 'timeout')) + return Cypress.backend('read:file', file, _.pick(options, 'encoding', 'timeout')).timeout(options.timeout) .catch((err) => { - if (err.name === 'AbortError') { + if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, args: { cmd: 'readFile', file, timeout: options.timeout }, }) } - if (err.name === 'SocketDisconnected') { - return $errUtils.throwErrByPath('files.socket_disconnected', { - onFail: options._log, - args: { cmd: 'readFile', file, message: err.message }, - }) - } - // Non-ENOENT errors are not retried if (err.code !== 'ENOENT') { return $errUtils.throwErrByPath('files.unexpected_error', { @@ -163,12 +156,12 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } - // We clear the default timeout because the read:file command will + // We clear the default timeout because the write:file command will // perform its own timeout in the server. If the server command times out, // it will throw an AbortError. cy.clearTimeout() - return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag', 'timeout')) + return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag', 'timeout')).timeout(options.timeout) .then(({ filePath, contents }) => { consoleProps['File Path'] = filePath consoleProps['Contents'] = contents @@ -176,20 +169,13 @@ export default (Commands, Cypress, cy) => { return null }) .catch((err) => { - if (err.name === 'AbortError') { + if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { onFail: options._log, args: { cmd: 'writeFile', file: fileName, timeout: options.timeout }, }) } - if (err.name === 'SocketDisconnected') { - return $errUtils.throwErrByPath('files.socket_disconnected', { - onFail: options._log, - args: { cmd: 'writeFile', file: fileName, message: err.message }, - }) - } - return $errUtils.throwErrByPath('files.unexpected_error', { onFail: options._log, args: { cmd: 'writeFile', action: 'write', file: fileName, filePath: err.filePath, error: err.message }, diff --git a/packages/driver/src/cypress/command_queue.ts b/packages/driver/src/cypress/command_queue.ts index b84b1aa4143..f9a4befed02 100644 --- a/packages/driver/src/cypress/command_queue.ts +++ b/packages/driver/src/cypress/command_queue.ts @@ -19,6 +19,11 @@ interface Command { finishLogs(): void } +interface Log { + error(e: any): any + get(key: string): any +} + const __stackReplacementMarker = (fn, ctx, args) => { return fn.apply(ctx, args) } @@ -35,6 +40,15 @@ const commandRunningFailed = (Cypress, state, err) => { } const current = state('current') + let existingLog = _.last(current.attributes.logs) as Log + + // if the cy.current property already has associated logs that + // have not ended, we fail the most recent log + if (existingLog && !existingLog.get('ended')) { + existingLog.error(err) + + return + } return Cypress.log({ end: true, diff --git a/packages/runner-shared/src/event-manager.js b/packages/runner-shared/src/event-manager.js index 103641bd653..1af50a3edaf 100644 --- a/packages/runner-shared/src/event-manager.js +++ b/packages/runner-shared/src/event-manager.js @@ -22,6 +22,24 @@ ws.on('connect', () => { ws.emit('runner:connected') }) +ws.on('disconnect', (reason) => { + if (!Cypress || !Cypress.currentTest) { + return + } + + let message = `Cypress command failed after socket disconnection due to: ${reason}.` + + if (reason === 'transport error' || reason === 'transport close') { + message = `${message}\n\nThis usually happens when your command's arguments exceed the socket's buffer limits.` + } + + const err = new Error(message) + + err.name = 'CypressError' + + Cypress.cy.fail(err, { async: true }) +}) + const driverToReporterEvents = 'paused session:add'.split(' ') const driverToLocalAndReporterEvents = 'run:start run:end'.split(' ') const driverToSocketEvents = 'backend:request automation:request mocha recorder:frame'.split(' ') @@ -403,23 +421,7 @@ export const eventManager = { _.each(driverToSocketEvents, (event) => { Cypress.on(event, (...args) => { - const cb = args.pop() - - const handleDisconnect = function (disconnectReason) { - const disconnectError = new Error(disconnectReason) - - disconnectError.name = 'SocketDisconnected' - cb({ - error: disconnectError, - }) - } - - ws.once('disconnect', handleDisconnect) - - return ws.emit(event, ...args, (...cbArgs) => { - ws.off('disconnect', handleDisconnect) - cb(...cbArgs) - }) + return ws.emit(event, ...args) }) }) From c16be3a2bd93fbd8e5a473eeb0f5f4ecc31979d6 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 3 Dec 2021 16:52:57 -0600 Subject: [PATCH 24/25] Reverting work in progress --- .../integration/commands/files_spec.js | 63 ++++++++---------- packages/driver/src/cy/commands/files.ts | 14 ++-- packages/driver/src/cypress/command_queue.ts | 14 ---- packages/driver/src/cypress/error_messages.ts | 6 -- packages/runner-shared/src/event-manager.js | 18 ----- packages/server/lib/files.js | 36 +--------- packages/server/test/unit/files_spec.js | 65 ------------------- 7 files changed, 35 insertions(+), 181 deletions(-) diff --git a/packages/driver/cypress/integration/commands/files_spec.js b/packages/driver/cypress/integration/commands/files_spec.js index 5d0f7a475ca..d1404357909 100644 --- a/packages/driver/cypress/integration/commands/files_spec.js +++ b/packages/driver/cypress/integration/commands/files_spec.js @@ -10,7 +10,7 @@ const okResponse = { describe('src/cy/commands/files', () => { beforeEach(() => { // call through normally on everything - cy.stub(Cypress, 'backend').callThrough().log(false) + cy.stub(Cypress, 'backend').callThrough() }) describe('#readFile', () => { @@ -21,7 +21,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'utf8', timeout: Cypress.config('responseTimeout') }, + { encoding: 'utf8' }, ) }) }) @@ -33,7 +33,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: 'ascii', timeout: Cypress.config('responseTimeout') }, + { encoding: 'ascii' }, ) }) }) @@ -49,7 +49,7 @@ describe('src/cy/commands/files', () => { expect(Cypress.backend).to.be.calledWith( 'read:file', 'foo.json', - { encoding: null, timeout: Cypress.config('responseTimeout') }, + { encoding: null }, ) }).should('eql', Buffer.from('\n')) }) @@ -145,7 +145,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'readFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.json') - expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) + expect(log.get('timeout')).to.eq(Cypress.config('defaultCommandTimeout')) } }) @@ -158,7 +158,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - responseTimeout: 50, + defaultCommandTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -324,10 +324,9 @@ describe('src/cy/commands/files', () => { }) it('throws when the read timeout expires', function (done) { - const abortError = new Error('read timed out in server') - - abortError.name = 'AbortError' - Cypress.backend.rejects(abortError) + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -347,13 +346,12 @@ describe('src/cy/commands/files', () => { cy.readFile('foo', { timeout: 10 }) }) - it('uses responseTimeout config value if option not provided', { - responseTimeout: 42, + it('uses defaultCommandTimeout config value if option not provided', { + defaultCommandTimeout: 42, }, function (done) { - const abortError = new Error('read timed out in server') - - abortError.name = 'AbortError' - Cypress.backend.rejects(abortError) + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -387,7 +385,6 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'w', - timeout: Cypress.config('responseTimeout'), }, ) }) @@ -404,7 +401,6 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: Cypress.config('responseTimeout'), }, ) }) @@ -422,7 +418,6 @@ describe('src/cy/commands/files', () => { { encoding: null, flag: 'w', - timeout: Cypress.config('responseTimeout'), }, ) }) @@ -439,7 +434,6 @@ describe('src/cy/commands/files', () => { { encoding: 'ascii', flag: 'w', - timeout: Cypress.config('responseTimeout'), }, ) }) @@ -491,7 +485,6 @@ describe('src/cy/commands/files', () => { { encoding: 'utf8', flag: 'a+', - timeout: Cypress.config('responseTimeout'), }, ) }) @@ -537,7 +530,7 @@ describe('src/cy/commands/files', () => { if (attrs.name === 'writeFile') { expect(log.get('state')).to.eq('pending') expect(log.get('message')).to.eq('foo.txt', 'contents') - expect(log.get('timeout')).to.eq(Cypress.config('responseTimeout')) + expect(log.get('timeout')).to.eq(Cypress.config('defaultCommandTimeout')) } }) @@ -550,7 +543,7 @@ describe('src/cy/commands/files', () => { }) describe('errors', { - responseTimeout: 50, + defaultCommandTimeout: 50, }, () => { beforeEach(function () { this.logs = [] @@ -660,10 +653,9 @@ describe('src/cy/commands/files', () => { }) it('throws when the write timeout expires', function (done) { - const abortError = new Error('write timed out in server') - - abortError.name = 'AbortError' - Cypress.backend.rejects(abortError) + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => {}) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -672,7 +664,7 @@ describe('src/cy/commands/files', () => { expect(lastLog.get('error')).to.eq(err) expect(lastLog.get('state')).to.eq('failed') expect(err.message).to.eq(stripIndent` - \`cy.writeFile("foo.txt")\` timed out after waiting \`11ms\`. + \`cy.writeFile("foo.txt")\` timed out after waiting \`10ms\`. `) expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') @@ -680,16 +672,15 @@ describe('src/cy/commands/files', () => { done() }) - cy.writeFile('foo.txt', 'contents', { timeout: 11 }) + cy.writeFile('foo.txt', 'contents', { timeout: 10 }) }) - it('uses responseTimeout config value if option not provided', { - responseTimeout: 43, + it('uses defaultCommandTimeout config value if option not provided', { + defaultCommandTimeout: 42, }, function (done) { - const abortError = new Error('write timed out in server') - - abortError.name = 'AbortError' - Cypress.backend.rejects(abortError) + Cypress.backend.callsFake(() => { + return new Cypress.Promise(() => { /* Broken promise for timeout */ }) + }) cy.on('fail', (err) => { const { lastLog } = this @@ -698,7 +689,7 @@ describe('src/cy/commands/files', () => { expect(lastLog.get('error')).to.eq(err) expect(lastLog.get('state')).to.eq('failed') expect(err.message).to.eq(stripIndent` - \`cy.writeFile("foo.txt")\` timed out after waiting \`43ms\`. + \`cy.writeFile("foo.txt")\` timed out after waiting \`42ms\`. `) expect(err.docsUrl).to.eq('https://on.cypress.io/writefile') diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index a45603b1abe..e20f5485606 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -21,7 +21,7 @@ export default (Commands, Cypress, cy) => { // to restore the default node behavior. encoding: encoding === undefined ? 'utf8' : encoding, log: true, - timeout: Cypress.config('responseTimeout'), + timeout: Cypress.config('defaultCommandTimeout'), }) const consoleProps = {} @@ -43,9 +43,8 @@ export default (Commands, Cypress, cy) => { }) } - // We clear the default timeout because the read:file command will - // perform its own timeout in the server. If the server command times out, - // it will throw an AbortError. + // We clear the default timeout so we can handle + // the timeout ourselves cy.clearTimeout() const verifyAssertions = () => { @@ -123,7 +122,7 @@ export default (Commands, Cypress, cy) => { encoding: encoding === undefined ? 'utf8' : encoding, flag: userOptions.flag ? userOptions.flag : 'w', log: true, - timeout: Cypress.config('responseTimeout'), + timeout: Cypress.config('defaultCommandTimeout'), }) const consoleProps = {} @@ -156,9 +155,8 @@ export default (Commands, Cypress, cy) => { contents = JSON.stringify(contents, null, 2) } - // We clear the default timeout because the write:file command will - // perform its own timeout in the server. If the server command times out, - // it will throw an AbortError. + // We clear the default timeout so we can handle + // the timeout ourselves cy.clearTimeout() return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag', 'timeout')).timeout(options.timeout) diff --git a/packages/driver/src/cypress/command_queue.ts b/packages/driver/src/cypress/command_queue.ts index f9a4befed02..b84b1aa4143 100644 --- a/packages/driver/src/cypress/command_queue.ts +++ b/packages/driver/src/cypress/command_queue.ts @@ -19,11 +19,6 @@ interface Command { finishLogs(): void } -interface Log { - error(e: any): any - get(key: string): any -} - const __stackReplacementMarker = (fn, ctx, args) => { return fn.apply(ctx, args) } @@ -40,15 +35,6 @@ const commandRunningFailed = (Cypress, state, err) => { } const current = state('current') - let existingLog = _.last(current.attributes.logs) as Log - - // if the cy.current property already has associated logs that - // have not ended, we fail the most recent log - if (existingLog && !existingLog.get('ended')) { - existingLog.error(err) - - return - } return Cypress.log({ end: true, diff --git a/packages/driver/src/cypress/error_messages.ts b/packages/driver/src/cypress/error_messages.ts index 61f7b8fed07..36bc19ee475 100644 --- a/packages/driver/src/cypress/error_messages.ts +++ b/packages/driver/src/cypress/error_messages.ts @@ -493,12 +493,6 @@ export default { docsUrl: `https://on.cypress.io/${_.toLower(obj.cmd)}`, } }, - socket_disconnected (obj) { - return { - message: `${cmd('{{cmd}}', '"{{file}}"')} failed because the socket disconnected due to: \`{{message}}\`.`, - docsUrl: `https://on.cypress.io/${_.toLower(obj.cmd)}`, - } - }, }, fixture: { diff --git a/packages/runner-shared/src/event-manager.js b/packages/runner-shared/src/event-manager.js index 1af50a3edaf..714e691ccd6 100644 --- a/packages/runner-shared/src/event-manager.js +++ b/packages/runner-shared/src/event-manager.js @@ -22,24 +22,6 @@ ws.on('connect', () => { ws.emit('runner:connected') }) -ws.on('disconnect', (reason) => { - if (!Cypress || !Cypress.currentTest) { - return - } - - let message = `Cypress command failed after socket disconnection due to: ${reason}.` - - if (reason === 'transport error' || reason === 'transport close') { - message = `${message}\n\nThis usually happens when your command's arguments exceed the socket's buffer limits.` - } - - const err = new Error(message) - - err.name = 'CypressError' - - Cypress.cy.fail(err, { async: true }) -}) - const driverToReporterEvents = 'paused session:add'.split(' ') const driverToLocalAndReporterEvents = 'run:start run:end'.split(' ') const driverToSocketEvents = 'backend:request automation:request mocha recorder:frame'.split(' ') diff --git a/packages/server/lib/files.js b/packages/server/lib/files.js index ecdaa96e132..3add13e81ed 100644 --- a/packages/server/lib/files.js +++ b/packages/server/lib/files.js @@ -5,28 +5,13 @@ module.exports = { readFile (projectRoot, file, options = {}) { const filePath = path.resolve(projectRoot, file) const readFn = (path.extname(filePath) === '.json' && options.encoding !== null) ? fs.readJsonAsync : fs.readFileAsync - const readFileAbortController = new AbortController() // https://github.com/cypress-io/cypress/issues/1558 // If no encoding is specified, then Cypress has historically defaulted // to `utf8`, because of it's focus on text files. This is in contrast to // NodeJs, which defaults to binary. We allow users to pass in `null` // to restore the default node behavior. - - const readOptions = { - encoding: options.encoding === undefined ? 'utf8' : options.encoding, - signal: readFileAbortController.signal, - } - - let readFileTimeout - - if (options.timeout !== undefined) { - readFileTimeout = setTimeout(() => { - readFileAbortController.abort() - }, options.timeout) - } - - return readFn(filePath, readOptions) + return readFn(filePath, options.encoding === undefined ? 'utf8' : options.encoding) .then((contents) => { return { contents, @@ -35,44 +20,27 @@ module.exports = { }) .catch((err) => { err.filePath = filePath - throw err }) - .finally(() => { - clearTimeout(readFileTimeout) - }) }, writeFile (projectRoot, file, contents, options = {}) { const filePath = path.resolve(projectRoot, file) - const writeFileAbortController = new AbortController() const writeOptions = { encoding: options.encoding === undefined ? 'utf8' : options.encoding, flag: options.flag || 'w', - signal: writeFileAbortController.signal, - } - - let writeFileTimeout - - if (options.timeout !== undefined) { - writeFileTimeout = setTimeout(() => { - writeFileAbortController.abort() - }, options.timeout) } return fs.outputFile(filePath, contents, writeOptions) .then(() => { return { + contents, filePath, } }) .catch((err) => { err.filePath = filePath - throw err }) - .finally(() => { - clearTimeout(writeFileTimeout) - }) }, } diff --git a/packages/server/test/unit/files_spec.js b/packages/server/test/unit/files_spec.js index ab95e8e6b79..af563f79e7f 100644 --- a/packages/server/test/unit/files_spec.js +++ b/packages/server/test/unit/files_spec.js @@ -2,7 +2,6 @@ require('../spec_helper') const config = require(`${root}lib/config`) const files = require(`${root}lib/files`) -const { fs } = require(`${root}lib/util/fs`) const FixturesHelper = require('@tooling/system-tests/lib/fixtures') describe('lib/files', () => { @@ -62,40 +61,6 @@ describe('lib/files', () => { ]) }) }) - - it('aborts readFn execution if not complete within the specified timeout', function () { - const mockTimeoutId = 4567 - - sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { - // execute timeout function synchronously so that abort signal is aborted prior - // to outputFile execution - funcArg() - - return mockTimeoutId - }) - - sinon.stub(global, 'clearTimeout') - - return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt', { timeout: 100 }).catch((err) => { - expect(err.name).to.equal('AbortError') - expect(err.filePath).to.include('tests/_fixtures/message.txt') - expect(global.clearTimeout).to.have.been.calledWith(mockTimeoutId) - }) - }) - - it('catches generic errors from readFn and appends filePath', function () { - sinon.stub(fs, 'readFileAsync').callsFake(function mockReadFile (path, options, callback) { - callback(new Error('UnexpectedError: How could this happen'), undefined) - }) - - sinon.stub(global, 'clearTimeout') - - return files.readFile(this.projectRoot, 'tests/_fixtures/message.txt').catch((err) => { - expect(err.message).to.equal('UnexpectedError: How could this happen') - expect(err.filePath).to.include('tests/_fixtures/message.txt') - expect(global.clearTimeout).to.have.been.calledWith(undefined) - }) - }) }) context('#writeFile', () => { @@ -153,35 +118,5 @@ describe('lib/files', () => { }) }) }) - - it('aborts outputFile execution if not complete within the specified timeout', function () { - sinon.stub(global, 'setTimeout').callsFake(function syncTimeout (funcArg) { - // execute timeout function synchronously so that abort signal is aborted prior - // to outputFile execution - funcArg() - - // returned timeoutId is synchronized with clearTimeout assertion below - return 12345 - }) - - sinon.stub(global, 'clearTimeout') - - return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo', { timeout: 100 }).catch((err) => { - expect(err.name).to.equal('AbortError') - expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') - expect(global.clearTimeout).to.have.been.calledWith(12345) - }) - }) - - it('catches generic errors from outputFile and appends filePath', function () { - sinon.stub(fs, 'outputFile').rejects(new Error('UnexpectedError: How could this happen')) - sinon.stub(global, 'clearTimeout') - - return files.writeFile(this.projectRoot, '.projects/write_file.txt', 'foo').catch((err) => { - expect(err.message).to.equal('UnexpectedError: How could this happen') - expect(err.filePath).to.include('.projects/todos/.projects/write_file.txt') - expect(global.clearTimeout).to.have.been.calledWith(undefined) - }) - }) }) }) From bf1c901cd0bb89a3578a00d3416db3d55a7e2294 Mon Sep 17 00:00:00 2001 From: Tyler Biethman Date: Fri, 3 Dec 2021 16:59:05 -0600 Subject: [PATCH 25/25] Missed these timeout options --- packages/driver/src/cy/commands/files.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/driver/src/cy/commands/files.ts b/packages/driver/src/cy/commands/files.ts index e20f5485606..7bf4c2db36c 100644 --- a/packages/driver/src/cy/commands/files.ts +++ b/packages/driver/src/cy/commands/files.ts @@ -48,7 +48,7 @@ export default (Commands, Cypress, cy) => { cy.clearTimeout() const verifyAssertions = () => { - return Cypress.backend('read:file', file, _.pick(options, 'encoding', 'timeout')).timeout(options.timeout) + return Cypress.backend('read:file', file, _.pick(options, 'encoding')).timeout(options.timeout) .catch((err) => { if (err.name === 'TimeoutError') { return $errUtils.throwErrByPath('files.timed_out', { @@ -159,7 +159,7 @@ export default (Commands, Cypress, cy) => { // the timeout ourselves cy.clearTimeout() - return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag', 'timeout')).timeout(options.timeout) + return Cypress.backend('write:file', fileName, contents, _.pick(options, 'encoding', 'flag')).timeout(options.timeout) .then(({ filePath, contents }) => { consoleProps['File Path'] = filePath consoleProps['Contents'] = contents