Skip to content

feat(rule-tester): add reimplementation of RuleTester #9331

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

Closed
wants to merge 9 commits into from

Conversation

luca-script
Copy link

PR Checklist

Overview

Adds a new FlatRuleTester implementation, which supports flat config and has been completely (from scratch) rewritten in full TypeScript with greater performance and following more modern coding standards (Namely defining your rules as code as opposed to passing a giant JSON-esque array)

Porting should be easy via the FlatRuleConfig.fromObject method (Which takes a mostly RuleTester.run-compatible object)
And should be more performant (Although I have not checked yet)

BREAKING CHANGES COMPARED TO RuleTester:

  • Uses ESLint's flat config system
  • Supports per-extension options
  • Requires type-checking
  • Does not support modifying parserOptions on a per-case basis (For performance reasons)

TODO

  • Add unit tests
  • Maybe make the API callback-based, i.e.
new FlatRuleTester({...}).rule("my-rule", rule, (def) => {
  def.configuration([], (conf) => {
    conf.valid(`console.log('Example');`);
  }));
});
  • Make it check TypeScript for errors and report on missing type definitions
  • Support auto importing types (as if /// <reference types="...">)
  • Add website documentation and migration guide
  • Implement missing options from RuleTester
  • By default, we should use @typescript-eslint/parser as the parser
  • maybe: Add support for a config file in the project root that defines where the fixtureRoot is (Maybe a subclass or configuration option that automatically does that?)
  • maybe: Fix internal ESLint Linter types
  • maybe: More Jest integrations?
  • maybe: Support caching results (Very unlikely to be implemented by me)
  • maybe: Support string imports for parsers
  • maybe: File-based testing? (Make files in a directory, point the RuleTester at it, and it'll test every single file, would also use comments to describe where errors should occur)
  • maybe: Support modifying parserOptions on a per-configuration basis (Would affect performance noticably)
  • maybe: Hook filesystem APIs to emulate a project (Should improve compatibility with ESLint plugins which require configuration files or file context)

luca-script and others added 7 commits June 7, 2024 10:00
Introduces a `FlatRuleTester` class which overhauls the API
Why?
Because we shouldn't be writing rules as JSON inside our (Java/Type)script
And the new API prevents writing our tests as a giant function call which we pass a giant object

We'll need to port all the rules over to the new tester eventually
@typescript-eslint
Copy link
Contributor

Thanks for the PR, @luca-script!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Jun 11, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 3a7ae98
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/66681ad56cac5b0008a640e9
😎 Deploy Preview https://deploy-preview-9331--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 95 (🔴 down 4 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 90 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@luca-script luca-script changed the title feat(rule-tester): Add reimplementation of RuleTester feat(rule-tester): add reimplementation of RuleTester Jun 11, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luca-script sorry, I'm very confused here. As I understood it, the backing ESLint PR eslint/eslint#17922 was mostly just a rename. From its OP:

What changes did you make? (Give an overview)

  1. Removed RuleTester
  2. Renamed FlatRuleTester to RuleTester
  3. Updated api.js to export the new RuleTester
  4. Updated unsupported-api.js to stop exporting FlatRuleTester
  5. Updated the API docs
  6. Updated the Migrate to v9 guide
  7. Updated all rule tests

...and our issue #9017 is just for us to do a similar rename.

Why the reimplementation of FlatRuleTester?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure. We don't want a complete reimplementation for scratch because we are looking for feature parity with upstream.

The existing implementation is intended to be as close to the original code as possible (warts and all) to make it easy for us to merge upstream changes back in to our fork.

Not that we need parity because it's intended to be a drop-in replacement - an augmentation of the core rule tester.

I don't much care for their code style either - and I would love to rewrite it from scratch - but that will make maintenance harder in the long run.

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Jun 27, 2024
@JoshuaKGoldberg
Copy link
Member

👋 @luca-script is this still something you have time + energy for?

@JoshuaKGoldberg JoshuaKGoldberg added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jul 17, 2024
@azat-io
Copy link
Contributor

azat-io commented Jul 17, 2024

@JoshuaKGoldberg Is this feature only planned for the next major release?

@JoshuaKGoldberg
Copy link
Member

@JoshuaKGoldberg Is this feature only planned for the next major release?

Yes: it's a breaking change so we can't get it into v7.

@JoshuaKGoldberg
Copy link
Member

Closing this PR out as I'm tackling the backing issue with #9603.

@luca-script you did a lot of great work in this PR and I'm sad to see it closed. Same as #9346. Next time, I'd suggest reading the issue carefully & checking with us before you put a lot of effort into a big PR. My advice in general is to ease into a new repository with smaller bugfixes & not-exceptionally-complex feature additions before tackling big issues like these ones. ❤️

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement(rule-tester): FlatRuleTester -> RuleTester
4 participants