-
-
Notifications
You must be signed in to change notification settings - Fork 75
Conversation
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.
LGTM, I just have a few minor questions.
tests/integration/utils.js
Outdated
}, | ||
sourceType: sourceType || "script" | ||
}, | ||
cwd: path.join(__dirname) |
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.
If I'm not mistaken, this could just be cwd: __dirname
.
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.
Ah yeah, that used to be nested that's why there is the join there!
Object.assign(cliEngineConfig, overrideConfig); | ||
} | ||
|
||
const cli = new eslint.CLIEngine(cliEngineConfig); |
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.
I think it would be possible to just use eslint.Linter
here:
const linter = new eslint.Linter();
linter.defineParser("typescript-eslint-parser", require("../../parser"));
linter.defineRules(rules);
const messages = linter.verify(
code,
{
parser: 'typescript-eslint-parser',
parserOptions: {
ecmaVersion: 8,
ecmaFeatures: {
jsx: true,
experimentalObjectRestSpread: true,
globalReturn: true
},
sourceType: sourceType || "script"
}
},
{ filename: 'fake-filename.js' }
)
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.
Unless I am mistaken when using the Linter, it is not possible to use extends
. Being able to write integration tests concisely using presets is important IMO.
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.
E.g.
typescript-eslint-parser/tests/integration/typescript.spec.js
Lines 9 to 27 in 9ae4baa
it("should fundamentally work with `eslint:all`", () => { | |
verifyAndAssertMessages( | |
unpad(` | |
"use strict"; | |
const foo: string = "bar"; | |
`), | |
{}, | |
["2:7 'foo' is assigned a value but never used. no-unused-vars"], | |
"script", | |
{ | |
baseConfig: { | |
extends: ["eslint:all"], | |
rules: { | |
"eol-last": "off" | |
} | |
} | |
} | |
); | |
}); |
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.
You're correct that it's not possible to use extends
with Linter, but it doesn't seem like extends
is being used for any of these tests. Were you planning to add it?
(I don't mean to block the PR with this concern -- I'm just bringing it up in case you made a mistake.)
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.
The one I pasted in directly above, line 20?
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.
Oops, I missed that, sorry. (I hadn't realized that this function was also being used from other files.)
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.
Yeah I wasn't a huge fan of the huge single file used by babel-eslint so I extracted it and refactored it within a helper function.
Thanks for the review!
const utils = require("./utils"); | ||
const verifyAndAssertMessages = utils.verifyAndAssertMessages; | ||
|
||
describe("Non-regression tests from babel-eslint", () => { |
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.
Do we need to copy-paste babel-eslint
's license here since the tests are from babel-eslint
?
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.
Yes, definitely
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.
Done
// { "no-unused-vars": 1, "no-undef": 1 }, | ||
// [] | ||
// ); | ||
// }); |
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.
Should these commented tests be removed?
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.
Yeah probably, there are some that will definitely never be relevant, some that I was planning on digging into at some point... Would definitely be fine just getting rid of them though.
9ae4baa
to
8b39912
Compare
I have cleaned up the babel-eslint tests somewhat (all the flow ones now gone, for example), but have left in some commented out tests as they will form a useful basis for the custom Referencer which will come in a follow up PR. |
No description provided.