fix(rule-tester): use cwd option to set base path for tests with file name #10201
Conversation
|
Thanks for the PR, @reduckted! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 1e47a9e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10201 +/- ##
==========================================
+ Coverage 86.18% 86.50% +0.32%
==========================================
Files 430 430
Lines 15029 15088 +59
Branches 4360 4380 +20
==========================================
+ Hits 12952 13052 +100
+ Misses 1725 1679 -46
- Partials 352 357 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Is the code coverage sufficient to get this merged? I feel like what it's reporting is wrong because it says that this statement is uncovered: basePath = path.parse(
path.resolve(basePath ?? process.cwd(), filename),
).root;But that statement is definitely being hit because the tests would fail if it wasn't. 😕 |
Hey thank you for taking a look at this. You're right, this line should be covered. But it's not being run at all somehow. It seems to me that the reason that it's not is this one line in import { RuleTester } from '@typescript-eslint/rule-tester';This appears to resolve to a built asset in the dist/ folder and should be replaced with import { RuleTester } from '../src/RuleTester';That's what See if this fixes the missing coverage issue! |
Ah, brilliant! That's exactly what the problem was. Thanks! |
|
Thank you @reduckted for this amazing fix ! I'm the one who introduced that regression with my changes and I sincerely apologize for any inconvenience it caused... 😔 |
I think for now, let's just go with "I tested it on my machine and it worked" (infamous last words) rather than upgrade eslint for this PR. Related, #1752. |
| errors: [ | ||
| { | ||
| messageId: 'foo', | ||
| suggestions: [{ messageId: 'createError', output: '//' }], |
There was a problem hiding this comment.
This isn't a syntax error, right? Cos a line comment is valid. We need something invalid like ( that will truly crash out the parser.
|
Thanks for your work here! |
|
Awesome! Any chance of a patch release for this? :) |
|
@jtbandes Yes, there will be a release, read this page, which explains: https://typescript-eslint.io/users/releases/ |
|
I meant to get to this before the release but baby stuff got in the way. https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.12.1 |
PR Checklist
RuleTester: cannot read properties ofundefined(reading'parse') #10191Overview
If the
filenamespecified in a test is absolute or steps up at least one directory, then theLinter'scwdoption is used to specify the base path.When creating a
FlatConfigArray, ESLint uses the linter'scwdoption to specify the base path:https://github.com/eslint/eslint/blob/725962731538eaa38d5d78b9e82ce3fccc9762d0/lib/linter/linter.js#L1524
The
cwdoption is specified when creating theLinterobject:https://github.com/eslint/eslint/blob/3a4eaf921543b1cd5d1df4ea9dec02fab396af2a/lib/linter/linter.js#L1304
That means the
RuleTestermay need to create multipleLinterobjects. I've created a Map ofLinterobjects keyed by their base path.For absolute paths and paths that step up a directory, the base path is calculated by resolving the file name relative to either the
tsconfigRootDirspecified in the parser options, orprocess.cwd(). The root of that path is used as the base path. This is equivalent to what #10174 was fixing. For any other paths, the filename does not affect the base path, meaning the behavior prior to #10174 is used.Warning
This bug only exists after ESLint v9.5 (see #10191 (comment)).
I have tested these changes locally against ESLint v9.13.0, but haven't updated the version in this PR. I'm not sure what the desired solution is in regards to testing against multiple ESLint versions.