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

[New] support eslint v9 #3759

Merged
merged 7 commits into from
Jul 15, 2024
Merged

Conversation

mdjermanovic
Copy link
Contributor

@mdjermanovic mdjermanovic commented May 19, 2024

Refs #3699

Adds utility to automatically transform tests to ESLint v9 format when ESLint v9 is used.

I have all tests still passing with ESLint v8 locally (we'll see what happens with the older versions in CI).

Around ~500 tests are failing with ESLint v9 due to new RuleTester checks. Some because of problems in tests (e.g., duplicate tests or missing some now-mandatory test case properties), some because of problems in rules (e.g., no-op options schema or unsubstituted placeholders in messages). I'll try to fix that in separate commits in this PR.

Also fixed bugs that ESLint v9 RuleTester caught in rules jsx-closing-bracket-location and no-invalid-html-attribute.

@mdjermanovic
Copy link
Contributor Author

CI with ESLint v9 fails here:

******> npm ls >/dev/null
npm error code ELSPROBLEMS
npm error invalid: eslint@9.3.0 /home/runner/work/eslint-plugin-react/eslint-plugin-react/node_modules/eslint
eslint-plugin-react@7.34.1 /home/runner/work/eslint-plugin-react/eslint-plugin-react
├── @babel/core@7.24.5
├── @babel/eslint-parser@7.24.5
├── @babel/plugin-syntax-decorators@7.24.1
├── @babel/plugin-syntax-do-expressions@7.24.1
├── @babel/plugin-syntax-function-bind@7.24.1
├── @babel/preset-react@7.24.1
├── @types/eslint@7.2.10
├── @types/estree@0.0.52
├── @types/node@4.9.5
├── @typescript-eslint/parser@5.62.0
├── array-includes@3.1.8
├── array.prototype.findlast@1.2.5
├── array.prototype.flatmap@1.3.2
├── array.prototype.toreversed@1.1.2
├── array.prototype.tosorted@1.1.3
├── aud@2.0.4
├── babel-eslint@10.1.0
├── doctrine@2.1.0
├── es-iterator-helpers@1.0.19
├── eslint-config-airbnb-base@15.0.0
├── eslint-doc-generator@1.7.1
├── eslint-plugin-eslint-plugin@5.5.1
├── eslint-plugin-import@2.29.1
├── eslint-remote-tester-repositories@1.0.1
├── eslint-remote-tester@3.0.1
├── eslint-scope@3.7.3
├── eslint@9.3.0 invalid: "^3 || ^4 || ^5 || ^6 || ^7 || ^8" from the root project
├── espree@3.5.4
├── estraverse@5.3.0
├── glob@10.3.7
├── istanbul@0.4.5
├── jackspeak@2.1.1
├── jsx-ast-utils@3.3.5
├── ls-engines@0.8.1
├── markdownlint-cli@0.32.2
├── minimatch@3.1.2
├── mocha@5.2.0
├── npmignore@0.3.1
├── object.entries@1.1.8
├── object.fromentries@2.0.8
├── object.hasown@1.1.4
├── object.values@1.2.0
├── prop-types@15.8.1
├── resolve@2.0.0-next.5
├── semver@6.3.1
├── sinon@7.5.0
├── string.prototype.matchall@4.0.11
├── typescript-eslint-parser@20.1.1
└── typescript@3.9.10

npm error A complete log of this run can be found in: /home/runner/.npm/_logs/2024-05-19T19_26_11_701Z-debug-0.log
got status code 1
1

Seems I'll have to update peerDependencies in root package.json in this PR to make it reach the test step?

Copy link

codecov bot commented May 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.62%. Comparing base (417e1ca) to head (ef6f9c5).
Report is 14 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3759      +/-   ##
==========================================
- Coverage   97.79%   97.62%   -0.17%     
==========================================
  Files         134      134              
  Lines        9613     9615       +2     
  Branches     3486     3486              
==========================================
- Hits         9401     9387      -14     
- Misses        212      228      +16     

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

@ljharb
Copy link
Member

ljharb commented May 20, 2024

yes, you'll need to update the peerDeps and devDeps ranges to include eslint 9.

tests/lib/rules/jsx-uses-react.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-uses-vars.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Contributor Author

In d8f3186, 9a86bc8 & 85eed32, I updated several tests where output was the same as code to use output: null to assert that there's no autofix expected. ESLint v9 RuleTester reports output === code as a possible error (by writing the full output code, it looks like the author is expecting an autofix). If you want, you can doublecheck whether in those test cases it is really expected that there is no autofix.

Copy link

socket-security bot commented May 20, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/eslint@9.7.0 environment Transitive: eval, filesystem, shell, unsafe +80 9.56 MB eslintbot

View full report↗︎

@mdjermanovic mdjermanovic changed the title [Tests] test rules with ESLint v9 [New] support eslint v9 May 20, 2024
@mdjermanovic
Copy link
Contributor Author

yes, you'll need to update the peerDeps and devDeps ranges to include eslint 9.

Done in a1402a2 & cda54a3. I expected the dev deps update to be problematic for linting since this project uses eslintrc config, but npm still installs ESLint v8 (I guess because of the plugins' peer deps) so it's okay. I can now see in CI the same tests that are failing for me locally with ESLint v9. Working on the fixes.

@mdjermanovic
Copy link
Contributor Author

mdjermanovic commented May 20, 2024

Actually, it isn't okay everywhere in CI. pretest fails with:

Oops! Something went wrong! :(

ESLint: 9.3.0

ESLint couldn't find an eslint.config.(js|mjs|cjs) file.

https://github.com/jsx-eslint/eslint-plugin-react/actions/runs/9157980135/job/25175410426?pr=3759#logs

code: '<TestComponent only={this.handleChange} />',
code: '<TestComponent2 only={this.handleChange} />',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looked like an intent to test both message, and messageId + data with two tests in a row, so I just modified one of those two a bit.

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

Successfully merging this pull request may close these issues.

4 participants