From fbd87fd54212a1fc4bdf8735ca9fd2dd2ef589c3 Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Mon, 23 Jan 2023 21:11:45 -0500 Subject: [PATCH] feat: ability to test rule errors and invalid schemas per RFC-103 --- docs/src/integrate/nodejs-api.md | 31 ++- lib/linter/linter.js | 2 + lib/rule-tester/rule-tester.js | 142 +++++++++++++- lib/shared/config-validator.js | 17 +- tests/lib/rule-tester/rule-tester.js | 270 +++++++++++++++++++++++++++ 5 files changed, 450 insertions(+), 12 deletions(-) diff --git a/docs/src/integrate/nodejs-api.md b/docs/src/integrate/nodejs-api.md index 853abdc57b8d..16241560cbac 100644 --- a/docs/src/integrate/nodejs-api.md +++ b/docs/src/integrate/nodejs-api.md @@ -796,7 +796,30 @@ ruleTester.run("my-rule", rule, { code: "var invalidVariable = true", errors: [{ message: /^Unexpected.+variable/ }] } - ] + ], + + // Optional array for testing invalid rule options or custom exceptions thrown by a rule. + fatal: [ + // Test case for invalid rule options. Useful for complex schemas. + { + // `code` can be omitted as it's irrelevant when testing the schema. + options: [{ foo: true }], + error: { + // Only one property in this error object is required. + name: 'SchemaValidationError', // Error class name. + message: 'Value {"foo":true} should NOT have additional properties.', // Error message. Can be provided as string or RegExp. + }, + }, + + // Test case for a custom exception thrown by the rule. + { + code: 'for(const x of [1, 2, 3]) {}', + error: { + name: 'NotYetImplementedError', + message: 'Not implemented', + }, + }, + ], }); ``` @@ -810,9 +833,9 @@ The `RuleTester#run()` method is used to run the tests. It should be passed the * The name of the rule (string) * The rule object itself (see ["working with rules"](../extend/custom-rules)) -* An object containing `valid` and `invalid` properties, each of which is an array containing test cases. +* An object containing the following test case array properties: `valid`, `invalid`, `fatal` (optional) -A test case is an object with the following properties: +Valid and invalid test cases are objects with the following properties: * `name` (string, optional): The name to use for the test case, to make it easier to find * `code` (string, required): The source code that the rule should be run on @@ -836,6 +859,8 @@ In addition to the properties above, invalid test cases can also have the follow If a string is provided as an error instead of an object, the string is used to assert the `message` of the error. * `output` (string, required if the rule fixes code): Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the `--fix` command line flag). If this is `null`, asserts that none of the reported problems suggest autofixes. +Fatal test cases (which are optional) are the same as invalid test cases, except that `code` is optional (it may be irrelevant when testing rule options), and there's an `error` object instead of an `errors` array. The `error` object should include one or both of the `message` of the error and the error (exception) class `name`. Fatal test cases can be used to test custom errors thrown by the rule, or invalid rule options (in which case the error `name` will be `SchemaValidationError`, and the `message` will be come from JSON Schema -- note that strings from JSON Schema are subject to change with future upgrades). + Any additional properties of a test case will be passed directly to the linter as config options. For example, a test case can have a `parserOptions` property to configure parser behavior: ```js diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 233cbed5b5cc..d296f7e1ba79 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -1340,6 +1340,7 @@ class Linter { providedOptions.physicalFilename ); } catch (err) { + err.messageForTest = err.message; // Original exception message without extra helper text for asserting in fatal test cases. err.message += `\nOccurred while linting ${options.filename}`; debug("An error occurred while traversing"); debug("Filename:", options.filename); @@ -1640,6 +1641,7 @@ class Linter { providedOptions.physicalFilename ); } catch (err) { + err.messageForTest = err.message; // Original exception message without extra helper text for asserting in fatal test cases. err.message += `\nOccurred while linting ${options.filename}`; debug("An error occurred while traversing"); debug("Filename:", options.filename); diff --git a/lib/rule-tester/rule-tester.js b/lib/rule-tester/rule-tester.js index 8518299d0b04..caa8b0daf273 100644 --- a/lib/rule-tester/rule-tester.js +++ b/lib/rule-tester/rule-tester.js @@ -18,6 +18,9 @@ * { code: "{code}", errors: {numErrors} }, * { code: "{code}", errors: ["{errorMessage}"] }, * { code: "{code}", options: {options}, globals: {globals}, parser: "{parser}", settings: {settings}, errors: [{ message: "{errorMessage}", type: "{errorNodeType}"}] } + * ], + * fatal: [ + * { code: "{code}", options: {options}, globals: {globals}, parser: "{parser}", settings: {settings}, error: { message: "{errorMessage}", name: "Error"} } * ] * }); * @@ -96,6 +99,19 @@ const { SourceCode } = require("../source-code"); * @property {boolean} [only] Run only this test case or the subset of test cases with this property. */ +/** + * A test case that is expected to trigger a fatal error / exception. + * @typedef {Object} FatalTestCase + * @property {string} [name] Name for the test case. + * @property {string} [code] Code for the test case. + * @property {TestCaseFatalError} error Expected error/exception. + * @property {any[]} [options] Options for the test case. + * @property {{ [name: string]: any }} [settings] Settings for the test case. + * @property {string} [filename] The fake filename for the test case. Useful for rules that make assertion about filenames. + * @property {LanguageOptions} [languageOptions] The language options to use in the test case. + * @property {boolean} [only] Run only this test case or the subset of test cases with this property. + */ + /** * A description of a reported error used in a rule tester test. * @typedef {Object} TestCaseError @@ -108,6 +124,13 @@ const { SourceCode } = require("../source-code"); * @property {number} [endLine] The 1-based line number of the reported end location. * @property {number} [endColumn] The 1-based column number of the reported end location. */ + +/** + * A description of an error/exception from a fatal rule tester test case. + * @typedef {Object} TestCaseFatalError + * @property {string | RegExp} [message] Error message. + * @property {string} [name] Error class name. + */ /* eslint-enable jsdoc/valid-types -- https://github.com/jsdoc-type-pratt-parser/jsdoc-type-pratt-parser/issues/4#issuecomment-778805577 */ //------------------------------------------------------------------------------ @@ -130,6 +153,7 @@ const RuleTesterParameters = [ "code", "filename", "options", + "error", "errors", "output", "only" @@ -151,6 +175,15 @@ const errorObjectParameters = new Set([ ]); const friendlyErrorObjectParameterList = `[${[...errorObjectParameters].map(key => `'${key}'`).join(", ")}]`; +/* + * All allowed property names in fatal error objects. + */ +const fatalErrorObjectParameters = new Set([ + "message", + "name" +]); +const friendlyFatalErrorObjectParameterList = `[${[...fatalErrorObjectParameters].map(key => `'${key}'`).join(", ")}]`; + /* * All allowed property names in suggestion objects. */ @@ -468,8 +501,8 @@ class RuleTester { /** * Adds the `only` property to a test to run it in isolation. - * @param {string | ValidTestCase | InvalidTestCase} item A single test to run by itself. - * @returns {ValidTestCase | InvalidTestCase} The test with `only` set. + * @param {string | ValidTestCase | InvalidTestCase | FatalTestCase} item A single test to run by itself. + * @returns {ValidTestCase | InvalidTestCase | FatalTestCase} The test with `only` set. */ static only(item) { if (typeof item === "string") { @@ -522,7 +555,8 @@ class RuleTester { * @param {Function} rule The rule to test. * @param {{ * valid: (ValidTestCase | string)[], - * invalid: InvalidTestCase[] + * invalid: InvalidTestCase[], + * fatal?: FatalTestCase[] * }} test The collection of tests to run. * @throws {TypeError|Error} If non-object `test`, or if a required * scenario of the given type is missing. @@ -679,7 +713,31 @@ class RuleTester { } } - validate(config, "rule-tester", id => (id === ruleName ? rule : null)); + if (item.error) { + + // Inside a fatal test case, which is expected to throw an error, so surround schema validation with try/catch. + try { + validate(config, "rule-tester", id => (id === ruleName ? rule : null)); + } catch (err) { + if (err.messageForTest) { + + // If this was a testable exception, return it so it can be compared against in the test case. + return { + messages: [{ + ruleId: ruleName, + fatal: true, + message: err.messageForTest, + name: err.name === "Error" ? err.constructor.name : err.name + }] + }; + } + + // Not one of the relevant exceptions for testing. + throw err; + } + } else { + validate(config, "rule-tester", id => (id === ruleName ? rule : null)); + } // Verify the code. const { getComments } = SourceCode.prototype; @@ -688,6 +746,22 @@ class RuleTester { try { SourceCode.prototype.getComments = getCommentsDeprecation; messages = linter.verify(code, config, filename); + } catch (err) { + if (item.error && err.messageForTest) { + + // If this was a testable exception and we're executing a fatal test case, return it so the error can be compared against in the test case. + return { + messages: [{ + ruleId: ruleName, + fatal: true, + message: err.messageForTest, + name: err.name === "Error" ? err.constructor.name : err.name + }] + }; + } + + // Not testing an exception. + throw err; } finally { SourceCode.prototype.getComments = getComments; } @@ -1019,6 +1093,55 @@ class RuleTester { assertASTDidntChange(result.beforeAST, result.afterAST); } + /** + * Check if the template is fatal or not. + * All fatal cases go through this. + * @param {string|Object} item Item to run the rule against + * @returns {void} + * @private + */ + function testFatalTemplate(item) { + if (item.code) { + assert.ok(typeof item.code === "string", "Optional test case property 'code' must be a string"); + } + if (item.name) { + assert.ok(typeof item.name === "string", "Optional test case property 'name' must be a string"); + } + assert.ok(typeof item.error === "object", + "Test case property 'error' must be an object"); + + assert.ok(typeof item.error.name === "string" || typeof item.error.message === "string" || item.error.message instanceof RegExp, + "Test case property 'error' object must provide a string 'name' property or string/regexp 'message' property"); + + assert.ok(typeof item.code === "string" || typeof item.options !== "undefined", "Test case must contain at least one of `code` and `options`"); + + const result = runRuleForItem(item); + + assert.ok(result.messages.length === 1 && result.messages[0].fatal === true, "A fatal test case must throw an exception"); + + const errorActual = result.messages[0]; + const errorExpected = item.error; + + Object.keys(errorExpected).forEach(propertyName => { + assert.ok( + fatalErrorObjectParameters.has(propertyName), + `Invalid error property name '${propertyName}'. Expected one of ${friendlyFatalErrorObjectParameterList}.` + ); + }); + + if (hasOwnProperty(errorExpected, "name")) { + assert.strictEqual( + errorActual.name, + errorExpected.name, + `Fatal error name should be "${errorExpected.name}" but got "${errorActual.name}" instead.` + ); + } + + if (hasOwnProperty(errorExpected, "message")) { + assertMessageMatches(errorActual.message, errorExpected.message); + } + } + /* * This creates a mocha test suite and pipes all supplied info through * one of the templates above. @@ -1045,6 +1168,17 @@ class RuleTester { ); }); }); + + this.constructor.describe("fatal", () => { + (test.fatal || []).forEach((fatal, index) => { + this.constructor[fatal.only ? "itOnly" : "it"]( + sanitize(fatal.name || fatal.code || `(Test Case #${index + 1})`), + () => { + testFatalTemplate(fatal); + } + ); + }); + }); }); } } diff --git a/lib/shared/config-validator.js b/lib/shared/config-validator.js index 47353ac4814b..96d48369457d 100644 --- a/lib/shared/config-validator.js +++ b/lib/shared/config-validator.js @@ -127,6 +127,14 @@ function validateRuleSchema(rule, localOptions) { } } +/** Exception thrown when schema validation fails because a rule is provided invalid options. */ +class SchemaValidationError extends Error { + constructor(message) { + super(message); + this.name = "SchemaValidationError"; + } +} + /** * Validates a rule's options against its schema. * @param {{create: Function}|null} rule The rule that the config is being validated for @@ -146,12 +154,11 @@ function validateRuleOptions(rule, ruleId, options, source = null) { } } catch (err) { const enhancedMessage = `Configuration for rule "${ruleId}" is invalid:\n${err.message}`; + const enhancedMessageWithSource = source ? `${source}:\n\t${enhancedMessage}` : enhancedMessage; + const schemaValidationError = new SchemaValidationError(enhancedMessageWithSource); - if (typeof source === "string") { - throw new Error(`${source}:\n\t${enhancedMessage}`); - } else { - throw new Error(enhancedMessage); - } + schemaValidationError.messageForTest = err.message.trim(); // Original exception message without extra helper text for asserting in fatal test cases. + throw schemaValidationError; } } diff --git a/tests/lib/rule-tester/rule-tester.js b/tests/lib/rule-tester/rule-tester.js index a36edafd4096..5d25c5efe01a 100644 --- a/tests/lib/rule-tester/rule-tester.js +++ b/tests/lib/rule-tester/rule-tester.js @@ -2230,6 +2230,267 @@ describe("RuleTester", () => { }); }); + describe("fatal test cases", () => { + const rule = { + meta: { schema: [] }, + create: context => ({ + Literal(node) { + + /** */ + class CustomError extends Error {} // Custom Error class. + if (node.value === "foo") { + throw new CustomError("Some custom error message"); + } + if (node.value === "bar") { + context.report(node, "no baz"); + } + } + }) + }; + const ruleWithoutCustomErrorClass = { + meta: { schema: [] }, + create: context => ({ + Literal(node) { + + const error = new Error("Some custom error message"); // Standard Error class. + + error.name = "CustomError"; + + if (node.value === "foo") { + throw error; + } + if (node.value === "bar") { + context.report(node, "no baz"); + } + } + }) + }; + + it("should handle custom rule error exception", () => { + + ruleTester.run("rule", rule, { + valid: ["'baz'"], + invalid: [ + { code: "'bar'", errors: [{ message: "no baz" }] } + ], + fatal: [ + { + code: "'foo'", + error: { + + // both `message` and `name + message: "Some custom error message", + name: "CustomError" + } + }, + { + code: "'foo'", + error: { + + // omit `message` + name: "CustomError" + } + }, + { + code: "'foo'", + error: { + + // omit `name` + message: "Some custom error message" + } + }, + { + code: "'foo'", + error: { + + // `message` as RegExp + message: /Some custom error message/u, + name: "CustomError" + } + } + ] + }); + + ruleTester.run("ruleWithoutCustomErrorClass", ruleWithoutCustomErrorClass, { + valid: ["'baz'"], + invalid: [ + { code: "'bar'", errors: [{ message: "no baz" }] } + ], + fatal: [ + { + code: "'foo'", + error: { + + // both `message` and `name + message: "Some custom error message", + name: "CustomError" + } + } + ] + }); + }); + + it("should handle a schema validation error", () => { + ruleTester.run("no-bar", rule, { + valid: ["foo()"], + invalid: [ + { code: "'bar'", errors: [{ message: "no baz" }] } + ], + fatal: [ + { + code: "'foo'", + options: [{ foo: true }], + error: { + + // both `message` and `name + message: 'Value [{"foo":true}] should NOT have more than 0 items.', + name: "SchemaValidationError" + } + }, + { + code: "'foo'", + options: [{ foo: true }], + error: { + + // omit `message` + name: "SchemaValidationError" + } + }, + { + code: "'foo'", + options: [{ foo: true }], + error: { + + // omit `name` + message: 'Value [{"foo":true}] should NOT have more than 0 items.' + } + }, + { + + // omit `code` + options: [{ foo: true }], + error: { + + // omit `name` + message: 'Value [{"foo":true}] should NOT have more than 0 items.' + } + } + ] + }); + }); + + it("throws when fatal test case does not match error thrown", () => { + + // error `name` does not match + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ code: "'foo'", error: { name: "FakeErrorName" } }] + }); + }, 'Fatal error name should be "FakeErrorName" but got "CustomError" instead.'); + + // error `message` does not match + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ code: "'foo'", error: { message: "Fake Message" } }] + }); + }); + + /* + * TODO: figure out why this assert message doesn't match: + * "Expected values to be strictly equal:\n+ actual - expected\n\n+ 'Some custom error message'\n- 'Fake Message'"); + */ + }); + + it("throws when fatal test case has no error", () => { + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ code: "foo", error: { name: "Error" } }] + }); + }, "A fatal test case must throw an exception"); + }); + + it("throws with missing properties or wrong property types", () => { + + // empty `fatal` object + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{}] + }); + }, "Test case property 'error' must be an object"); + + // wrong type `code` + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ code: true }] + }); + }, "Optional test case property 'code' must be a string"); + + // wrong type `name` + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ name: true }] + }); + }, "Optional test case property 'name' must be a string"); + + // extra property in `fatal` object + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ error: { name: "Error" }, code: "'foo'", foo: true }] + }); + }, 'ESLint configuration in rule-tester is invalid:\n\t- Unexpected top-level property "foo".'); + + // extra property in `error` object + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ error: { name: "Error", foo: true }, code: "'foo'" }] + }); + }, "Invalid error property name 'foo'. Expected one of ['message', 'name']."); + + // empty `error` object + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ error: {} }] + }); + }, "Test case property 'error' object must provide a string 'name' property or string/regexp 'message' property"); + + // wrong type `error.name` + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ error: { name: true } }] + }); + }, "Test case property 'error' object must provide a string 'name' property or string/regexp 'message' property"); + + // wrong type `error.message` + assert.throws(() => { + ruleTester.run("foo", rule, { + valid: [{ code: "'baz'" }], + invalid: [{ code: "'bar'", errors: [{ type: "Literal" }] }], + fatal: [{ error: { message: true } }] + }); + }, "Test case property 'error' object must provide a string 'name' property or string/regexp 'message' property"); + }); + }); + describe("deprecations", () => { let processStub; const ruleWithNoSchema = { @@ -2787,6 +3048,15 @@ describe("RuleTester", () => { sinon.assert.calledWith(spyRuleTesterIt, code); }); + it("should label a fatal test case with no `code` correctly", () => { + ruleTester.run("no-var", require("../../fixtures/testers/rule-tester/no-var"), { + valid: [], + invalid: [], + fatal: [{ options: [{ foo: true }] }] + }); + sinon.assert.calledWith(spyRuleTesterIt, "(Test Case #1)"); + }); + }); describe("SourceCode#getComments()", () => {