Skip to content
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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 36 additions & 14 deletions lib/rule-tester/rule-tester.js
Expand Up @@ -120,7 +120,8 @@ const RuleTesterParameters = [
"filename",
"options",
"errors",
"output"
"output",
"cwd"
];

/*
Expand Down Expand Up @@ -337,7 +338,6 @@ class RuleTester {
* @type {Object}
*/
this.rules = {};
this.linter = new Linter();
}

/**
Expand Down Expand Up @@ -416,21 +416,30 @@ class RuleTester {
* @param {Function} rule The rule to test.
* @param {{
* valid: (ValidTestCase | string)[],
* invalid: InvalidTestCase[]
* invalid: InvalidTestCase[],
* linterMapDevOnly?: Map<string | undefined, Linter>
* }} test The collection of tests to run.
* test.linterMapDevOnly is for testing purpose
Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* @returns {void}
*/
run(ruleName, rule, test) {

const testerConfig = this.testerConfig,
requiredScenarios = ["valid", "invalid"],
scenarioErrors = [],
linter = this.linter;
rules = this.rules;

if (lodash.isNil(test) || typeof test !== "object") {
throw new TypeError(`Test Scenarios for rule ${ruleName} : Could not find test scenario object`);
}

/**
* Linter map, cwd -> Linter instance
* Injection of value in `test` is for test purpose.
* @type {Map<string | undefined, Linter>}
*/
const { linterMapDevOnly: linterMap = new Map() } = test;

requiredScenarios.forEach(scenarioType => {
if (lodash.isNil(test[scenarioType])) {
scenarioErrors.push(`Could not find any ${scenarioType} test scenarios`);
Expand All @@ -443,20 +452,32 @@ class RuleTester {
].concat(scenarioErrors).join("\n"));
}

/**
* get linter from linterMap according to cwd
* @param {string} [cwd] cwd
* @returns {Linter} Linter instance
* @private
*/
function getLinter(cwd) {
if (!linterMap.has(cwd)) {
const linter = new Linter({ cwd });

linter.defineRule(ruleName, Object.assign({}, rule, {
linter.defineRule(ruleName, Object.assign({}, rule, {

// Create a wrapper rule that freezes the `context` properties.
create(context) {
freezeDeeply(context.options);
freezeDeeply(context.settings);
freezeDeeply(context.parserOptions);
// Create a wrapper rule that freezes the `context` properties.
create(context) {
freezeDeeply(context.options);
freezeDeeply(context.settings);
freezeDeeply(context.parserOptions);

return (typeof rule === "function" ? rule : rule.create)(context);
return (typeof rule === "function" ? rule : rule.create)(context);
}
}));
linter.defineRules(rules);
linterMap.set(cwd, linter);
}
}));

linter.defineRules(this.rules);
return linterMap.get(cwd);
}

/**
* Run the rule for the given item
Expand All @@ -467,6 +488,7 @@ class RuleTester {
function runRuleForItem(item) {
let config = lodash.cloneDeep(testerConfig),
code, filename, output, beforeAST, afterAST;
const linter = getLinter(item.cwd);

if (typeof item === "string") {
code = item;
Expand Down
29 changes: 26 additions & 3 deletions tests/lib/rule-tester/rule-tester.js
Expand Up @@ -10,6 +10,7 @@
const sinon = require("sinon"),
EventEmitter = require("events"),
{ RuleTester } = require("../../../lib/rule-tester"),
{ Linter } = require("../../../lib/linter"),
assert = require("chai").assert,
nodeAssert = require("assert"),
{ noop } = require("lodash");
Expand Down Expand Up @@ -751,8 +752,15 @@ describe("RuleTester", () => {
});

it("should pass-through the parser to the rule", () => {
const spy = sinon.spy(ruleTester.linter, "verify");

const linter = new Linter();
const spy = sinon.spy(linter, "verify");
const linterMap = new Map([
// eslint-disable-next-line no-undefined
[undefined, linter]
]);

// ruleTester = new RuleTester({ linterMapDevOnly: linterMap });
ruleTester = new RuleTester();
ruleTester.run("no-eval", require("../../fixtures/testers/rule-tester/no-eval"), {
valid: [
{
Expand All @@ -765,7 +773,8 @@ describe("RuleTester", () => {
parser: require.resolve("esprima"),
errors: [{ line: 1 }]
}
]
],
linterMapDevOnly: linterMap
});
assert.strictEqual(spy.args[1][1].parser, require.resolve("esprima"));
});
Expand Down Expand Up @@ -890,6 +899,20 @@ describe("RuleTester", () => {
}, /Property "env" is the wrong type./u);
});

it("should pass-through the cwd to the linter", () => {
ruleTester.run("some-random-rule", ctx => {

assert.strictEqual(ctx.getCwd(), "myCwd");
return {};
}, {
valid: [{
code: "var test = 'foo'",
cwd: "myCwd"
}],
invalid: []
});
});

it("should pass-through the tester config to the rule", () => {
ruleTester = new RuleTester({
globals: { test: true }
Expand Down