From 05d4274370ca7e296c66005c94cb3adef4544034 Mon Sep 17 00:00:00 2001 From: Donovan Moini Date: Fri, 31 Jul 2020 14:10:41 -0700 Subject: [PATCH 1/3] fix: issue with array environment variables not being handled correctly (#6810) Signed-off-by: Donovan Moini --- packages/server/lib/util/coerce.js | 19 ++++++++++++++++++- packages/server/test/unit/args_spec.js | 9 +++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/packages/server/lib/util/coerce.js b/packages/server/lib/util/coerce.js index 4794d9dbe12c..013fde2aa546 100644 --- a/packages/server/lib/util/coerce.js +++ b/packages/server/lib/util/coerce.js @@ -7,12 +7,29 @@ const isValue = (value) => { } } +const toArray = (value) => { + if (typeof (value) !== 'string' || (value[0] !== '[' && value[value.length - 1] !== ']')) { + return + } + + const arr = value.substring(1, value.length - 1).split(',') + + // The default `toString` array method returns one string containing each array element separated + // by commas, but without '[' or ']'. If an environment variable is intended to be an array, it + // will begin and end with '[' and ']' respectively. To correctly compare the value argument to + // the value in `process.env`, the `toString` method must be updated to include '[' and ']'. + arr.toString = () => `[${arr.join(',')}]` + + return arr +} + module.exports = (value) => { const num = _.toNumber(value) const bool = toBoolean(value) + const arr = toArray(value) return _ - .chain([num, bool]) + .chain([num, bool, arr]) .find(isValue(value)) .defaultTo(value) .value() diff --git a/packages/server/test/unit/args_spec.js b/packages/server/test/unit/args_spec.js index 657ddfeeb062..640f4268891a 100644 --- a/packages/server/test/unit/args_spec.js +++ b/packages/server/test/unit/args_spec.js @@ -169,6 +169,15 @@ describe('lib/util/args', () => { hash: '769e98018', }) }) + + // https://github.com/cypress-io/cypress/issues/6810 + it('handles values that are arrays', function () { + const options = this.setup('--env', 'foo="[bar1,bar2,bar3]"') + + expect(options.config.env).to.deep.eq({ + foo: '[bar1|bar2|bar3]', + }) + }) }) context('--reporterOptions', () => { From 15f48c9e2558e8cab844456f989d7eac7963862c Mon Sep 17 00:00:00 2001 From: Donovan Moini Date: Wed, 5 Aug 2020 14:17:55 -0700 Subject: [PATCH 2/3] test: added unit tests for coerce.js Signed-off-by: Donovan Moini --- packages/server/test/unit/coerce_spec.js | 69 ++++++++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 packages/server/test/unit/coerce_spec.js diff --git a/packages/server/test/unit/coerce_spec.js b/packages/server/test/unit/coerce_spec.js new file mode 100644 index 000000000000..1baad202dfe0 --- /dev/null +++ b/packages/server/test/unit/coerce_spec.js @@ -0,0 +1,69 @@ +require('../spec_helper') + +const coerce = require(`${root}lib/util/coerce`) +const getProcessEnvVars = require(`${root}lib/config`).getProcessEnvVars + +describe('lib/util/coerce', () => { + beforeEach(function () { + this.env = process.env + }) + + afterEach(function () { + process.env = this.env + }) + + context('coerce', () => { + it('coerces string', () => { + expect(coerce('foo')).to.eq('foo') + }) + + it('coerces string from process.env', () => { + process.env['CYPRESS_STRING'] = 'bar' + const cypressEnvVar = getProcessEnvVars(process.env) + + expect(coerce(cypressEnvVar)).to.deep.include({ STRING: 'bar' }) + }) + + it('coerces number', () => { + expect(coerce('123')).to.eq(123) + }) + + // NOTE: When exporting shell variables, they are saved in `process.env` as strings, hence why + // all `process.env` variables are assigned as strings in these unit tests + it('coerces number from process.env', () => { + process.env['CYPRESS_NUMBER'] = '8000' + const cypressEnvVar = getProcessEnvVars(process.env) + + expect(coerce(cypressEnvVar)).to.deep.include({ NUMBER: 8000 }) + }) + + it('coerces boolean', () => { + expect(coerce('true')).to.be.true + }) + + it('coerces boolean from process.env', () => { + process.env['CYPRESS_BOOLEAN'] = 'false' + const cypressEnvVar = getProcessEnvVars(process.env) + + expect(coerce(cypressEnvVar)).to.deep.include({ BOOLEAN: false }) + }) + + it('coerces array', () => { + expect(coerce('[foo,bar]')).to.have.members(['foo', 'bar']) + }) + + it('coerces array from process.env', () => { + process.env['CYPRESS_ARRAY'] = '[google.com,yahoo.com]' + const cypressEnvVar = getProcessEnvVars(process.env) + + const coercedCypressEnvVar = coerce(cypressEnvVar) + + expect(coercedCypressEnvVar).to.have.keys('ARRAY') + expect(coercedCypressEnvVar['ARRAY']).to.have.members(['google.com', 'yahoo.com']) + }) + + it('defaults value with multiple types to string', () => { + expect(coerce('123foo456')).to.eq('123foo456') + }) + }) +}) From 2643d806deaf2a516b47247f2985c169436f383b Mon Sep 17 00:00:00 2001 From: Donovan Moini Date: Thu, 6 Aug 2020 03:03:52 -0700 Subject: [PATCH 3/3] refactor: improved readability of code for coerce and added more comments Signed-off-by: Donovan Moini --- packages/server/lib/util/coerce.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/server/lib/util/coerce.js b/packages/server/lib/util/coerce.js index 013fde2aa546..e883d1a847d5 100644 --- a/packages/server/lib/util/coerce.js +++ b/packages/server/lib/util/coerce.js @@ -7,17 +7,24 @@ const isValue = (value) => { } } +// https://github.com/cypress-io/cypress/issues/6810 const toArray = (value) => { - if (typeof (value) !== 'string' || (value[0] !== '[' && value[value.length - 1] !== ']')) { + const valueIsNotStringOrArray = typeof (value) !== 'string' || (value[0] !== '[' && value[value.length - 1] !== ']') + + if (valueIsNotStringOrArray) { return } - const arr = value.substring(1, value.length - 1).split(',') + // '[foo,bar]' => ['foo', 'bar'] + const convertStringToArray = () => value.substring(1, value.length - 1).split(',') + const arr = convertStringToArray() // The default `toString` array method returns one string containing each array element separated // by commas, but without '[' or ']'. If an environment variable is intended to be an array, it // will begin and end with '[' and ']' respectively. To correctly compare the value argument to // the value in `process.env`, the `toString` method must be updated to include '[' and ']'. + // Default `toString()` on array: ['foo', 'bar'].toString() => 'foo,bar' + // Custom `toString()` on array: ['foo', 'bar'].toString() => '[foo,bar]' arr.toString = () => `[${arr.join(',')}]` return arr