From c9a503571a4662f6c2d31cabc7fd7819ec388150 Mon Sep 17 00:00:00 2001 From: Nick Harris Date: Tue, 7 Apr 2020 17:09:08 -0700 Subject: [PATCH] Fix: newBasePath should be an absolute path (fixes #12850) (#13078) * Fix: newBasePath should be an absolute path (fixes #12850) * review feedback * PR feeedback, add comments to test --- lib/cli-engine/config-array/ignore-pattern.js | 8 +- .../cli-engine/config-array/ignore-pattern.js | 178 ++++++++++++------ 2 files changed, 123 insertions(+), 63 deletions(-) diff --git a/lib/cli-engine/config-array/ignore-pattern.js b/lib/cli-engine/config-array/ignore-pattern.js index 92690b9f8ae..6eaec4258e1 100644 --- a/lib/cli-engine/config-array/ignore-pattern.js +++ b/lib/cli-engine/config-array/ignore-pattern.js @@ -71,7 +71,13 @@ function getCommonAncestorPath(sourcePaths) { } } - return result || path.sep; + let resolvedResult = result || path.sep; + + // if Windows common ancestor is root of drive must have trailing slash to be absolute. + if (resolvedResult && resolvedResult.endsWith(":") && process.platform === "win32") { + resolvedResult += path.sep; + } + return resolvedResult; } /** diff --git a/tests/lib/cli-engine/config-array/ignore-pattern.js b/tests/lib/cli-engine/config-array/ignore-pattern.js index 443d350a2ed..9d652f3b689 100644 --- a/tests/lib/cli-engine/config-array/ignore-pattern.js +++ b/tests/lib/cli-engine/config-array/ignore-pattern.js @@ -6,6 +6,7 @@ const assert = require("assert"); const path = require("path"); +const sinon = require("sinon"); const { IgnorePattern } = require("../../../../lib/cli-engine/config-array/ignore-pattern"); describe("IgnorePattern", () => { @@ -52,74 +53,127 @@ describe("IgnorePattern", () => { describe("static createIgnore(ignorePatterns)", () => { describe("with two patterns should return a function, and the function", () => { - const cwd = process.cwd(); - const basePath1 = path.join(cwd, "foo/bar"); - const basePath2 = path.join(cwd, "abc/"); - const ignores = IgnorePattern.createIgnore([ - new IgnorePattern(["*.js", "/*.ts", "!a.*", "!/b.*"], basePath1), - new IgnorePattern(["*.js", "/*.ts", "!a.*", "!/b.*"], basePath2) - ]); - const patterns = [ - ["a.js", false], - ["a.ts", false], - ["b.js", false], - ["b.ts", false], - ["c.js", false], - ["c.ts", false], - ["dir/a.js", false], - ["dir/a.ts", false], - ["dir/b.js", false], - ["dir/b.ts", false], - ["dir/c.js", false], - ["dir/c.ts", false], - ["foo/bar/a.js", false], - ["foo/bar/a.ts", false], - ["foo/bar/b.js", false], - ["foo/bar/b.ts", false], - ["foo/bar/c.js", true], - ["foo/bar/c.ts", true], - ["foo/bar/dir/a.js", false], - ["foo/bar/dir/a.ts", false], - ["foo/bar/dir/b.js", true], - ["foo/bar/dir/b.ts", false], - ["foo/bar/dir/c.js", true], - ["foo/bar/dir/c.ts", false], - ["abc/a.js", false], - ["abc/a.ts", false], - ["abc/b.js", false], - ["abc/b.ts", false], - ["abc/c.js", true], - ["abc/c.ts", true], - ["abc/dir/a.js", false], - ["abc/dir/a.ts", false], - ["abc/dir/b.js", true], - ["abc/dir/b.ts", false], - ["abc/dir/c.js", true], - ["abc/dir/c.ts", false] - ]; - - for (const [filename, expected] of patterns) { - it(`should return ${expected} if '${filename}' was given.`, () => { - assert.strictEqual(ignores(path.join(cwd, filename)), expected); + + /** + * performs static createIgnre assertions against the cwd. + * @param {string} cwd cwd to be the base path for assertions + * @returns {void} + */ + function assertions(cwd) { + const basePath1 = path.join(cwd, "foo/bar"); + const basePath2 = path.join(cwd, "abc/"); + const ignores = IgnorePattern.createIgnore([ + new IgnorePattern(["*.js", "/*.ts", "!a.*", "!/b.*"], basePath1), + new IgnorePattern(["*.js", "/*.ts", "!a.*", "!/b.*"], basePath2) + ]); + const patterns = [ + ["a.js", false], + ["a.ts", false], + ["b.js", false], + ["b.ts", false], + ["c.js", false], + ["c.ts", false], + ["dir/a.js", false], + ["dir/a.ts", false], + ["dir/b.js", false], + ["dir/b.ts", false], + ["dir/c.js", false], + ["dir/c.ts", false], + ["foo/bar/a.js", false], + ["foo/bar/a.ts", false], + ["foo/bar/b.js", false], + ["foo/bar/b.ts", false], + ["foo/bar/c.js", true], + ["foo/bar/c.ts", true], + ["foo/bar/dir/a.js", false], + ["foo/bar/dir/a.ts", false], + ["foo/bar/dir/b.js", true], + ["foo/bar/dir/b.ts", false], + ["foo/bar/dir/c.js", true], + ["foo/bar/dir/c.ts", false], + ["abc/a.js", false], + ["abc/a.ts", false], + ["abc/b.js", false], + ["abc/b.ts", false], + ["abc/c.js", true], + ["abc/c.ts", true], + ["abc/dir/a.js", false], + ["abc/dir/a.ts", false], + ["abc/dir/b.js", true], + ["abc/dir/b.ts", false], + ["abc/dir/c.js", true], + ["abc/dir/c.ts", false] + ]; + + for (const [filename, expected] of patterns) { + it(`should return ${expected} if '${filename}' was given.`, () => { + assert.strictEqual(ignores(path.join(cwd, filename)), expected); + }); + } + + it("should return false if '.dot.js' and false was given.", () => { + assert.strictEqual(ignores(path.join(cwd, ".dot.js"), false), true); }); - } - it("should return false if '.dot.js' and false was given.", () => { - assert.strictEqual(ignores(path.join(cwd, ".dot.js"), false), true); - }); + it("should return true if '.dot.js' and true were given.", () => { + assert.strictEqual(ignores(path.join(cwd, ".dot.js"), true), false); + }); - it("should return true if '.dot.js' and true were given.", () => { - assert.strictEqual(ignores(path.join(cwd, ".dot.js"), true), false); - }); + it("should return false if '.dot/foo.js' and false was given.", () => { + assert.strictEqual(ignores(path.join(cwd, ".dot/foo.js"), false), true); + }); + it("should return true if '.dot/foo.js' and true were given.", () => { + assert.strictEqual(ignores(path.join(cwd, ".dot/foo.js"), true), false); + }); + } - it("should return false if '.dot/foo.js' and false was given.", () => { - assert.strictEqual(ignores(path.join(cwd, ".dot/foo.js"), false), true); - }); + assertions(process.cwd()); + + /* + * This will catch regressions of Windows specific issue #12850 when run on CI nodes. + * This runs the full set of assertions for the function returned from IgnorePattern.createIgnore. + * When run on Windows CI nodes the .root drive i.e C:\ will be supplied + * forcing getCommonAncestors to resolve to the root of the drive thus catching any regrssion of 12850. + * When run on *nix CI nodes provides additional coverage on this OS too. + * assertions when run on Windows CI nodes and / on *nix OS + */ + assertions(path.parse(process.cwd()).root); + }); + }); - it("should return true if '.dot/foo.js' and true were given.", () => { - assert.strictEqual(ignores(path.join(cwd, ".dot/foo.js"), true), false); - }); + describe("static createIgnore(ignorePatterns)", () => { + + /* + * This test will catch regressions of Windows specific issue #12850 when run on your local dev box + * irrespective of if you are running a Windows or *nix style OS. + * When running on *nix sinon is used to emulate Windows behaviors of path and platform APIs + * thus ensuring that the Windows specific fix is exercised and any regression is caught. + */ + it("with common ancestor of drive root on windows should not throw", () => { + try { + + /* + * When not on Windows return win32 values so local runs on *nix hit the same code path as on Windows + * thus enabling developers with *nix style OS to catch and debug any future regression of #12850 without + * requiring a Windows based OS. + */ + if (process.platform !== "win32") { + sinon.stub(process, "platform").value("win32"); + sinon.stub(path, "sep").value(path.win32.sep); + sinon.replace(path, "isAbsolute", path.win32.isAbsolute); + } + + const ignores = IgnorePattern.createIgnore([ + new IgnorePattern(["*.js"], "C:\\foo\\bar"), + new IgnorePattern(["*.js"], "C:\\abc\\") + ]); + + // calls to this should not throw when getCommonAncestor returns root of drive + ignores("C:\\abc\\contract.d.ts"); + } finally { + sinon.restore(); + } }); }); });