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

feat(lint): rule noMisplacedAssertion #1935

Merged
merged 6 commits into from
Mar 27, 2024
Merged

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Feb 28, 2024

Summary

This PR closes #1848 (last rule)

It implements the rule noMisplacedAssertion, which checks if expect or assert are called inside the it function.

This rule has options: users can define the name of the assertion functions and the specifiers from which these functions are imported.

Test Plan

Added test cases

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Parser Area: parser A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Feb 28, 2024
Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 7caeea7
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/660421de5ed1b20008043dcf
😎 Deploy Preview https://deploy-preview-1935--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 98 (🔴 down 1 from production)
Accessibility: 97 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 593 593 0
Failed 69 69 0
Panics 0 0 0
Coverage 89.58% 89.58% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13439 13439 0
Failed 4205 4205 0
Panics 2 2 0
Coverage 76.16% 76.16% 0.00%

Copy link

codspeed-hq bot commented Feb 28, 2024

CodSpeed Performance Report

Merging #1935 will not alter performance

Comparing feat/no-misplaced-assertion (7caeea7) with main (ad0a0b5)

Summary

✅ 93 untouched benchmarks

@github-actions github-actions bot added A-Diagnostic Area: diagnostocis and removed A-Project Area: project A-Diagnostic Area: diagnostocis labels Mar 13, 2024
@github-actions github-actions bot added the A-Project Area: project label Mar 26, 2024
@ematipico ematipico marked this pull request as ready for review March 26, 2024 09:55
@ematipico ematipico requested review from a team March 26, 2024 09:55
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

I like this rule, but I think it's a bit too opinionated towards the describe/it pattern. At the least I would let it support the test function (and ideally also Deno.test) by default as well, like these:

And then we can probably add the bun:test specifier too :)

@ematipico
Copy link
Member Author

ematipico commented Mar 27, 2024

I refactored the rule. I removed the options - for now. Let's see what the users say and devise a plan when the time comes.

Now the built-in assertions are wider, for example bun, vitest and Deno.test are taken in consideration.

@ematipico ematipico requested a review from arendjr March 27, 2024 11:31
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Very nice!

@ematipico ematipico merged commit 699ed38 into main Mar 27, 2024
21 checks passed
@ematipico ematipico deleted the feat/no-misplaced-assertion branch March 27, 2024 15:03
@ipanasenko
Copy link
Contributor

What about waitFor from testing-library? Their official docs recommend to use expect inside waitFor to wait for something: https://testing-library.com/docs/dom-testing-library/api-async#waitfor

@ipanasenko
Copy link
Contributor

And maybe allow to use expect() inside other functions that start with expect? E.g. a common const expectSomeStuffToMatch = (stuff) => expect(stuff).toMatch()

@ematipico
Copy link
Member Author

Hey @ipanasenko, thank you for your suggestions. I advise you to create a discussion, a thread on discord or a new issue. Commenting on closed PRs doesn't help the visibility of your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Parser Area: parser A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

☂️ Lint rules for testing frameworks
3 participants