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

test(linter): add linter to coverage task #3591

Closed
wants to merge 2 commits into from

Conversation

DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Jun 9, 2024

Makes cargo coverage run the linter. It can be run in isolation with cargo coverage linter.

The linter is run with fixes and all rules enabled (except nursery rules) in an identical fashion to oxlint -W all --fix. The fixed code is then re-parsed and checked for errors. Tests will fail if parsing fails or if semantic analysis produces more errors the second time around than when the original source code is analyzed.

I've already found several problems with fixes, including 2 panics. One of them was fixed in #3590, and the other is fixed in this PR.

@DonIsaac DonIsaac added the A-linter Area - Linter label Jun 9, 2024
Copy link

graphite-app bot commented Jun 9, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

Copy link

codspeed-hq bot commented Jun 9, 2024

CodSpeed Performance Report

Merging #3591 will not alter performance

Comparing don/linter/test/conformance (8cd8450) with main (f0b689d)

Summary

✅ 22 untouched benchmarks

@DonIsaac DonIsaac marked this pull request as ready for review June 9, 2024 03:19
@Boshen
Copy link
Member

Boshen commented Jun 9, 2024

Should we add this to https://github.com/oxc-project/oxlint-ecosystem-ci/actions/workflows/ecosystem-ci.yml instead 🤔 It's quite difficult to interpret the snapshot results.

This also makes the conformance suite even slower to compile :-(

@DonIsaac
Copy link
Contributor Author

DonIsaac commented Jun 9, 2024

It appears to add 50s to the conformance CI job. If we think this is too high I can move it to the ecosystem CI job, though it may not catch some of the weirder fixes (e.g. labels)

@Boshen
Copy link
Member

Boshen commented Jun 9, 2024

It appears to add 50s to the conformance CI job. If we think this is too high I can move it to the ecosystem CI job, though it may not catch some of the weirder fixes (e.g. labels)

Let's try and move this to ecosystem CI with fixes applied to running oxlint, and then check whether the fixed file is good, maybe by running oxlint over it again?

@Boshen Boshen marked this pull request as draft June 12, 2024 08:18
@Boshen
Copy link
Member

Boshen commented Jun 12, 2024

Moving to oxc-project/oxlint-ecosystem-ci#11

@Boshen Boshen closed this Jun 12, 2024
@Boshen Boshen deleted the don/linter/test/conformance branch October 14, 2024 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants