From 653739bb5c0bf210402aa7837441a4a4af6dbf92 Mon Sep 17 00:00:00 2001 From: Gleb Bahmutov Date: Mon, 22 Jun 2020 14:34:59 -0400 Subject: [PATCH] Handle --project "" command line argument (#7744) Co-authored-by: Jennifer Shehane --- cli/__snapshots__/errors_spec.js | 2 + cli/lib/cypress.js | 12 +++++ cli/lib/errors.js | 31 ++++++++++--- cli/lib/exec/run.js | 61 ++++++++++++++++++++++---- cli/test/lib/cypress_spec.js | 12 +++++ cli/test/lib/exec/run_spec.js | 22 ++++++++++ packages/server/lib/util/args.js | 29 +++++++++--- packages/server/test/unit/args_spec.js | 43 ++++++++++++++++++ 8 files changed, 193 insertions(+), 19 deletions(-) diff --git a/cli/__snapshots__/errors_spec.js b/cli/__snapshots__/errors_spec.js index e04b953cee2e..f97265efbf98 100644 --- a/cli/__snapshots__/errors_spec.js +++ b/cli/__snapshots__/errors_spec.js @@ -35,6 +35,7 @@ exports['errors individual has the following errors 1'] = [ "incompatibleHeadlessFlags", "invalidCacheDirectory", "invalidCypressEnv", + "invalidRunProjectPath", "invalidSmokeTestDisplayError", "missingApp", "missingDependency", @@ -44,6 +45,7 @@ exports['errors individual has the following errors 1'] = [ "removed", "smokeTestFailure", "unexpected", + "unknownError", "versionMismatch" ] diff --git a/cli/lib/cypress.js b/cli/lib/cypress.js index ad49ce60b143..25ffa5fe9c9f 100644 --- a/cli/lib/cypress.js +++ b/cli/lib/cypress.js @@ -9,13 +9,25 @@ const run = require('./exec/run') const util = require('./util') const cypressModuleApi = { + /** + * Opens Cypress GUI + * @see https://on.cypress.io/module-api#cypress-open + */ open (options = {}) { options = util.normalizeModuleOptions(options) return open.start(options) }, + /** + * Runs Cypress tests in the current project + * @see https://on.cypress.io/module-api#cypress-run + */ run (options = {}) { + if (!run.isValidProject(options.project)) { + return Promise.reject(new Error(`Invalid project path parameter: ${options.project}`)) + } + options = util.normalizeModuleOptions(options) return tmp.fileAsync() diff --git a/cli/lib/errors.js b/cli/lib/errors.js index 233b0ac855b3..d0e5334d6eee 100644 --- a/cli/lib/errors.js +++ b/cli/lib/errors.js @@ -9,13 +9,36 @@ const state = require('./tasks/state') const docsUrl = 'https://on.cypress.io' const requiredDependenciesUrl = `${docsUrl}/required-dependencies` +const runDocumentationUrl = `${docsUrl}/cypress-run` // TODO it would be nice if all error objects could be enforced via types // to only have description + solution properties const hr = '----------' +const genericErrorSolution = stripIndent` + Search for an existing issue or open a GitHub issue at + + ${chalk.blue(util.issuesUrl)} +` + // common errors Cypress application can encounter +const unknownError = { + description: 'Unknown Cypress CLI error', + solution: genericErrorSolution, +} + +const invalidRunProjectPath = { + description: 'Invalid --project path', + solution: stripIndent` + Please provide a valid project path. + + Learn more about ${chalk.cyan('cypress run')} at: + + ${chalk.blue(runDocumentationUrl)} + `, +} + const failedDownload = { description: 'The Cypress App could not be downloaded.', solution: stripIndent` @@ -26,11 +49,7 @@ const failedDownload = { const failedUnzip = { description: 'The Cypress App could not be unzipped.', - solution: stripIndent` - Search for an existing issue or open a GitHub issue at - - ${chalk.blue(util.issuesUrl)} - `, + solution: genericErrorSolution, } const missingApp = (binaryDir) => { @@ -390,6 +409,7 @@ module.exports = { getError, hr, errors: { + unknownError, nonZeroExitCodeXvfb, missingXvfb, missingApp, @@ -408,5 +428,6 @@ module.exports = { smokeTestFailure, childProcessKilled, incompatibleHeadlessFlags, + invalidRunProjectPath, }, } diff --git a/cli/lib/exec/run.js b/cli/lib/exec/run.js index 68b5e219149f..f173465c3f63 100644 --- a/cli/lib/exec/run.js +++ b/cli/lib/exec/run.js @@ -6,11 +6,60 @@ const spawn = require('./spawn') const verify = require('../tasks/verify') const { exitWithError, errors } = require('../errors') -// maps options collected by the CLI -// and forms list of CLI arguments to the server +/** + * Throws an error with "details" property from + * "errors" object. + * @param {Object} details - Error details + */ +const throwInvalidOptionError = (details) => { + if (!details) { + details = errors.unknownError + } + + // throw this error synchronously, it will be caught later on and + // the details will be propagated to the promise chain + const err = new Error() + + err.details = details + throw err +} + +/** + * Typically a user passes a string path to the project. + * But "cypress open" allows using `false` to open in global mode, + * and the user can accidentally execute `cypress run --project false` + * which should be invalid. + */ +const isValidProject = (v) => { + if (typeof v === 'boolean') { + return false + } + + if (v === '' || v === 'false' || v === 'true') { + return false + } + + return true +} + +/** + * Maps options collected by the CLI + * and forms list of CLI arguments to the server. + * + * Note: there is lightweight validation, with errors + * thrown synchronously. + * + * @returns {string[]} list of CLI arguments + */ const processRunOptions = (options = {}) => { debug('processing run options %o', options) + if (!isValidProject(options.project)) { + debug('invalid project option %o', { project: options.project }) + + return throwInvalidOptionError(errors.invalidRunProjectPath) + } + const args = ['--run-project', options.project] if (options.browser) { @@ -55,12 +104,7 @@ const processRunOptions = (options = {}) => { if (options.headless) { if (options.headed) { - // throw this error synchronously, it will be caught later on and - // the details will be propagated to the promise chain - const err = new Error() - - err.details = errors.incompatibleHeadlessFlags - throw err + return throwInvalidOptionError(errors.incompatibleHeadlessFlags) } args.push('--headed', !options.headless) @@ -123,6 +167,7 @@ const processRunOptions = (options = {}) => { module.exports = { processRunOptions, + isValidProject, // resolves with the number of failed tests start (options = {}) { _.defaults(options, { diff --git a/cli/test/lib/cypress_spec.js b/cli/test/lib/cypress_spec.js index ac4c85daff73..000575b60dc2 100644 --- a/cli/test/lib/cypress_spec.js +++ b/cli/test/lib/cypress_spec.js @@ -148,6 +148,18 @@ describe('cypress', function () { }) }) + it('rejects if project is an empty string', () => { + return expect(cypress.run({ project: '' })).to.be.rejected + }) + + it('rejects if project is true', () => { + return expect(cypress.run({ project: true })).to.be.rejected + }) + + it('rejects if project is false', () => { + return expect(cypress.run({ project: false })).to.be.rejected + }) + it('passes quiet: true', () => { const opts = { quiet: true, diff --git a/cli/test/lib/exec/run_spec.js b/cli/test/lib/exec/run_spec.js index 69df0f1fa71a..4142654d92a1 100644 --- a/cli/test/lib/exec/run_spec.js +++ b/cli/test/lib/exec/run_spec.js @@ -15,6 +15,28 @@ describe('exec run', function () { }) context('.processRunOptions', function () { + it('allows string --project option', () => { + const args = run.processRunOptions({ + project: '/path/to/project', + }) + + expect(args).to.deep.equal(['--run-project', '/path/to/project']) + }) + + it('throws an error for empty string --project', () => { + expect(() => run.processRunOptions({ project: '' })).to.throw() + }) + + it('throws an error for boolean --project', () => { + expect(() => run.processRunOptions({ project: false })).to.throw() + expect(() => run.processRunOptions({ project: true })).to.throw() + }) + + it('throws an error for --project "false" or "true"', () => { + expect(() => run.processRunOptions({ project: 'false' })).to.throw() + expect(() => run.processRunOptions({ project: 'true' })).to.throw() + }) + it('passes --browser option', () => { const args = run.processRunOptions({ browser: 'test browser', diff --git a/packages/server/lib/util/args.js b/packages/server/lib/util/args.js index 348a43f33039..43184b043ea6 100644 --- a/packages/server/lib/util/args.js +++ b/packages/server/lib/util/args.js @@ -34,17 +34,27 @@ const normalizeBackslash = (s) => { return s } +/** + * remove stray double quote from runProject and other path properties + * due to bug in NPM passing arguments with backslash at the end + * @see https://github.com/cypress-io/cypress/issues/535 + * + */ const normalizeBackslashes = (options) => { - // remove stray double quote from runProject and other path properties - // due to bug in NPM passing arguments with - // backslash at the end - // https://github.com/cypress-io/cypress/issues/535 // these properties are paths and likely to have backslash on Windows const pathProperties = ['runProject', 'project', 'appPath', 'execPath', 'configFile'] pathProperties.forEach((property) => { - if (options[property]) { + // sometimes a string parameter might get parsed into a boolean + // for example "--project ''" will be transformed in "project: true" + // which we should treat as undefined + if (typeof options[property] === 'string') { options[property] = normalizeBackslash(options[property]) + } else { + // configFile is a special case that can be set to false + if (property !== 'configFile') { + delete options[property] + } } }) @@ -148,6 +158,8 @@ const sanitizeAndConvertNestedArgs = (str, argname) => { } module.exports = { + normalizeBackslashes, + toObject (argv) { debug('argv array: %o', argv) @@ -210,7 +222,12 @@ module.exports = { let { spec } = options const { env, config, reporterOptions, outputPath, tag } = options - const project = options.project || options.runProject + let project = options.project || options.runProject + + // only accept project if it is a string + if (typeof project !== 'string') { + project = undefined + } if (spec) { const resolvePath = (p) => { diff --git a/packages/server/test/unit/args_spec.js b/packages/server/test/unit/args_spec.js index a09082c159bf..d7be0988e085 100644 --- a/packages/server/test/unit/args_spec.js +++ b/packages/server/test/unit/args_spec.js @@ -24,6 +24,37 @@ describe('lib/util/args', () => { }) }) + context('normalizeBackslashes', () => { + it('sets non-string properties to undefined', () => { + const input = { + // string properties + project: true, + appPath: '/foo/bar', + // this option can be string or false + configFile: false, + // unknown properties will be preserved + somethingElse: 42, + } + const output = argsUtil.normalizeBackslashes(input) + + expect(output).to.deep.equal({ + appPath: '/foo/bar', + configFile: false, + somethingElse: 42, + }) + }) + + it('handles empty project path string', () => { + const input = { + project: '', + } + const output = argsUtil.normalizeBackslashes(input) + + // empty project path remains + expect(output).to.deep.equal(input) + }) + }) + context('--project', () => { it('sets projectRoot', function () { const projectRoot = path.resolve(cwd, './foo/bar') @@ -31,6 +62,18 @@ describe('lib/util/args', () => { expect(options.projectRoot).to.eq(projectRoot) }) + + it('is undefined if not specified', function () { + const options = this.setup() + + expect(options.projectRoot).to.eq(undefined) + }) + + it('handles bool project parameter', function () { + const options = this.setup('--project', true) + + expect(options.projectRoot).to.eq(undefined) + }) }) context('--run-project', () => {