From a4232cd1d1e6e834cd466b980b9801f455b118b6 Mon Sep 17 00:00:00 2001 From: Keshav Kini Date: Thu, 14 Oct 2021 21:47:35 -0700 Subject: [PATCH] Test envVarsService newline substitution behavior The behavior of interpreting "\n" inside quoted strings in env files was neither documented nor tested, so this commit modifies the unit tests to pin down that behavior. I also added a test for the example I'm proposing to add to the docs in PR microsoft/vscode-docs#4888. I also extended the test for escaping "$" to show that it works even when the "$" isn't part of something that matches SUBST_REGEX (i.e. isn't the beginning of something looking like "${...}"). --- news/3 Code Health/17747.md | 1 + .../variables/envVarsService.unit.test.ts | 56 ++++++++++++------- 2 files changed, 38 insertions(+), 19 deletions(-) create mode 100644 news/3 Code Health/17747.md diff --git a/news/3 Code Health/17747.md b/news/3 Code Health/17747.md new file mode 100644 index 000000000000..8f5d6010f64d --- /dev/null +++ b/news/3 Code Health/17747.md @@ -0,0 +1 @@ +Improve unit tests for envVarsService, in particular the variable substitution logic (Thanks [Keshav Kini](https://github.com/kini)) diff --git a/src/test/common/variables/envVarsService.unit.test.ts b/src/test/common/variables/envVarsService.unit.test.ts index 38a4f678c2da..03a8f2bf212f 100644 --- a/src/test/common/variables/envVarsService.unit.test.ts +++ b/src/test/common/variables/envVarsService.unit.test.ts @@ -406,30 +406,30 @@ SPAM= expect(vars).to.have.property('SPAM', '', 'value is invalid'); }); - test('Outer quotation marks are removed', () => { + test('Outer quotation marks are removed and cause newline substitution', () => { const vars = parseEnvFile(` -SPAM=1234 -HAM='5678' -EGGS="9012" -FOO='"3456"' -BAR="'7890'" -BAZ="\"ABCD" -VAR1="EFGH -VAR2=IJKL" +SPAM=12\\n34 +HAM='56\\n78' +EGGS="90\\n12" +FOO='"34\\n56"' +BAR="'78\\n90'" +BAZ="\"AB\\nCD" +VAR1="EF\\nGH +VAR2=IJ\\nKL" VAR3='MN'OP' VAR4="QR"ST" `); expect(vars).to.not.equal(undefined, 'Variables is undefiend'); expect(Object.keys(vars!)).lengthOf(10, 'Incorrect number of variables'); - expect(vars).to.have.property('SPAM', '1234', 'value is invalid'); - expect(vars).to.have.property('HAM', '5678', 'value is invalid'); - expect(vars).to.have.property('EGGS', '9012', 'value is invalid'); - expect(vars).to.have.property('FOO', '"3456"', 'value is invalid'); - expect(vars).to.have.property('BAR', "'7890'", 'value is invalid'); - expect(vars).to.have.property('BAZ', '"ABCD', 'value is invalid'); - expect(vars).to.have.property('VAR1', '"EFGH', 'value is invalid'); - expect(vars).to.have.property('VAR2', 'IJKL"', 'value is invalid'); + expect(vars).to.have.property('SPAM', '12\\n34', 'value is invalid'); + expect(vars).to.have.property('HAM', '56\n78', 'value is invalid'); + expect(vars).to.have.property('EGGS', '90\n12', 'value is invalid'); + expect(vars).to.have.property('FOO', '"34\n56"', 'value is invalid'); + expect(vars).to.have.property('BAR', "'78\n90'", 'value is invalid'); + expect(vars).to.have.property('BAZ', '"AB\nCD', 'value is invalid'); + expect(vars).to.have.property('VAR1', '"EF\\nGH', 'value is invalid'); + expect(vars).to.have.property('VAR2', 'IJ\\nKL"', 'value is invalid'); // TODO: Should the outer marks be left? expect(vars).to.have.property('VAR3', "MN'OP", 'value is invalid'); @@ -515,6 +515,22 @@ PYTHONPATH=${REPO}/foo:${REPO}/bar \n\ ); }); + test('Example from docs', () => { + const vars = parseEnvFile( + '\ +VAR1=abc \n\ +VAR2_A="${VAR1}\\ndef" \n\ +VAR2_B="${VAR1}\\n"def \n\ + ', + ); + + expect(vars).to.not.equal(undefined, 'Variables is undefined'); + expect(Object.keys(vars!)).lengthOf(3, 'Incorrect number of variables'); + expect(vars).to.have.property('VAR1', 'abc', 'value is invalid'); + expect(vars).to.have.property('VAR2_A', 'abc\ndef', 'value is invalid'); + expect(vars).to.have.property('VAR2_B', '"abc\\n"def', 'value is invalid'); + }); + test('Curly braces are required for substitution', () => { const vars = parseEnvFile('\ SPAM=1234 \n\ @@ -592,20 +608,22 @@ PYTHONPATH=${PYTHONPATH}:${REPO}/bar \n\ ); }); - test('Substitution may be escaped', () => { + test('"$" may be escaped', () => { const vars = parseEnvFile( '\ SPAM=1234 \n\ EGGS=\\${SPAM}/foo:\\${SPAM}/bar \n\ HAM=$ ... $$ \n\ +FOO=foo\\$bar \n\ ', ); expect(vars).to.not.equal(undefined, 'Variables is undefiend'); - expect(Object.keys(vars!)).lengthOf(3, 'Incorrect number of variables'); + expect(Object.keys(vars!)).lengthOf(4, 'Incorrect number of variables'); expect(vars).to.have.property('SPAM', '1234', 'value is invalid'); expect(vars).to.have.property('EGGS', '${SPAM}/foo:${SPAM}/bar', 'value is invalid'); expect(vars).to.have.property('HAM', '$ ... $$', 'value is invalid'); + expect(vars).to.have.property('FOO', 'foo$bar', 'value is invalid'); }); test('base substitution variables', () => {