Fix: newBasePath should be an absolute path (fixes #12850)

[nickharris]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

fixes #12850

Is there anything you'd like reviewers to focus on?

[mysticatea]

Thank you for your work.

Hmm. It may be a different opinion from the other team members, but I think that we should not stub the internal of IgnorePattern class.

Because we have CI for each environment, I think that the important here is to add tests for variable cases. For example,

• full different paths (/aaa/foo.js vs /bbb/bar.js, or c:\foo.js vs d:\bar.js)
• different paths except for the top-level directory (/aaa/foo.js vs /aaa/bar.js, or c:\foo.js vs c:\bar.js)

If the drive letters are a special concept, we can add special tests only on the win32 platform, instead of mocking. (for example, (process.platform === "win32" ? describe : xdescribe)("Drive Letter", () => { /* special tests */ }))

That said, if the other team members want it, I don't oppose stubbing.

[nickharris]

@mysticatea thankyou for your feedback. The scenario of adding tests across patterns starting with root of the drive and without stubbing anything within IgnorePattern is already covered in the code i added at line 132 - assertions(path.parse(process.cwd()).root); the function assertions is your original set of test patterns and since we pass the .root of the drive it that means means:

additional coverage on Windows CI nodes has already been added for following paths by line 132:
c:\a.js
c:\a.ts
c:\b.js
c:\b.ts
c:\c.js
c:\c.ts
c:\dir\a.js
c:\dir\a.ts
c:\dir\b.js
c:\dir\b.ts
c:\dir\c.js
c:\dir\c.ts
c:\foo\bar\a.js
c:\foo\bar\a.ts
c:\foo\bar\b.js
c:\foo\bar\b.ts
c:\foo\bar\c.js
c:\foo\bar\c.ts
c:\foo\bar\dir\a.js
c:\foo\bar\dir\a.ts
c:\foo\bar\dir\b.js
c:\foo\bar\dir\b.ts
c:\foo\bar\dir\c.js
c:\foo\bar\dir\c.ts
c:\abc\a.js
c:\abc\a.ts
c:\abc\b.js
c:\abc\b.ts
c:\abc\c.js
c:\abc\c.ts
c:\abc\dir\a.js
c:\abc\dir\a.ts
c:\abc\dir\b.js
c:\abc\dir\b.ts
c:\abc\dir\c.js
c:\abc\dir\c.ts
c:.dot.js
c:.dot\foo.js

additional coverage on *nix CI nodes has already been added by line 132 for:
/a.js
/a.ts
/b.js
/b.ts
/c.js
/c.ts
/dir/a.js
/dir/a.ts
/dir/b.js
/dir/b.ts
/dir/c.js
/dir/c.ts
/foo/bar/a.js
/foo/bar/a.ts
/foo/bar/b.js
/foo/bar/b.ts
/foo/bar/c.js
/foo/bar/c.ts
/foo/bar/dir/a.js
/foo/bar/dir/a.ts
/foo/bar/dir/b.js
/foo/bar/dir/b.ts
/foo/bar/dir/c.js
/foo/bar/dir/c.ts
/abc/a.js
/abc/a.ts
/abc/b.js
/abc/b.ts
/abc/c.js
/abc/c.ts
/abc/dir/a.js
/abc/dir/a.ts
/abc/dir/b.js
/abc/dir/b.ts
/abc/dir/c.js
/abc/dir/c.ts
/.dot.js
/.dot/foo.js

I think this change has more then enough coverage added to both test the change and catch any future regressions. From my point of view there is nothing more to be done here. Let me know if you think otherwise.

Intention is not to hold up this PR.

[kaicataldo]

My intention isn't to hold up this PR. If we have adequate coverage and others are okay with the current tests, I'll defer to them.

[nzakas]

@nickharris as it seems several team members are a bit confused about what is covered, could you add some comments in the test file reiterating what you’ve explained in the comments on this PR?

[nickharris]

@nzakas sure thing - done in latest commit. Thanks all!

[kaicataldo]

LGTM, thanks for sticking with this.

[nickharris]

@kaicataldo no problems at all!

[nzakas]

Awesome, thanks so much.

[nickharris]

anything else required from me here?

[nzakas]

@nickharris - not at this time. We tend to leave PRs open a bit just to make sure other teams members get a chance to chime in, too.