From a3f7b958d8c8dfefbe37df9253cbc4ececdebd53 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Thu, 27 Oct 2022 17:26:40 +1030 Subject: [PATCH 1/4] feat(utils): add `RuleTester` API for top-level dependency constraints --- .../eslint-utils/rule-tester/RuleTester.ts | 83 ++++++++++++++++--- packages/utils/src/ts-eslint/RuleTester.ts | 9 +- .../rule-tester/RuleTester.test.ts | 68 ++++++++++++++- 3 files changed, 142 insertions(+), 18 deletions(-) diff --git a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts index e81d23d0206..b925021572c 100644 --- a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts +++ b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts @@ -1,9 +1,13 @@ import type * as TSESLintParserType from '@typescript-eslint/parser'; +import assert from 'assert'; import { version as eslintVersion } from 'eslint/package.json'; import * as path from 'path'; import * as semver from 'semver'; -import * as TSESLint from '../../ts-eslint'; +import type { ParserOptions } from '../../ts-eslint/ParserOptions'; +import type { RuleModule } from '../../ts-eslint/Rule'; +import type { RuleTesterTestFrameworkFunction } from '../../ts-eslint/RuleTester'; +import * as BaseRuleTester from '../../ts-eslint/RuleTester'; import { deepMerge } from '../deepMerge'; import type { DependencyConstraint } from './dependencyConstraints'; import { satisfiesAllDependencyConstraints } from './dependencyConstraints'; @@ -11,18 +15,28 @@ import { satisfiesAllDependencyConstraints } from './dependencyConstraints'; const TS_ESLINT_PARSER = '@typescript-eslint/parser'; const ERROR_MESSAGE = `Do not set the parser at the test level unless you want to use a parser other than ${TS_ESLINT_PARSER}`; -type RuleTesterConfig = Omit & { +type RuleTesterConfig = Omit & { parser: typeof TS_ESLINT_PARSER; + /** + * Constraints that must pass in the current environment for any tests to run + */ + dependencyConstraints?: DependencyConstraint; }; interface InvalidTestCase< TMessageIds extends string, TOptions extends Readonly, -> extends TSESLint.InvalidTestCase { +> extends BaseRuleTester.InvalidTestCase { + /** + * Constraints that must pass in the current environment for the test to run + */ dependencyConstraints?: DependencyConstraint; } interface ValidTestCase> - extends TSESLint.ValidTestCase { + extends BaseRuleTester.ValidTestCase { + /** + * Constraints that must pass in the current environment for the test to run + */ dependencyConstraints?: DependencyConstraint; } interface RunTests< @@ -36,24 +50,29 @@ interface RunTests< type AfterAll = (fn: () => void) => void; -class RuleTester extends TSESLint.RuleTester { +class RuleTester extends BaseRuleTester.RuleTester { readonly #baseOptions: RuleTesterConfig; - static #afterAll: AfterAll; + static #afterAll: AfterAll | undefined; /** * If you supply a value to this property, the rule tester will call this instead of using the version defined on * the global namespace. */ static get afterAll(): AfterAll { return ( - this.#afterAll || + this.#afterAll ?? (typeof afterAll === 'function' ? afterAll : (): void => {}) ); } - static set afterAll(value) { + static set afterAll(value: AfterAll | undefined) { this.#afterAll = value; } + private get staticThis(): typeof RuleTester { + // the cast here is due to https://github.com/microsoft/TypeScript/issues/3841 + return this.constructor as typeof RuleTester; + } + constructor(baseOptions: RuleTesterConfig) { super({ ...baseOptions, @@ -73,8 +92,7 @@ class RuleTester extends TSESLint.RuleTester { // make sure that the parser doesn't hold onto file handles between tests // on linux (i.e. our CI env), there can be very a limited number of watch handles available - // the cast here is due to https://github.com/microsoft/TypeScript/issues/3841 - (this.constructor as typeof RuleTester).afterAll(() => { + this.staticThis.afterAll(() => { try { // instead of creating a hard dependency, just use a soft require // a bit weird, but if they're using this tooling, it'll be installed @@ -85,11 +103,11 @@ class RuleTester extends TSESLint.RuleTester { } }); } - private getFilename(testOptions?: TSESLint.ParserOptions): string { + private getFilename(testOptions?: ParserOptions): string { const resolvedOptions = deepMerge( this.#baseOptions.parserOptions, testOptions, - ) as TSESLint.ParserOptions; + ) as ParserOptions; const filename = `file.ts${resolvedOptions.ecmaFeatures?.jsx ? 'x' : ''}`; if (resolvedOptions.project) { return path.join( @@ -107,9 +125,48 @@ class RuleTester extends TSESLint.RuleTester { // This is a lot more explicit run>( name: string, - rule: TSESLint.RuleModule, + rule: RuleModule, testsReadonly: RunTests, ): void { + if ( + this.#baseOptions.dependencyConstraints && + !satisfiesAllDependencyConstraints( + this.#baseOptions.dependencyConstraints, + ) + ) { + type DescribeWithMaybeSkip = RuleTesterTestFrameworkFunction & { + skip?: RuleTesterTestFrameworkFunction; + }; + const describe: DescribeWithMaybeSkip = this.staticThis.describe; + if ('skip' in describe && typeof describe.skip === 'function') { + // for frameworks like mocha or jest that have a "skip" version of their function + // we can provide a nice skipped test! + type DescribeWithSkip = RuleTesterTestFrameworkFunction & { + skip: RuleTesterTestFrameworkFunction; + }; + (this.staticThis.describe as DescribeWithSkip).skip(name, () => { + this.staticThis.it( + 'All test skipped due to unsatisfied constructor dependency constraints', + () => {}, + ); + }); + } else { + // otherwise just declare an empty test + this.staticThis.describe(name, () => { + this.staticThis.it( + 'All test skipped due to unsatisfied constructor dependency constraints', + () => { + // some frameworks error if there are no assertions + assert.equal(true, true); + }, + ); + }); + } + + // don't run any tests because we don't match the base constraint + return; + } + const tests = { // standardize the valid tests as objects valid: testsReadonly.valid.map(test => { diff --git a/packages/utils/src/ts-eslint/RuleTester.ts b/packages/utils/src/ts-eslint/RuleTester.ts index 7002fc538cd..61855f4bbfe 100644 --- a/packages/utils/src/ts-eslint/RuleTester.ts +++ b/packages/utils/src/ts-eslint/RuleTester.ts @@ -169,7 +169,8 @@ declare class RuleTesterBase { * @param text a string describing the rule * @param callback the test callback */ - static describe?: RuleTesterTestFrameworkFunction; + static get describe(): RuleTesterTestFrameworkFunction; + static set describe(value: RuleTesterTestFrameworkFunction | undefined); /** * If you supply a value to this property, the rule tester will call this instead of using the version defined on @@ -177,7 +178,8 @@ declare class RuleTesterBase { * @param text a string describing the test case * @param callback the test callback */ - static it?: RuleTesterTestFrameworkFunction; + static get it(): RuleTesterTestFrameworkFunction; + static set it(value: RuleTesterTestFrameworkFunction | undefined); /** * If you supply a value to this property, the rule tester will call this instead of using the version defined on @@ -185,7 +187,8 @@ declare class RuleTesterBase { * @param text a string describing the test case * @param callback the test callback */ - static itOnly?: RuleTesterTestFrameworkFunction; + static get itOnly(): RuleTesterTestFrameworkFunction; + static set itOnly(value: RuleTesterTestFrameworkFunction | undefined); /** * Define a rule for one particular run of tests. diff --git a/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts b/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts index cab7666196a..ca8ee21e1fd 100644 --- a/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts +++ b/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts @@ -73,8 +73,8 @@ RuleTester.itOnly = jest.fn(); /* eslint-enable jest/prefer-spy-on */ const mockedAfterAll = jest.mocked(RuleTester.afterAll); -const _mockedDescribe = jest.mocked(RuleTester.describe); -const _mockedIt = jest.mocked(RuleTester.it); +const mockedDescribe = jest.mocked(RuleTester.describe); +const mockedIt = jest.mocked(RuleTester.it); const _mockedItOnly = jest.mocked(RuleTester.itOnly); const runSpy = jest.spyOn(BaseRuleTester.prototype, 'run'); const mockedParserClearCaches = jest.mocked(parser.clearCaches); @@ -715,5 +715,69 @@ describe('RuleTester', () => { } `); }); + + describe('constructor constraints', () => { + it('skips all tests if a constructor constraint is not satisifed', () => { + const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + dependencyConstraints: { + 'totally-real-dependency': '999', + }, + }); + + ruleTester.run('my-rule', NOOP_RULE, { + invalid: [ + { + code: 'failing - major', + errors: [], + }, + ], + valid: [ + { + code: 'passing - major', + }, + ], + }); + + expect(mockedDescribe.mock.lastCall).toMatchInlineSnapshot(` + [ + "my-rule", + [Function], + ] + `); + + // trigger the describe block + mockedDescribe.mock.lastCall?.[1](); + expect(mockedIt.mock.lastCall).toMatchInlineSnapshot(` + [ + "All test skipped due to unsatisfied constructor dependency constraints", + [Function], + ] + `); + }); + + it('does not skip all tests if a constructor constraint is satisifed', () => { + const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + dependencyConstraints: { + 'totally-real-dependency': '10', + }, + }); + + ruleTester.run('my-rule', NOOP_RULE, { + invalid: [ + { + code: 'valid', + errors: [], + }, + ], + valid: [ + { + code: 'valid', + }, + ], + }); + }); + }); }); }); From e220691b7f001b7f1af96a19d1c67f67b2aba7b2 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Thu, 27 Oct 2022 16:27:06 -0700 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Josh Goldberg --- packages/utils/src/eslint-utils/rule-tester/RuleTester.ts | 4 ++-- .../utils/tests/eslint-utils/rule-tester/RuleTester.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts index b925021572c..90c2f194386 100644 --- a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts +++ b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts @@ -146,7 +146,7 @@ class RuleTester extends BaseRuleTester.RuleTester { }; (this.staticThis.describe as DescribeWithSkip).skip(name, () => { this.staticThis.it( - 'All test skipped due to unsatisfied constructor dependency constraints', + 'All tests skipped due to unsatisfied constructor dependency constraints', () => {}, ); }); @@ -154,7 +154,7 @@ class RuleTester extends BaseRuleTester.RuleTester { // otherwise just declare an empty test this.staticThis.describe(name, () => { this.staticThis.it( - 'All test skipped due to unsatisfied constructor dependency constraints', + 'All tests skipped due to unsatisfied constructor dependency constraints', () => { // some frameworks error if there are no assertions assert.equal(true, true); diff --git a/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts b/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts index ca8ee21e1fd..a378e7e6106 100644 --- a/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts +++ b/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts @@ -750,7 +750,7 @@ describe('RuleTester', () => { mockedDescribe.mock.lastCall?.[1](); expect(mockedIt.mock.lastCall).toMatchInlineSnapshot(` [ - "All test skipped due to unsatisfied constructor dependency constraints", + "All tests skipped due to unsatisfied constructor dependency constraints", [Function], ] `); From 3e851846469544648a1cae95fcd4776fb70dc81a Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Mon, 31 Oct 2022 21:32:57 +1030 Subject: [PATCH 3/4] address comments --- .../eslint-utils/rule-tester/RuleTester.ts | 19 +++++----- packages/utils/src/ts-eslint/RuleTester.ts | 12 +++---- .../rule-tester/RuleTester.test.ts | 35 +++++++++++++++---- 3 files changed, 43 insertions(+), 23 deletions(-) diff --git a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts index 90c2f194386..9925ee2fd9f 100644 --- a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts +++ b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts @@ -50,6 +50,14 @@ interface RunTests< type AfterAll = (fn: () => void) => void; +function isDescribeWithSkip( + value: unknown, +): value is RuleTesterTestFrameworkFunction & { + skip: RuleTesterTestFrameworkFunction; +} { + return 'skip' in describe && typeof describe.skip === 'function'; +} + class RuleTester extends BaseRuleTester.RuleTester { readonly #baseOptions: RuleTesterConfig; @@ -134,17 +142,10 @@ class RuleTester extends BaseRuleTester.RuleTester { this.#baseOptions.dependencyConstraints, ) ) { - type DescribeWithMaybeSkip = RuleTesterTestFrameworkFunction & { - skip?: RuleTesterTestFrameworkFunction; - }; - const describe: DescribeWithMaybeSkip = this.staticThis.describe; - if ('skip' in describe && typeof describe.skip === 'function') { + if (isDescribeWithSkip(this.staticThis.describe)) { // for frameworks like mocha or jest that have a "skip" version of their function // we can provide a nice skipped test! - type DescribeWithSkip = RuleTesterTestFrameworkFunction & { - skip: RuleTesterTestFrameworkFunction; - }; - (this.staticThis.describe as DescribeWithSkip).skip(name, () => { + this.staticThis.describe.skip(name, () => { this.staticThis.it( 'All tests skipped due to unsatisfied constructor dependency constraints', () => {}, diff --git a/packages/utils/src/ts-eslint/RuleTester.ts b/packages/utils/src/ts-eslint/RuleTester.ts index 61855f4bbfe..6c0b98b795f 100644 --- a/packages/utils/src/ts-eslint/RuleTester.ts +++ b/packages/utils/src/ts-eslint/RuleTester.ts @@ -125,6 +125,10 @@ interface TestCaseError { // readonly message?: string | RegExp; } +/** + * @param text a string describing the rule + * @param callback the test callback + */ type RuleTesterTestFrameworkFunction = ( text: string, callback: () => void, @@ -166,8 +170,6 @@ declare class RuleTesterBase { /** * If you supply a value to this property, the rule tester will call this instead of using the version defined on * the global namespace. - * @param text a string describing the rule - * @param callback the test callback */ static get describe(): RuleTesterTestFrameworkFunction; static set describe(value: RuleTesterTestFrameworkFunction | undefined); @@ -175,8 +177,6 @@ declare class RuleTesterBase { /** * If you supply a value to this property, the rule tester will call this instead of using the version defined on * the global namespace. - * @param text a string describing the test case - * @param callback the test callback */ static get it(): RuleTesterTestFrameworkFunction; static set it(value: RuleTesterTestFrameworkFunction | undefined); @@ -184,16 +184,12 @@ declare class RuleTesterBase { /** * If you supply a value to this property, the rule tester will call this instead of using the version defined on * the global namespace. - * @param text a string describing the test case - * @param callback the test callback */ static get itOnly(): RuleTesterTestFrameworkFunction; static set itOnly(value: RuleTesterTestFrameworkFunction | undefined); /** * Define a rule for one particular run of tests. - * @param name The name of the rule to define. - * @param rule The rule definition. */ defineRule>( name: string, diff --git a/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts b/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts index a378e7e6106..2e620332942 100644 --- a/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts +++ b/packages/utils/tests/eslint-utils/rule-tester/RuleTester.test.ts @@ -739,15 +739,17 @@ describe('RuleTester', () => { ], }); - expect(mockedDescribe.mock.lastCall).toMatchInlineSnapshot(` + // trigger the describe block + expect(mockedDescribe.mock.calls.length).toBeGreaterThanOrEqual(1); + mockedDescribe.mock.lastCall?.[1](); + expect(mockedDescribe.mock.calls).toMatchInlineSnapshot(` [ - "my-rule", - [Function], + [ + "my-rule", + [Function], + ], ] `); - - // trigger the describe block - mockedDescribe.mock.lastCall?.[1](); expect(mockedIt.mock.lastCall).toMatchInlineSnapshot(` [ "All tests skipped due to unsatisfied constructor dependency constraints", @@ -777,6 +779,27 @@ describe('RuleTester', () => { }, ], }); + + // trigger the describe block + expect(mockedDescribe.mock.calls.length).toBeGreaterThanOrEqual(1); + mockedDescribe.mock.lastCall?.[1](); + expect(mockedDescribe.mock.calls).toMatchInlineSnapshot(` + [ + [ + "my-rule", + [Function], + ], + [ + "valid", + [Function], + ], + [ + "invalid", + [Function], + ], + ] + `); + // expect(mockedIt.mock.lastCall).toMatchInlineSnapshot(`undefined`); }); }); }); From 48c11966533a1569815e88faf36603d51df3c023 Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Mon, 31 Oct 2022 21:40:59 +1030 Subject: [PATCH 4/4] oops --- packages/utils/src/eslint-utils/rule-tester/RuleTester.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts index 9925ee2fd9f..54a645ccf25 100644 --- a/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts +++ b/packages/utils/src/eslint-utils/rule-tester/RuleTester.ts @@ -55,7 +55,12 @@ function isDescribeWithSkip( ): value is RuleTesterTestFrameworkFunction & { skip: RuleTesterTestFrameworkFunction; } { - return 'skip' in describe && typeof describe.skip === 'function'; + return ( + typeof value === 'object' && + value != null && + 'skip' in value && + typeof (value as Record).skip === 'function' + ); } class RuleTester extends BaseRuleTester.RuleTester {