-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
New: Allow mocking the cwd in rule tester #12443
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.
This looks good to me. Thanks for contributing!
I have requested reviews from the team members that merged the original cwd
PR, just to make sure this looks good from their side as well.
Also, I don't see a strong need to send this through the RFC process-- if anything, this could be seen as an addition to the previous RFC. But other team members may disagree. |
Thank you for your contribution! I'm not sure if we need RFC for testers. I'm OK to advance this without RFCs. I'd like to see if we can add |
Should be possible, I will have a try. |
Hi, it seems that I may need to modify the If |
No. I had imaged that the tester re-creates linter instance for different |
Cool, I understand you concern about recreating the linter instance during the test. I will add the |
Sorry, my previous comment may not be clear. I'm imaging that we can realize I don't think that the change of |
Oh I see. |
I'm assuming @platinumazure supports this, but it would be great if you could leave a 👍. We'll need one more 👍 and a champion to mark this as accepted. |
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'd be a champion.
However, my stance is this comment; it's better if we can configure the cwd
in each pattern. For example:
new RuleTester().run("my-rule", rule, {
valie: [
{ code: "", cwd: "", filename: "", ... }
],
invalie: [
{ code: "", errors: [], cwd: "", filename: "", ... }
],
})
@fa93hws do you plan on continuing to work on this? |
Ah, sorry I forget about this PR. Will take a look later. |
Thanks! |
9db291e
to
902e55f
Compare
lib/rule-tester/rule-tester.js
Outdated
create(context) { | ||
freezeDeeply(context.options); | ||
freezeDeeply(context.settings); | ||
freezeDeeply(context.parserOptions); | ||
linter.defineRule(ruleName, Object.assign({}, rule, { | ||
|
||
return (typeof rule === "function" ? rule : rule.create)(context); | ||
} | ||
})); | ||
// Create a wrapper rule that freezes the `context` properties. | ||
create(context) { | ||
freezeDeeply(context.options); | ||
freezeDeeply(context.settings); | ||
freezeDeeply(context.parserOptions); | ||
|
||
linter.defineRules(this.rules); | ||
return (typeof rule === "function" ? rule : rule.create)(context); | ||
} | ||
})); | ||
linter.defineRules(rules); |
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.
Thank you for the update!
Originally, the tester runs this process one time. Therefore, I think we can move these lines into if (!linterMap.has(cwd)) { ... }
block.
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 the rules won't be set if the tester is ran multiple times.
Say
ruleTester.run('rule-a', ...);
ruleTester.run('rule-b', ...);
This is because linterMap.has(cwd)
gives true
at the second time.
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.
Oh, I got it. Thank you for the explanation.
How about clearing linterMap
at the top of the run()
method?
My concern is low performance because getLinter()
will be call on every test pattern. I know several rules that have 10k 5k test patterns.
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.
Will do
lib/rule-tester/rule-tester.js
Outdated
* linterMapDevOnly?: Map<string | undefined, Linter> | ||
* }} test The collection of tests to run. | ||
* test.linterMapDevOnly is for testing purpose |
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.
There is one unit test that need access to linter
to verify the parser
.
Can't think of a better way to inject the linter
...
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.
It's doable to have linterMap
to be a class property.
But I think it would be better not to do so, because it's used in run
method only.
An additional method parameter is better than a class property in my point of view.
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.
Removing the instance field sounds nice.
I like using Symbol("linterMapDevOnly")
in that case. Our public API doesn't contain the symbol and we use it only in tests.
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.
For example, similarly lib/linter/linter.js
exposes getLinterInternalSlots()
function for tests.
d44700f
to
78a1393
Compare
@mysticatea have your concerns been addressed? |
@mysticatea One more friendly ping. |
allow mocking `cwd` in `RuleTester` test cases. Patch based on this pull request: eslint/eslint#12443
Closing due to age. |
What is the purpose of this pull request? (put an "X" next to item)
[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
#12389 adds the
cwd
to theCLIEngine
and theLinter
.However it's not possible to mock it in the
ruleTester
.This PR aims to solve this issue.
Is there anything you'd like reviewers to focus on?
N/A
B.T.W. Does that need to go through the RFC process?