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

Consider splitting out a generalized testing plugin #1289

Closed
JoshuaKGoldberg opened this issue Nov 18, 2022 · 8 comments
Closed

Consider splitting out a generalized testing plugin #1289

JoshuaKGoldberg opened this issue Nov 18, 2022 · 8 comments

Comments

@JoshuaKGoldberg
Copy link
Contributor

Hi! I'm a big fan of this plugin and have been using it for a while. But I recently switched some projects to Vitest and not all the rules in this plugin apply quite the same in Vitest (e.g. no-jasmine-globals).

eslint-plugin-vitest exists, with some duplication of rule logic. It's much newer, though, and has fewer rules implemented & a much smaller commmunity.

What do you think about us extracting out the rules from these two plugins into a shared plugin, like eslint-plugin-tests, that's meant to work for the common style of testing libraries in JS/TS today? I'm a maintainer on typescript-eslint and am confident I can help or lead this if you want. 😄

cc @veritem from eslint-plugin-vitest 👋

@mrazauskas
Copy link
Contributor

Sounds interesting. How do you imagine the usage? Should user add both eslint-plugin-tests and eslint-plugin-jest to ESLint config?

I am asking, because this could be useful for one of mine work-in-progress project too. It has Jest style API and could reuse many rules. (Only API is similar, but that is not one more Jest like testing framework.)

@JoshuaKGoldberg
Copy link
Contributor Author

Good question. In my mind, this is exclusively & only for the concerns that are common across all tests (e.g. not having conditionals, not having duplicate test names, etc.). Anything specific to tests (e.g. Jasmine globals) would be relegated to a framework-specific library like eslint-plugin-jest.

In that world, one option for how users would use the plugins would be to include both:

{
  extends: ["plugin:tests/recommended", "plugin:jest/recommended"],
  plugins: ["tests", "jest"],
}

However, thinking about user concerns, that's a little annoying. Plus I imagine test-specific libraries might want to configure the core eslint-plugin-tests rules with their own specific concerns & rules. So maybe eslint-plugin-jest & co would want people to only go through it:

{
  extends: ["plugin:jest/recommended"],
  plugins: ["jest"],
}

@mrazauskas
Copy link
Contributor

Can it be a library which is exporting a set of common rules, which could be imported and used by each framework specific plugin? This way users will not notice any difference.

Only question: does ESLint plugin architecture allow this? I don't have much experience in the field. You are the expert here (;

@JoshuaKGoldberg
Copy link
Contributor Author

It should, yes - https://github.com/Codecademy/client-modules/tree/main/packages/eslint-config is what we used in Codecademy to pull in rules from other configs. Additionally, a lot of users end up writing their own shareable configs. https://sourcegraph.com/search?q=context:global+repo:.*eslint.*config.*+file:README.md&patternType=standard&sm=1 shows results for a few, such as https://github.com/RebeccaStevens/eslint-config-rebeccastevens.

I suppose one more user concern is that it could be confusing which rule to configure when a plugin includes two of them:

{
  "rules": {
    "jest/no-jasmine-globals": "error",
    "tests/no-focused-tests": "error",
  }
}

This new project could lean even harder into being a building block layer and, instead of acting as an ESLint config/plugin, act as a "builder" to generate rules. Vaguely:

import { builders } from "eslint-test-plugins";

export const noFocusedTestsRule = builders.noFocusedTests(/* ... */);

Then each config would have its own version of the rules, and would need to document them separately:

{
  "rules": {
    "jest/no-jasmine-globals": "error",
    "jest/no-focused-tests": "error",
  }
}

cc @bmish - I don't know how well that would play with the future of eslint-doc-generator.

I'm personally leaning mildly against this because it's more complicated, a relatively new thing, and creates multiple versions of many similar rules, but figured it's an option to bring up. 😄

@bmish
Copy link
Contributor

bmish commented Nov 18, 2022

I have definitely seen overlap in lint rules for different test frameworks. Many best practices are the same regardless of the test framework. So I know ESLint plugins for different test frameworks already take inspiration from each other's rules on occasion or otherwise end up with some similar rules (in particular, I work on eslint-plugin-qunit and this has been the case).

However, I do worry that trying to share rules between testing frameworks would increase complexity too much. There are obviously syntax/naming/behavior differences between testing frameworks that each lint rule would have to handle. It can already be challenging just trying to ensure a lint rule handles different major versions of a single testing framework (especially with occasional breaking changes), and so trying to handle multiple testing frameworks in the same rule would add yet another dimension to the matrix of scenarios to handle. And just imagine trying to test and document each rule in such a way that it covers every possible testing framework...that's a lot of maintenance overhead and complexity vs. just having a single, light-weight, focused rule per test framework (the no-focused-tests rule is pretty simple as-is). Rules that try to do too much can become unmanageable in my experience.

@mrazauskas
Copy link
Contributor

Programmatic API would be harder to implement, but it would allow more flexibility. For example, "no-deprecated-functions" rule could be build on top of shared logic using framework specific list of deprecated APIs; or having custom rule name would be useful for "jest/no-restricted-jest-methods" and "vitest/no-restricted-vitest-methods" (can be this one is not yet implemented).

@G-Rath
Copy link
Collaborator

G-Rath commented Nov 18, 2022

So I've thought about this in the past (especially when I started being involved in maintaining eslint-plugin-jest-dom, among other things), and overall came to the conclusion that while it seems like a really good and useful idea on the face, in practice there's not a whole lot of gain vs the huge potential complexities because of how different core features can be handled in testing frameworks, and that rules typically stabilize very quickly so I wouldn't expect you would need to be sinking a lot of effort into keeping a copied rule up to date.

This has also been the reason why I've not pulled the utilities out into their own package*, because while they look like they'd be useful across a bunch of plugins, everyone has a slightly different implementations which would mean having to get buy-in from the plugin maintainers to potentially refactor their rules heavily, when their existing implementations work fine and generally have a low footprint so it doesn't feel like there's a strong enough gain to be worth the effort it'll require...

So yeah, for me whenever I've thought about this I've ended up at "this feels like a lot of work for something that doesn't really fix anything that's broken"

*: this will probably happen at some point for parseJestFn and friends because eslint-plugin-jest, eslint-plugin-jest-dom, and eslint-plugin-jest-extended should all be resolving jest functions with that, but I've got to review eslint-plugin-jest-dom first as that currently uses selectors very heavily.

@JoshuaKGoldberg
Copy link
Contributor Author

Closing out my old issues for repos I no longer have context on. If anybody has a need for what this issue was asking about, I'd encourage them/you to file a new issue. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants