-
-
Notifications
You must be signed in to change notification settings - Fork 643
Add proper rule tests #1248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add proper rule tests #1248
Conversation
3a23b91 to
0d06a6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
|
@sindresorhus Apologies for the delay on this, I'm a bit occupied with some household work. But, I'll get to this sometime this week or next. |
| const {ruleTester, fixturePath} = createTypeAwareRuleTester({ | ||
| 'package.json': `${packageContent}\n`, | ||
| 'index.d.ts': indexContent, | ||
| 'source/exported-type.d.ts': exportedTypeContent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sindresorhus QQ, out of curiosity, is the code on these on-disk files actually being used anywhere?
From my initial testing:
Create source/exported-type.d.ts file with some arbitrary content:
const {ruleTester, fixturePath} = createTypeAwareRuleTester({
'package.json': `${packageContent}\n`,
'index.d.ts': indexContent,
'source/exported-type.d.ts': 'The code here isn\'t being used anywhere, because the virtual file will overwrite it.',
// ...
});The code that we specify in code is what is being used for linting:
const exportedTypeContent = 'export type ExportedType = {\n\tvalue: string;\n};\n';
ruleTester.run('require-exported-types', requireExportedTypesRule, {
valid: [
{
code: exportedTypeContent,
filename: fixturePath('source/exported-type.d.ts'),
},
],
invalid: [],
});Verified this using the following log in the lint rule:
console.log({
sourceCode: context.sourceCode.text,
});This log prints:
{ sourceCode: 'export type ExportedType = {\n\tvalue: string;\n};\n' }Creating index.d.ts and package.json files on disk makes sense because the individual test cases don't create those files. But having to create files on disk for all test cases beforehand feels odd to me, but maybe that's a standard practice.
Does it make sense to set them to undefined instead?
const {ruleTester, fixturePath} = createTypeAwareRuleTester({
'package.json': `${packageContent}\n`,
'index.d.ts': indexContent,
'source/exported-type.d.ts': undefined,
'source/internal/internal-type.d.ts': undefined,
'source/internal/nested/deep.d.ts': undefined,
'source/private-type.d.ts': undefined,
'source/missing-type.d.ts': undefined,
'source/unexported-multiple.d.ts': undefined,
'source/implementation.ts': undefined,
'test/test.d.ts': undefined,
'lib/test.d.ts': undefined,
});And, in test-utils.js add an empty string fallback:
fs.writeFileSync(absolutePath, content ?? '');There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it would have been better if we had to create only the files that are separate from the ones we're linting, i.e., index.d.ts and package.json.
But it seems like doing that brings in a lot of other complications. Here's all I had to do to get it working:
import {createTypeAwareRuleTester} from './test-utils.js';
import {requireExportedTypesRule} from './require-exported-types.js';
const packageContent = JSON.stringify({
name: 'require-exported-types-fixture',
}, null, '\t');
const indexContent = 'export {type ExportedType} from \'./source/exported-type\';\n';
const exportedTypeContent = 'export type ExportedType = {\n\tvalue: string;\n};\n';
const {ruleTester, fixturePath} = createTypeAwareRuleTester({
'package.json': `${packageContent}\n`,
'index.d.ts': indexContent,
}, {
ruleTester: {
languageOptions: {
parserOptions: {
projectService: {
allowDefaultProject: ['source/*.ts*'],
},
},
},
},
});
const withIndexReference = code => `/// <reference path="${fixturePath('index.d.ts')}" />\n${code}`;
ruleTester.run('require-exported-types', requireExportedTypesRule, {
valid: [
{
code: withIndexReference(exportedTypeContent),
filename: fixturePath('source/exported-type.d.ts'),
},
],
invalid: [],
});Changes:
- Only
index.d.tsandpackage.jsonfiles are being created on disk. - Had to update
allowDefaultProjectto include'source/exported-type.d.ts'. - Had to add
/// <reference path="${fixturePath('index.d.ts')}" />incodeto includeindex.d.ts, otherwiseindex.d.tsseems to not be part of the program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to set them to undefined instead?
Makes sense. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR LGTM, just few comments regarding adding some test cases.
Co-authored-by: Som Shekhar Mukherjee <[email protected]>
Co-authored-by: Som Shekhar Mukherjee <[email protected]>
Co-authored-by: Som Shekhar Mukherjee <[email protected]>
dc633cd to
b60713c
Compare
No description provided.