chore(eslint): Add lint rules for disabled or focused tests #8489
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
mydea
reviewed
Jul 11, 2023
Lms24
force-pushed
the
lms/chore-add-test-eslint-rules
branch
from
20002e8
to
7858d95
Compare
AbhiPrasad
approved these changes
Jul 12, 2023
There was a problem hiding this comment.
Limitation: Our Playwright browser integration tests use a custom test function (sentryTest instead of test) which these rules cannot detect. I tried setting the global aliases setting but it didn't work because sentryTest is imported and hence not considered a global. We can revisit adding a custom rule for this later.
We don't have to worry about playwright for test.only because we can turn on
test.skip is still a problem I guess.
Lms24
force-pushed
the
lms/chore-add-test-eslint-rules
branch
from
7858d95
to
dd2dffb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We sometimes forget to remove focused or disabled annotations after debugging tests. This of course isn't ideal and as brought up in #8485 we should add checks against this. Therefore, this PR:
eslint-plugin-jestpackage which contains a bunch of jest-syntax-specific rulesno-focused-testsrule to throw a lint error if a test is focused withit.only,fitor similar functions.no-disabled-testsrule to throw a lint error if a test (suite) from a suite is disabled/skipped withit.skip,xitor similar functions. While we sometimes skip tests on purpose, I'd argue that we generally want to avoid this, as it can also happen accidentally. For the few exceptions of this rule, we can always ignore it.If reviewers want to avoid adding another dependency to the project, we can also extract the rules and vendor them. However, I'd argue that adding
eslint-plugin-jestis probably the simplest way to enable this rules and we might want to enable other rules in the future (e.g.no-conditional-expects).This rule also works with vitest syntax which we use in the SvelteKit package.
Limitation: Our Playwright browser integration tests use a custom test function (
sentryTestinstead oftest) which these rules cannot detect. I tried setting the global aliases setting but it didn't work becausesentryTestis imported and hence not considered a global. We can revisit adding a custom rule for this later.