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

tests: save legacy tests #205

Merged
merged 2 commits into from May 7, 2024

Conversation

MikeMcC399
Copy link
Collaborator

@MikeMcC399 MikeMcC399 commented May 7, 2024

Situation

Tests in the tests/lib/rules which run internally under ESLint 7.x and 8.x are not compatible with ESLint 9.x.

The guide Migrate to 9.0 > FlatRuleTester is now RuleTester explains:

As announced in our blog post, the temporary FlatRuleTester class has been renamed to RuleTester, while the RuleTester class from v8.x has been removed. Additionally, the FlatRuleTester export from eslint/use-at-your-own-risk has been removed.

This is a breaking change for the tests.

Goal

The enhancement goal is to be able to test the repo rules using each of the supported versions of ESLint: 7.x, 8.x and 9.x.

Migration steps

Achieving the goal requires separating the tests into a legacy copy, for use with the RuleTester class of ESLint 7.x and 8.x, and another copy which will be migrated to use the RuleTester of ESLint 9.x.

The migration is done in two steps:

  1. Create the legacy copy of tests, a script to use this copy and a modification to the CircleCI workflow.
  2. Modify the current copy of tests for compatibility with the new RuleTester class in ESLint 9.x.

Changes

This PR deals with creating the legacy copy.

A follow-on PR will take care of the second step of migrating tests to run under to ESLint 9.x.

  1. Copy tests to tests-legacy
  2. Create a jest.config-legacy.js
  3. Create a test:legacy script
  4. Modify circle.yml to test ESLint 7 and 8 with test-legacy

Note that the original tests/lib/rules directory is temporarily unused in this PR.

References

Verification

Ubuntu 22.04.4 LTS, Node.js v20.12.2 LTS and on
Windows 11, Node.js v20.12.2

npm ci
npm install eslint@7 --force
npm run test:legacy
npm install eslint@8
npm run test:legacy

At this point, ESLint 9 cannot be tested and would result in multiple errors. The resolution is left for a follow-on PR.

@cypress-app-bot
Copy link

@MikeMcC399 MikeMcC399 marked this pull request as ready for review May 7, 2024 07:16
const errors = [{ messageId: 'unexpected' }]
const parserOptions = { ecmaVersion: 6 }

ruleTester.run('assertion-before-screenshot', rule, {
Copy link
Member

Choose a reason for hiding this comment

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

@MikeMcC399 I'm not sure how the new tester works, but if possible, it seems like there'd be utility to breaking out these rules so that they can be in a shared file used for both legacy and recent tests. Just thinking how manual it will be to update both sets of tests whenever an update it needed. Not really sure it's worth it though for just 2 files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jennifer-shehane

This is similar to the Cypress migration from 9.x to 10.x where it was necessary in Cypress GitHub Actions to have two sets of tests in parallel for a limited time, one set for Cypress 9.x that was only updated as absolutely necessary and a new set of tests for Cypress 10.x which were maintained and updated. At some stage, a new major version was declared and support for Cypress 9.x was dropped.

It will be difficult to continue to support ESLint 7, 8 and 9 all in one version of this plugin and at some stage support for earlier versions will need to be dropped. I will put this is a separate issue for discussion.

Thanks for your comments!

@jennifer-shehane jennifer-shehane merged commit a980b1f into cypress-io:master May 7, 2024
8 checks passed
@MikeMcC399 MikeMcC399 deleted the save-legacy-tests branch May 7, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants