New: Allow mocking the cwd in rule tester #12443

fa93hws · 2019-10-16T01:10:07Z

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 the CLIEngine and the Linter.
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?

platinumazure

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.

platinumazure · 2019-10-17T14:29:37Z

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.

mysticatea · 2019-10-18T05:45:59Z

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 cwd option to each test pattern, like filename option.

fa93hws · 2019-10-18T05:56:12Z

I'd like to see if we can add cwd option to each test pattern, like filename option.

Should be possible, I will have a try.

fa93hws · 2019-10-18T06:52:04Z

I'd like to see if we can add cwd option to each test pattern, like filename option.

Hi, it seems that I may need to modify the ConfigData to implement this change.
Does that SGTY?
Because there is a settings field in the ConfigData which I believe is a representation of the .eslintrc file.

If cwd is added to the ConfigData, it might not be a bad idea for it to live in the settings which means it will no longer be a specific configuration for the CLIEngine but for the whole eslint?

mysticatea · 2019-10-18T07:31:34Z

No. ConfigData is the form of .eslintrc.* files.

I had imaged that the tester re-creates linter instance for different cwds.

fa93hws · 2019-10-18T09:52:18Z

No. ConfigData is the form of .eslintrc.* files.

I had imaged that the tester re-creates linter instance for different cwds.

Cool, I understand you concern about recreating the linter instance during the test.
I was thinking removing the cwd from the CLIEngine and have that in the .eslintrc.
But then I realize that the CLIEngine can have the option even though it's in the .eslintrc.🤦‍♂️

I will add the cwd property to providedConfig then.

mysticatea · 2019-10-18T10:21:51Z

Sorry, my previous comment may not be clear.

I'm imaging that we can realize cwd option in each test pattern by recreating the linter instance for each cwd values. So probably the this.linter property becomes a map, from cwd to the linter instance.

I don't think that the change of ConfigData is good. It's a core enhancement that is too big to do in this PR.

fa93hws · 2019-10-18T10:37:44Z

Sorry, my previous comment may not be clear.

I'm imaging that we can realize cwd option in each test pattern by recreating the linter instance for each cwd values. So probably the this.linter property becomes a map, from cwd to the linter instance.

I don't think that the change of ConfigData is good. It's a core enhancement that is too big to do in this PR.

Oh I see.
Agree, the change to ConfigData is a bit too large without RFC.

kaicataldo · 2019-12-20T23:43:17Z

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.

mysticatea

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: "", ... }
    ],
})

nzakas · 2020-06-11T18:26:02Z

@fa93hws do you plan on continuing to work on this?

fa93hws · 2020-06-12T01:04:30Z

Ah, sorry I forget about this PR. Will take a look later.

nzakas · 2020-06-12T19:22:01Z

Thanks!

mysticatea · 2020-06-21T05:17:10Z

lib/rule-tester/rule-tester.js

-            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);


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.

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.

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.

fa93hws · 2020-06-21T06:49:42Z

lib/rule-tester/rule-tester.js

+     *   linterMapDevOnly?: Map<string | undefined, Linter>
     * }} test The collection of tests to run.
+     * test.linterMapDevOnly is for testing purpose


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...

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.

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.

For example, similarly lib/linter/linter.js exposes getLinterInternalSlots() function for tests.

nzakas · 2020-07-07T20:21:40Z

@mysticatea have your concerns been addressed?

kaicataldo · 2020-08-14T20:59:58Z

@mysticatea One more friendly ping.

allow mocking `cwd` in `RuleTester` test cases. Patch based on this pull request: eslint/eslint#12443

nzakas · 2021-10-09T00:43:09Z

Closing due to age.

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Oct 16, 2019

platinumazure requested review from mysticatea and kaicataldo and removed request for mysticatea October 17, 2019 14:25

platinumazure approved these changes Oct 17, 2019

View reviewed changes

kaicataldo approved these changes Dec 20, 2019

View reviewed changes

ilyavolodin approved these changes Dec 20, 2019

View reviewed changes

mysticatea suggested changes Dec 23, 2019

View reviewed changes

mysticatea self-assigned this Dec 23, 2019

mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 23, 2019

