Codecov Report

Attention: Patch coverage is 91.17647% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 95.72%. Comparing base (f77ceb6) to head (f3318fb).

❗ Current head f3318fb differs from pull request most recent head 727839a. Consider uploading reports for the commit 727839a to get more accurate results

@@             Coverage Diff             @@
##             main    #2996       +/-   ##
===========================================
+ Coverage   85.67%   95.72%   +10.04%     
===========================================
  Files          78       79        +1     
  Lines        3275     3301       +26     
  Branches     1150     1140       -10     
===========================================
+ Hits         2806     3160      +354     
+ Misses        469      141      -328     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ljharb I'm probably going to knock off soon - would you mind having a look over both the changes and CI, and leave me some pointers for the coming days?

In addition to just the standard "how do you feel about this code", I'm not sure why some of the builds are failing with "Class constructor RuleTester cannot be invoked without 'new'" as I'm 99% sure my JS syntax is correct so I'd say the compiler is doing it wrong but that seems unlikely...? That could be related to why I found I couldn't use export function even though export default was being used elsewhere...

Also I've not yet looked into the utils package (?) - it looks like there's at least one function call there that will require a new argument.

But hey we've got about half some of the jobs passing :D

I've just gotten back from travel, so I'll try to take a look at it in the coming days.

One likely obstacle is that all the tests assume eslintrc, but eslint 9 requires an env var to support it, otherwise it defaults to flat config. The initial support needs to test eslintrc, and it would be fine to add flat config tests in a follow-on if needed.

It's likely that the way eslint < 9's RuleTester works is subtly different than in eslint 9, so we may need an abstraction to handle that.

Yup part of this includes switching to a custom rule tester that converts the tests from eslintrc to flat config if they're running on v9, which is what we've been using in eslint-plugin-jest without issue.

That's great, but testing eslintrc on eslint 9 is also very important.

It'd also be great if there was a commit or two I could land separately, that worked on eslint < 9, so that only the 9-specific parts remained in this PR after that was landed and rebased :-)

If you're meaning the context helper stuff, yeah I'm happy to pull them out I just figured you'd have asked them to be tested against ESLint v9 to prove they actually worked :-)

You figured right :-) but i can just cherry-pick the commits out of this PR once things are working.

Right well they're already good for cherry-picking - you want to pick from e31fe5c...543e378 (sans f0853cb once #2997 is landed), and away you go.

I think you can have the eslintrc-on-v9 support by just running the whole test suite twice with the env enabled and an extra test in the custom rule tester - I'll push that up shortly

@ljharb RuleTester does not support eslintrc in v9 - the env variable is only used by the CLI

well that sucks, we’ll need to file an eslint issue about that gap.

I'll leave that to you as I suspect you'll have more pull :-)

Filed eslint/eslint#18292.

Can you help me understand what withoutAutofixOutput is for

That was a helper which already existed in one of the tests, so I decided to pull it out as a utility for the others for consistency and then it can be cherry picked ahead of v9.

As for why it's required, v9 rule tester considers it an error for the output to match the code - using this utility means (once cherry picked) in this PR we can make one change to the utility (to make output null) rather than have to change the tests for every single rule.

Also while I don't mind input, just confirming I'm still chipping away at the general list of failures - I plan to come back later to address the finer details like adding comments :)

If someone has the time to look at the failures especially during my night, that's be appreciated - it looks like there's some around regression tests invoking eslint programmatically that I've not been able to figure out how to change while ensuring they're still testing what they're meant to be testing. Even if someone can figure out the general solution, Im happy to handle applying it to all the tests of that type

Totally, while it's still a draft I'm indeed just chipping away at the review :-) your effort is appreciated!