From d30bf8a3ebcafb1279e1c4361fc615424c2690ed Mon Sep 17 00:00:00 2001 From: Brandon Mills Date: Sun, 27 Dec 2020 22:45:48 -0500 Subject: [PATCH 1/5] New: RuleTester test isolation with only --- designs/2020-rule-tester-only/README.md | 171 ++++++++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 designs/2020-rule-tester-only/README.md diff --git a/designs/2020-rule-tester-only/README.md b/designs/2020-rule-tester-only/README.md new file mode 100644 index 00000000..23f960c6 --- /dev/null +++ b/designs/2020-rule-tester-only/README.md @@ -0,0 +1,171 @@ +- Repo: eslint/eslint +- Start Date: 2020-12-27 +- RFC PR: (leave this empty, to be filled in later) +- Authors: Brandon Mills ([@btmills](https://github.com/btmills)) + +# `RuleTester` test isolation with `only` + +## Summary + + + +`RuleTester` currently lacks a built-in way for developers to isolate individual tests during development. +Temporarily deleting or commenting out the other tests is tedious. +This adds an optional `only` property to run individual tests in isolation. + +## Motivation + + + +When developers are working on a rule, we can run its tests with `npm run test:cli tests/lib/rules/my-rule.js`. +Debugging sometimes requires focusing on a particular test and running it in isolation to avoid unrelated noise from other tests. + +Tools like Mocha already offer `it.only()` for low-friction test isolation. +Developers may already be familiar with that API. +Exposing a similar API via `RuleTester` would improve the ergonomics of debugging tests. +This API is beneficial to any approach from `console.log("HERE");` to interactive debugging with breakpoints. + +## Detailed Design + + + +Add an optional boolean `only` property to `RuleTester`'s `ValidTestCase` and `InvalidTestCase` types. +Add `only` to the other `RuleTester`-only properties in the `RuleTesterParameters` array to exclude it from configuration schema validation. + +To allow using `RuleTester` with a custom test framework other than Mocha, parallel `RuleTester`'s existing `describe` and `it` implementations for `itOnly`: + +1. Declare an `IT_ONLY` `Symbol`, set it as a static property on `RuleTester`, and initialize it to `null`. +1. Add an `itOnly` `set` accessor that sets `RuleTester[IT_ONLY]`. +1. Add an `itOnly` `get` accessor. + 1. If `RuleTester[IT_ONLY]` is set, return it. + 2. If global `it` and `it.only` are functions, return `it.only`. *TODO: Is `this` necessary? Could return a wrapper instead.* + 3. Throw an error: + 1. If either `RuleTester[DESCRIBE]` or `RuleTester[IT]` is customized, recommend setting a custom `RuleTester.itOnly`. + 2. If global `it` is a function, the current test framework does not support `only`. + 3. Otherwise recommend installing a test framework like Mocha so that `only` can be used. + +At the end of `RuleTester`'s `run()` method, for each valid and invalid item, if `only` is `true`, call `RuleTester.itOnly` instead of `RuleTester.it`. + +## Documentation + + + +The [RuleTester section](https://eslint.org/docs/developer-guide/nodejs-api#ruletester) in our Node.js API docs will need to include a description of the `only` property on test case objects. + +The [Unit Tests](https://eslint.org/docs/developer-guide/unit-tests) page in the developer guide will also include a note about this next to the [Running Individual Tests](https://eslint.org/docs/developer-guide/unit-tests#running-individual-tests) section. + +## Drawbacks + + + +This exposes new API surface. +That is mitigated by the implementation still being a wrapper around Mocha's existing API like the rest of `RuleTester`. + +Deleted or commented-out tests are hard to miss in diffs. +An extra `only: true` could more easily sneak through, accidentally disabling all other tests. +We could mitigate this by always exiting with 1 when `only: true` is used even if the test itself passes, though this diverges from Mocha's behavior. +We could instead write a custom internal rule or add a rule to `eslint-plugin-eslint-plugin` to flag `only` in committed code. + +## Backwards Compatibility Analysis + + + +This change is backwards compatible. +`RuleTester` currently fails any tests that include unrecognized properties like `only`, so we have no risk of breaking existing tests. + +If someone has customized `RuleTester` with a custom `it()` method, we cannot assume that `it.only()` has the same semantics as Mocha. +Their existing tests will still work, but if they wish to use this new `only` property, they would need to provide a custom `itOnly()` alongside `it()`. + +## Alternatives + + + +This builds upon Mocha's established `it.only()` API as prior art. + +The status quo has two alternatives: + +1. We can temporarily delete or comment out the other tests. This is tedious. +1. We can filter through noise by scrolling through `console.log()` output or using conditional breakpoints, though the latter is not always possible. + +## Open Questions + + + +1. We could define a static `only()` convenience method on `RuleTester`. +Given, for example, a valid string test `"var foo = 42;"`, it is easier to convert to `RuleTester.only("var foo = 42;")`, which is equivalent to `{ code: "var foo = 42;", only: true }`. +The helper sets `only: true` on a test case argument after converting string cases to objects if necessary. +For invalid cases that are already objects, adding `only: true` is likely easier than using the helper. +1. Should using `only` cause the process to exit `1` even if tests pass as described in "Drawbacks"? +1. In the `RuleTester.itOnly` `get` accessor, if `RuleTester[IT_ONLY]` is not customized and the global `it.only` is not a function, is throwing an error the right choice? +We could instead pass through to `RuleTester.it`, ignoring `only` if it's not supported. +1. Do we need to support `skip`, the inverse of `only`? + +## Help Needed + + + +I can implement this myself. + +## Frequently Asked Questions + + + +## Related Discussions + + + +- This was suggested as an alternative solution to [RFC67](https://github.com/eslint/rfcs/pull/67) and [eslint/eslint#13625](https://github.com/eslint/eslint/issues/13625) because that change may not be possible as currently described. From 3f3f4c7c2a86a538d9fa73fe65676bbd07a6307a Mon Sep 17 00:00:00 2001 From: Brandon Mills Date: Fri, 1 Jan 2021 16:48:49 -0500 Subject: [PATCH 2/5] Use mocha --forbid-only --- designs/2020-rule-tester-only/README.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/designs/2020-rule-tester-only/README.md b/designs/2020-rule-tester-only/README.md index 23f960c6..815ffae3 100644 --- a/designs/2020-rule-tester-only/README.md +++ b/designs/2020-rule-tester-only/README.md @@ -54,6 +54,8 @@ To allow using `RuleTester` with a custom test framework other than Mocha, paral At the end of `RuleTester`'s `run()` method, for each valid and invalid item, if `only` is `true`, call `RuleTester.itOnly` instead of `RuleTester.it`. +Add [`--forbid-only`](https://mochajs.org/#-forbid-only) to the [`mocha` target in `Makefile.js`](https://github.com/eslint/eslint/blob/cc4871369645c3409dc56ded7a555af8a9f63d51/Makefile.js#L548) to prevent accidentally merging tests before removing `only`. + ## Documentation