From a588ba861bf199b45784128f4ebb6fd76c2ef1b8 Mon Sep 17 00:00:00 2001 From: Anna Bocharova Date: Mon, 12 Aug 2024 22:40:24 +0200 Subject: [PATCH 01/17] RFC draft. --- designs/2024-hooks-for-test-cases/README.md | 107 ++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 designs/2024-hooks-for-test-cases/README.md diff --git a/designs/2024-hooks-for-test-cases/README.md b/designs/2024-hooks-for-test-cases/README.md new file mode 100644 index 00000000..27e3cf0b --- /dev/null +++ b/designs/2024-hooks-for-test-cases/README.md @@ -0,0 +1,107 @@ +- Repo: eslint/eslint +- Start Date: 2024-08-12 +- RFC PR: +- Authors: [Anna Bocharova](https://github.com/RobinTail) + +# Hooks for test cases + +## Summary + + + +## Motivation + + + +## Detailed Design + + + +## Documentation + + + +## Drawbacks + + + +## Backwards Compatibility Analysis + + + +## Alternatives + + + +## Open Questions + + + +## Help Needed + + + +## Frequently Asked Questions + + + +## Related Discussions + + + +- Prior issue: https://github.com/eslint/eslint/issues/18770 +- Draft implementation PR: https://github.com/eslint/eslint/pull/18771 From 13067a575006f08c78883bfae6d8879e7a97aee3 Mon Sep 17 00:00:00 2001 From: Anna Bocharova Date: Mon, 12 Aug 2024 23:28:09 +0200 Subject: [PATCH 02/17] Summary and motivation draft. --- designs/2024-hooks-for-test-cases/README.md | 26 +++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/designs/2024-hooks-for-test-cases/README.md b/designs/2024-hooks-for-test-cases/README.md index 27e3cf0b..af6b3be9 100644 --- a/designs/2024-hooks-for-test-cases/README.md +++ b/designs/2024-hooks-for-test-cases/README.md @@ -9,10 +9,32 @@ +I propose adding an optional `setup` property to the test cases that the ESLint `RuleTester` runs. This feature +will allow developers to prepare specific environments needed for certain rules before running each test case. + ## Motivation - + + +Some rules require more awareness of the user environment, such as dependencies listed in the `package.json` file. +That information is not provided to the rule by `RuleContext`, therefore the rule has to read it from disk. + +Generally, testing a rule using `RuleTester` implies running the linter against the variety of `code` samples and +rule `options` both `valid` and `invalid` in order to cover all possible scenarios of its operation. However, testing +a rule having behavior depending on user's `package.json` becomes more challenging, since the user environment becomes +another variable that has to be different for every test case. + +The currently well-known approach is fixtures: having actual files on disk, but those files are separated from the +test cases, they have to be maintained (they can be renamed, deleted or changed regardless the cases). +The proposed `setup` hooks provides the place to keep the environment preparation, such as mocking the returns of the +file system methods, right within the test cases along with other case variables: `code` and `options`. + +The expected outcome is a more streamlined and maintainable testing process: +- Enhanced Developer Experience: developers can quickly understand and modify tests; +- Improved Readability: Keeping the setup logic within the test case makes it easier to read and debug; +- Reduced Maintenance: Eliminating the need for numerous fixture files reduces the overhead of maintenance. + +Ultimately, this feature will support more efficient and effective testing of rules that depend on user environment. ## Detailed Design From cd412cb5cd52aef8b86749359047e920170c16f0 Mon Sep 17 00:00:00 2001 From: Anna Bocharova Date: Tue, 13 Aug 2024 00:00:56 +0200 Subject: [PATCH 03/17] Detailed design draft. --- designs/2024-hooks-for-test-cases/README.md | 66 +++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/designs/2024-hooks-for-test-cases/README.md b/designs/2024-hooks-for-test-cases/README.md index af6b3be9..f348e780 100644 --- a/designs/2024-hooks-for-test-cases/README.md +++ b/designs/2024-hooks-for-test-cases/README.md @@ -19,11 +19,15 @@ will allow developers to prepare specific environments needed for certain rules Some rules require more awareness of the user environment, such as dependencies listed in the `package.json` file. That information is not provided to the rule by `RuleContext`, therefore the rule has to read it from disk. +For example: [`eslint-plugin-import/no-extraneous-dependencies`](https://github.com/import-js/eslint-plugin-import/blob/09476d7dac1ab36668283f9626f85e2223652b37/src/rules/no-extraneous-dependencies.js#L23) + Generally, testing a rule using `RuleTester` implies running the linter against the variety of `code` samples and rule `options` both `valid` and `invalid` in order to cover all possible scenarios of its operation. However, testing a rule having behavior depending on user's `package.json` becomes more challenging, since the user environment becomes another variable that has to be different for every test case. +For example: [see the tests of the rule mentioned above](https://github.com/import-js/eslint-plugin-import/blob/09476d7dac1ab36668283f9626f85e2223652b37/tests/src/rules/no-extraneous-dependencies.js#L21-L29) + The currently well-known approach is fixtures: having actual files on disk, but those files are separated from the test cases, they have to be maintained (they can be renamed, deleted or changed regardless the cases). The proposed `setup` hooks provides the place to keep the environment preparation, such as mocking the returns of the @@ -47,6 +51,68 @@ Ultimately, this feature will support more efficient and effective testing of ru used. Be sure to define any new terms in this section. --> +The proposed feature introduces the `setup` property to each test case in the ESLint rule tester. +This property is an optional function that runs before the linting process for that specific test case. + +### Implementation + +- Change the test case schema in the ESLint rule tester to include the new property. + - The property is a function that takes no arguments and returns `void`. +- Modify the `RuleTester`: + - It should check for the presence of the setup function within each test case. + - If the `setup` function exists, it should be executed before running the test. + +### Example + +Here is an example of how test case definitions would look with the new `setup` property: + +```javascript +new RuleTester().run("my-custom-rule", myCustomRule, { + valid: [ + { + code: '/* valid code example */', + options: [/* plugin options */], + setup: () => { + // Mock `package.json` or other necessary setup + jest.mock('fs', () => ({ + readFileSync: jest.fn().mockReturnValue(JSON.stringify({ + dependencies: { "some-package": "^1.0.0" } + })), + })); + } + } + ], + invalid: [ + { + code: '/* invalid code example */', + errors: [{ messageId: "someErrorId" }], + setup: () => { + // Mock different `package.json` setup + jest.mock('fs', () => ({ + readFileSync: jest.fn().mockReturnValue(JSON.stringify({ + devDependencies: { "another-package": "^2.0.0" } + })), + })); + } + } + ] +}); +``` + +### Corner Cases + +- `setup` throws `Error`: then the test case should fail, and the error should be reported; +- Developer must ensure that the `setup` function does not inadvertently affect global state in a way that impacts + other test cases. +- Teardown Considerations: while a `teardown` function is not necessary for the current proposal, it may be worth + considering for a symmetry and completeness in more complex scenarios: + +```javascript +const teardown = () => { + jest.resetAllMocks(); // Explicitly clean up any mocks or state changes +} +``` + ## Documentation +The new `setup` property should be formally documented in the following ways: + +- ESLint Rule Tester Documentation: Update the official ESLint `RuleTester` documentation to include detailed + information about the new property. This documentation should explain: + - The purpose and use cases of the setup property; + - How to implement the setup function in test cases; + - Example demonstrating the usage of the `setup` property for both `valid` and `invalid` test cases; +- Changelog: Include an entry in the ESLint changelog outlining the new property with a brief description; + +### Announcement + +To ensure that the ESLint community is aware of the new feature and understands its motivation and usage, a formal +announcement should be made on the ESLint blog. This announcement can include: + +- Introduction to the Feature: Explain what the setup property is and provide context as to why it was introduced; +- Motivation: Describe the motivation for introducing the setup property, including the challenges with the current + approach and the anticipated benefits of the new feature; +- Examples and Use Cases: Provide concrete examples and use cases to show how the setup property can be used to + improve the testing process for ESLint plugins; +- Link to Documentation: Include links to the updated documentation having more detailed information. + ## Drawbacks +Possible concerns: + +- Potential for Misuse: + - Despite being an optional feature, there remains a risk that some users might misuse the setup property + by inadvertently modifying global state, leading to flaky tests. +- Performance: + - There is a risk that developers might overload the setup property, leading to performance decrease. +- Redundancy: confusion with `beforeEach`: + - It is important to emphasize the key difference between these entities in the documentation: + - `beforeEach` is for **consistent** actions applicable to **all** tests; + - `setup` is for **specific** actions applicable to **particular** test. +- Limited Use Case: + - The necessity for the `setup` might be limited to specific rules and plugins. The broader ESLint user + community may not frequently encounter these challenges, and it may seem unjustified to them. + ## Backwards Compatibility Analysis +Adding new optional property to the test case is backwards compatible. +When the property is not set, `RuleTester` acts the same for existing users — no disruption expected. + ## Alternatives +My current workaround is assigning a custom `it` function to the `RuleTester` that looks up the for a mock by test name: + +```javascript +const mockedEnvironments = { + "test case one": { dependencies: { "some-package": "^1.0.0" } }, + "test case two": { devDependencies: { "another-package": "^2.0.0" } } +} + +RuleTester.it = (name, ...rest) => { + jest.mock('fs', () => ({ + readFileSync: jest.fn().mockReturnValue( + JSON.stringify( mockedEnvironments[name] ) + ), + })); + it(name, ...rest); +}; + +new RuleTester().run("my-custom-rule", myCustomRule, { + valid: [ + { + name: "test case one", + code: '/* valid code example */', + options: [/* some options */] + } + ], + invalid: [ + { + code: '/* invalid code example */', + errors: [{ messageId: "someErrorId" }], + options: [/* some options */] + } + ] +}); +``` + +However, I'm not satisfied with this approach due to a number of disadvantages: + +- The `name` property is required to be set on each test case so that `it` could distinguish between them; +- Also, that `name` must be unique for proper lookup; +- And most importantly: mocked environment is detached from the test cases definitions: + - They are having only that weak reference via the `name`; + - It's harder to read, to maintain and to make some adjustments; + - The mocked environment is, in essence, one of the arguments to the rule being tested, + therefore it does make sense for them to be near `options` and `code`. + ## Open Questions +- Should there also be the `teardown` property? + ## Help Needed +The implementation of `setup` is already ready in the mentioned pull request, however, I may need recommendations +regarding changes to the documentation. + ## Frequently Asked Questions +- **Why not use `beforeEach` to achieve the same functionality?** + - While `beforeEach` is effective for common setups applied across all tests in a suite, it falls short when + different tests require unique setups. Using `beforeEach` for such cases would necessitate complex conditional + logic to differentiate setups, leading to harder-to-maintain and less readable code. The `setup` property, + on the other hand, allows for context-specific configurations directly within each test case, making the + tests easier to read and maintain. +- **How would the new `setup` property affect existing test cases?** + - The `setup` property is an opt-in feature. Existing test cases will remain unaffected as they do not utilize + this new property. Only test cases that specifically define the `setup` function will be impacted. + Since `RuleTester` invalidates the unknown props in test cases, no existing users should be affected. + This ensures backward compatibility and does not force any changes on users who prefer existing testing methodology. +- **Will the `setup` property introduce performance overhead?** + - While the execution of individual `setup` functions could introduce some overhead, it is comparable to the + overhead introduced by `beforeEach` hook. Users are encouraged to keep their setup functions lightweight and + efficient. The benefit of having isolated and context-specific setups typically outweighs the minor performance + costs, especially in complex testing scenarios. +- **What if users require a `teardown` function?** + - I can add it to my implementation. +- **What are the specific use cases where `setup` is more beneficial than existing solutions?** + - The setup property is particularly beneficial in scenarios where: + - Different test cases require different environmental configurations; + - Tests need to be highly readable and maintainable, with a preparation logic kept closely to each test case; + - Users need to avoid the complexity of conditional logic in shared hooks like `beforeEach`. + ## Related Discussions -My current workaround is assigning a custom `it` function to the `RuleTester` that looks up the for a mock by test name: +My current workaround is assigning a custom `it` function to the `RuleTester` that looks up for a mock by test name: ```javascript const readerMock = jest.fn(); From f7ac4af1fda8067accb43759b9675ffd1839b9a3 Mon Sep 17 00:00:00 2001 From: Anna Bocharova Date: Thu, 29 Aug 2024 12:16:26 +0000 Subject: [PATCH 16/17] Renaming setup to before, adding after as well. --- designs/2024-hooks-for-test-cases/README.md | 95 +++++++++++---------- 1 file changed, 48 insertions(+), 47 deletions(-) diff --git a/designs/2024-hooks-for-test-cases/README.md b/designs/2024-hooks-for-test-cases/README.md index cf64082d..53c03e34 100644 --- a/designs/2024-hooks-for-test-cases/README.md +++ b/designs/2024-hooks-for-test-cases/README.md @@ -9,8 +9,8 @@ -I propose adding an optional `setup` property to the test cases that the ESLint `RuleTester` runs. This feature -will allow developers to prepare specific environments needed for certain rules before running each test case. +I propose adding an optional `before` (and `after`) properties to the test cases that the ESLint `RuleTester` runs. This +feature will allow developers to prepare specific environments needed for certain rules before running each test case. ## Motivation @@ -30,7 +30,7 @@ For example: [see the tests of the rule mentioned above](https://github.com/impo The currently well-known approach is fixtures: having actual files on disk, but those files are separated from the test cases, they have to be maintained (they can be renamed, deleted or changed regardless the cases). -The proposed `setup` hook provides the place to keep the environment preparation, such as mocking the returns of the +The proposed `before` hook provides the place to keep the environment preparation, such as mocking the returns of the file system methods, right within the test cases along with other case variables: `code` and `options`. The expected outcome is a more streamlined and maintainable testing process: @@ -51,20 +51,22 @@ Ultimately, this feature will support more efficient and effective testing of ru used. Be sure to define any new terms in this section. --> -The proposed feature introduces the `setup` property to each test case in the ESLint rule tester. -This property is an optional function that runs before the linting process for that specific test case. +The proposed feature introduces the `before` and `after` properties to each test case in the ESLint rule tester. +These properties are an optional functions that runs before and after the linting process for that specific test +case accordingly. ### Implementation -- Change the test case schema in the ESLint rule tester to include the new property. - - The property is a function that takes no arguments and returns `void`. +- Change the test case schema in the ESLint rule tester to include the new properties. + - The properties are functions that take no arguments and return `void`. - Modify the `RuleTester`: - - It should check for the presence of the setup function within each test case. - - If the `setup` function exists, it should be executed before running the test. + - It should check for the presence of the `before` function within each test case; + - If the `before` function exists, it should be executed before running the test; + - When the `after` function exists, it should be executed after running the test regardless of its result. ### Example -Here is an example of how test case definitions would look with the new `setup` property: +Here is an example of how test case definitions would look with the new properties: ```javascript const readerMock = jest.fn(); @@ -77,11 +79,14 @@ new RuleTester().run("my-custom-rule", myCustomRule, { { code: '/* valid code example */', options: [/* plugin options */], - setup: () => { + before: () => { // Mock `package.json` or other necessary setup readerMock.mockReturnValueOnce(JSON.stringify({ dependencies: { "some-package": "^1.0.0" } })); + }, + after: () => { + readerMock.mockReset(); } } ], @@ -89,11 +94,14 @@ new RuleTester().run("my-custom-rule", myCustomRule, { { code: '/* invalid code example */', errors: [{ messageId: "someErrorId" }], - setup: () => { + before: () => { // Mock different `package.json` setup readerMock.mockReturnValueOnce(JSON.stringify({ devDependencies: { "another-package": "^2.0.0" } })); + }, + after: () => { + readerMock.mockReset(); } } ] @@ -102,17 +110,9 @@ new RuleTester().run("my-custom-rule", myCustomRule, { ### Corner Cases -- `setup` throws `Error`: then the test case should fail, and the error should be reported; -- Developer must ensure that the `setup` function does not inadvertently affect global state in a way that impacts +- `before` throws `Error`: then the test case should fail, and the error should be reported; +- Developer must ensure that the `before` function does not inadvertently affect global state in a way that impacts other test cases. -- Teardown Considerations: while a `teardown` function is not necessary for the current proposal, it may be worth - considering for a symmetry and completeness in more complex scenarios: - -```javascript -const teardown = () => { - jest.resetAllMocks(); // Explicitly clean up any mocks or state changes -} -``` ## Documentation @@ -121,23 +121,23 @@ const teardown = () => { on the ESLint blog to explain the motivation? --> -The new `setup` property should be formally documented in the following ways: +The new `before` property should be formally documented in the following ways: - ESLint Rule Tester Documentation: Update the official ESLint `RuleTester` documentation to include detailed information about the new property. This documentation should explain: - - The purpose and use cases of the setup property; - - How to implement the setup function in test cases; - - Example demonstrating the usage of the `setup` property for both `valid` and `invalid` test cases; + - The purpose and use cases of the `before` property; + - How to implement the `before` function in test cases; + - Example demonstrating the usage of the `before` property for both `valid` and `invalid` test cases; ### Announcement To ensure that the ESLint community is aware of the new feature and understands its motivation and usage, a formal announcement should be made on the ESLint blog. This announcement can include: -- Introduction to the Feature: Explain what the setup property is and provide context as to why it was introduced; -- Motivation: Describe the motivation for introducing the setup property, including the challenges with the current +- Introduction to the Feature: Explain what the `before` property is and provide context as to why it was introduced; +- Motivation: Describe the motivation for introducing the `before` property, including the challenges with the current approach and the anticipated benefits of the new feature; -- Examples and Use Cases: Provide concrete examples and use cases to show how the setup property can be used to +- Examples and Use Cases: Provide concrete examples and use cases to show how the `before` property can be used to improve the testing process for ESLint plugins; - Link to Documentation: Include links to the updated documentation having more detailed information. @@ -156,17 +156,19 @@ announcement should be made on the ESLint blog. This announcement can include: Possible concerns: +- Checking duplicates among the test cases will not work when `before` or `after` property is present: + - This drawback comes from the nature of the serialization approach used for comparing test cases; - Potential for Misuse: - - Despite being an optional feature, there remains a risk that some users might misuse the setup property + - Despite being an optional feature, there remains a risk that some users might misuse the `before` property by inadvertently modifying global state, leading to flaky tests. - Performance: - - There is a risk that developers might overload the setup property, leading to performance decrease. + - There is a risk that developers might overload the `before` property, leading to performance decrease. - Redundancy: confusion with `beforeEach`: - It is important to emphasize the key difference between these entities in the documentation: - `beforeEach` is for **consistent** actions applicable to **all** tests; - - `setup` is for **specific** actions applicable to **particular** test. + - `before` is for **specific** actions applicable to **particular** test. - Limited Use Case: - - The necessity for the `setup` might be limited to specific rules and plugins. The broader ESLint user + - The necessity for the `before` might be limited to specific rules and plugins. The broader ESLint user community may not frequently encounter these challenges, and it may seem unjustified to them. ## Backwards Compatibility Analysis @@ -250,7 +252,7 @@ However, I'm not satisfied with this approach due to a number of disadvantages: you can remove this section. --> -- Should there also be the `teardown` property? +None ## Help Needed @@ -261,8 +263,7 @@ However, I'm not satisfied with this approach due to a number of disadvantages: of help would you need from the team? --> -The implementation of `setup` is already ready in the mentioned pull request, however, I may need recommendations -regarding changes to the documentation. +I may require recommendations on placing the `after` handling and advices on changes to the documentation. ## Frequently Asked Questions @@ -277,23 +278,23 @@ regarding changes to the documentation. - **Why not use `beforeEach` to achieve the same functionality?** - While `beforeEach` is effective for common setups applied across all tests in a suite, it falls short when different tests require unique setups. Using `beforeEach` for such cases would necessitate complex conditional - logic to differentiate setups, leading to harder-to-maintain and less readable code. The `setup` property, + logic to differentiate setups, leading to harder-to-maintain and less readable code. The `before` property, on the other hand, allows for context-specific configurations directly within each test case, making the tests easier to read and maintain. -- **How would the new `setup` property affect existing test cases?** - - The `setup` property is an opt-in feature. Existing test cases will remain unaffected as they do not utilize - this new property. Only test cases that specifically define the `setup` function will be impacted. +- **How would the new `before` property affect existing test cases?** + - The `before` property is an opt-in feature. Existing test cases will remain unaffected as they do not utilize + this new property. Only test cases that specifically define the `before` function will be impacted. Since `RuleTester` invalidates the unknown props in test cases, no existing users should be affected. This ensures backward compatibility and does not force any changes on users who prefer existing testing methodology. -- **Will the `setup` property introduce performance overhead?** - - While the execution of individual `setup` functions could introduce some overhead, it is comparable to the - overhead introduced by `beforeEach` hook. Users are encouraged to keep their setup functions lightweight and +- **Will the `before` property introduce performance overhead?** + - While the execution of individual `before` functions could introduce some overhead, it is comparable to the + overhead introduced by `beforeEach` hook. Users are encouraged to keep their `before` functions lightweight and efficient. The benefit of having isolated and context-specific setups typically outweighs the minor performance costs, especially in complex testing scenarios. -- **What if users require a `teardown` function?** - - I can add it to my implementation. -- **What are the specific use cases where `setup` is more beneficial than existing solutions?** - - The setup property is particularly beneficial in scenarios where: +- **What if users require a teardown function?** + - They could use the corresponding `after` property for that purpose. +- **What are the specific use cases where `before` is more beneficial than existing solutions?** + - The `before` property is particularly beneficial in scenarios where: - Different test cases require different environmental configurations; - Tests need to be highly readable and maintainable, with a preparation logic kept closely to each test case; - Users need to avoid the complexity of conditional logic in shared hooks like `beforeEach`. From 339fa205c98b18c689c668b30c71c50a3fabcf1a Mon Sep 17 00:00:00 2001 From: Anna Bocharova Date: Wed, 11 Sep 2024 20:58:07 +0200 Subject: [PATCH 17/17] Addressing corner cases topic --- designs/2024-hooks-for-test-cases/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/designs/2024-hooks-for-test-cases/README.md b/designs/2024-hooks-for-test-cases/README.md index 53c03e34..b8c16e9a 100644 --- a/designs/2024-hooks-for-test-cases/README.md +++ b/designs/2024-hooks-for-test-cases/README.md @@ -110,7 +110,8 @@ new RuleTester().run("my-custom-rule", myCustomRule, { ### Corner Cases -- `before` throws `Error`: then the test case should fail, and the error should be reported; +- `before` or `after` throws `Error`: then the test case should fail, and the error should be reported; +- `after` should be executed even when `before` throws in order to minimize potential impact on other test cases; - Developer must ensure that the `before` function does not inadvertently affect global state in a way that impacts other test cases.