New: Allow mocking the cwd in rule tester

902e55f

fa93hws force-pushed the eric/allow-cwd-in-rule-tester branch from 9db291e to 902e55f Compare June 20, 2020 06:03

fa93hws requested a review from mysticatea June 20, 2020 06:06

mysticatea reviewed Jun 21, 2020

View reviewed changes

run set rules once

66d011f

fa93hws commented Jun 21, 2020

View reviewed changes

fa93hws requested a review from mysticatea June 21, 2020 06:50

hide linterMapSymbol

78a1393

fa93hws force-pushed the eric/allow-cwd-in-rule-tester branch from d44700f to 78a1393 Compare June 21, 2020 07:50

jarrodldavis added a commit to jarrodldavis/eslint-plugin-tailwindcss that referenced this pull request Nov 2, 2020

patch ESLint

d35c7c2

allow mocking `cwd` in `RuleTester` test cases. Patch based on this pull request: eslint/eslint#12443

jarrodldavis mentioned this pull request Nov 2, 2020

Allow using PostCSS config jarrodldavis/eslint-plugin-tailwindcss#3

Merged

nzakas closed this Oct 9, 2021

eslint-github-bot bot locked and limited conversation to collaborators Apr 8, 2022

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Allow mocking the cwd in rule tester #12443

New: Allow mocking the cwd in rule tester #12443

fa93hws commented Oct 16, 2019 •

edited

platinumazure left a comment

platinumazure commented Oct 17, 2019

mysticatea commented Oct 18, 2019

fa93hws commented Oct 18, 2019

fa93hws commented Oct 18, 2019

mysticatea commented Oct 18, 2019 •

edited

fa93hws commented Oct 18, 2019 •

edited

mysticatea commented Oct 18, 2019

fa93hws commented Oct 18, 2019

kaicataldo commented Dec 20, 2019

mysticatea left a comment

nzakas commented Jun 11, 2020

fa93hws commented Jun 12, 2020

nzakas commented Jun 12, 2020

mysticatea Jun 21, 2020

fa93hws Jun 21, 2020

mysticatea Jun 21, 2020 •

edited

fa93hws Jun 21, 2020

fa93hws Jun 21, 2020

fa93hws Jun 21, 2020

mysticatea Jun 21, 2020 •

edited

mysticatea Jun 21, 2020

nzakas commented Jul 7, 2020

kaicataldo commented Aug 14, 2020

nzakas commented Oct 9, 2021

New: Allow mocking the cwd in rule tester #12443

New: Allow mocking the cwd in rule tester #12443

Conversation

fa93hws commented Oct 16, 2019 • edited

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure commented Oct 17, 2019

mysticatea commented Oct 18, 2019

fa93hws commented Oct 18, 2019

fa93hws commented Oct 18, 2019

mysticatea commented Oct 18, 2019 • edited

fa93hws commented Oct 18, 2019 • edited

mysticatea commented Oct 18, 2019

fa93hws commented Oct 18, 2019

kaicataldo commented Dec 20, 2019

mysticatea left a comment

Choose a reason for hiding this comment

nzakas commented Jun 11, 2020

fa93hws commented Jun 12, 2020

nzakas commented Jun 12, 2020

mysticatea Jun 21, 2020

Choose a reason for hiding this comment

fa93hws Jun 21, 2020

Choose a reason for hiding this comment

mysticatea Jun 21, 2020 • edited

Choose a reason for hiding this comment

fa93hws Jun 21, 2020

Choose a reason for hiding this comment

fa93hws Jun 21, 2020

Choose a reason for hiding this comment

fa93hws Jun 21, 2020

Choose a reason for hiding this comment

mysticatea Jun 21, 2020 • edited

Choose a reason for hiding this comment

mysticatea Jun 21, 2020

Choose a reason for hiding this comment

nzakas commented Jul 7, 2020

kaicataldo commented Aug 14, 2020

nzakas commented Oct 9, 2021

fa93hws commented Oct 16, 2019 •

edited

mysticatea commented Oct 18, 2019 •

edited

fa93hws commented Oct 18, 2019 •

edited

mysticatea Jun 21, 2020 •

edited

mysticatea Jun 21, 2020 •

edited