10000 test(eslint-plugin): install multiple eslint versions to test matrix by esetnik · Pull Request #2464 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]

Skip to content

test(eslint-plugin): install multiple eslint versions to test matrix #2464

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

Conversation

esetnik
Copy link
Contributor
@esetnik esetnik commented Sep 1, 2020

No description provided.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @esetnik!

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. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@esetnik esetnik marked this pull request as ready for review September 1, 2020 17:15
@bradzacher bradzacher added the tests anything to do with testing label Sep 1, 2020
@esetnik
Copy link
Contributor Author
esetnik commented Sep 1, 2020

@bradzacher note that a maintainer will need to update the list of expected tests to include the new matrix

@esetnik esetnik force-pushed the test-eslint-multiple-versions branch from 0c46d01 to dddb051 Compare September 1, 2020 17:55
@esetnik esetnik force-pushed the test-eslint-multiple-versions branch from 870cb82 to 7f54ae3 Compare September 1, 2020 18:10
@esetnik
Copy link
Contributor Author
esetnik commented Sep 1, 2020

@bradzacher I'm not sure why the tests are still failing. I rebased off master which should have fixed the eslint v5/v6 compatibility issues.

@bradzacher
Copy link
Member

Our extension rules build on top of the underlying rule. In some rules we just short-circuit branches for TS usecases, in other cases we do our own reporting for TS usecases.

The latter is why our no-unused-vars extension rule fails on eslint v6.

Our tests include a number of tests to ensure that cases our extensions don't touch remain unaffected. These tests are using message IDs.

So this is failing because the tests are expecting that the rule reports using a message ID.
For older versions of ESLint, the core rule won't report using a message ID (it'll use a raw message string).

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Sep 1, 2020
@esetnik
Copy link
Contributor Author
esetnik commented Sep 2, 2020

@bradzacher do you have a proposal of how to solve this? One idea I had is to implement some detection logic in the rule tester which will detect eslint version under test and change the expectations accordingly. But maybe it's not the best solution. It'd be nice if the tests themselves were stable across supported eslint versions and didn't rely on any specific implementation details of eslint v7.

@bradzacher
Copy link
Member

do you have a proposal of how to solve this?

There's two solutions I can think of.

First would be to use message instead of messageId in the tests.

The eslint tester will resolve the reported messageId with data before it does its checks. This means that message will potentially work across versions.

Note that this will work across versions if and only if the content of the message remains consistent across versions. This is the danger of message, and why messageId is always preferred (because it decouples the test from the message content).


Second would be to have the test be aware of the eslint version and update the assertions appropriately (I.e. Wrap every error object in a helper function to do a transform).

The eslint package exports the current version as a property on the [CLIEngine class] (https://github.com/typescript-eslint/typescript-eslint/blob/v4.0.0/packages/experimental-utils/src/ts-eslint/CLIEngine.ts#L103), so you can detect the version easily enough.

This would probably be my preferred option, as it will ensure the tests remain stable for all future versions.

@bradzacher bradzacher marked this pull request as draft September 15, 2020 03:55
@bradzacher bradzacher added DO NOT MERGE PRs which should not be merged yet and removed awaiting response Issues waiting for a reply from the OP or another party labels Jan 7, 2021
@JoshuaKGoldberg
Copy link
Member

👋 @esetnik, just checking in! Do you still have interest & time to work on this PR?

No worries if not!

@bradzacher
Copy link
Member

Closing for housekeeping purposes.
Anyone is free to raise a fresh PR and continue this work!

8BF1

@bradzacher bradzacher closed this Mar 4, 2022
@bradzacher bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Mar 4, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
DO NOT MERGE PRs which should not be merged yet stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period tests anything to do with testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0