Prevent accidentally committing focused test selectors #2199
Conversation
sarayourfriend
added
🟨 priority: medium
github-actions
bot
added
🏷 status: label work required
|
Size Change: 0 B Total Size: 839 kB ℹ️ View Unchanged
|
|
@dhruvkb I started rebasing that PR earlier this week and need to get back to it. I basically just need to start back from scratch rather than trying to apply it directly here. I think the changes in here (hoisting the eslint config for tests up) will actually make it easier. Something I ran into and kept trying to work around in that PR was weird dependency installation issues for nested configurations. If we run eslint in a local hook (as discussed in #2197) that would also help as installing a local package also took a good bit of fiddling and in the end didn't really work correctly all the time. Hopefully next week I can get that other eslint PR working fully and we can write some more abstract custom rules 🤞 |
obulat
removed
🏷 status: label work required
|
@sarayourfriend I'm seeing a discrepancy between the PR description and the actual diff. The description mentions adding and configuring the Jest ESLint plugin while the diff only shows the addition of custom rules for disallowing the focused test selectors. I'm happy with either approach, just want to make sure I review the intended one. |
sarayourfriend
force-pushed
the
add/disallow-commit-test-only
branch
from
741ff93
to
a969327
Compare
|
Shoot, I forgot to push 🤦 Changes are up now, apologies for the confusion. |
| { | ||
| selector: | ||
| "ImportDeclaration[source.value='@vue/test-utils']:has(ImportSpecifier[local.name='shallowMount'])", | ||
| message: | ||
| "Do not use @vue/test-utils' `shallowMount`. Use @testing-library/vue's `render` instead.", | ||
| }, |
There was a problem hiding this comment.
This rule is moved to the unit test scope in overrides further down in this file.
| env: { jest: true }, | ||
| files: ["frontend/test/unit/**"], | ||
| plugins: ["jest"], | ||
| extends: ["plugin:jest/recommended"], | ||
| rules: { | ||
| "import/no-named-as-default-member": ["off"], | ||
| "@intlify/vue-i18n/no-raw-text": ["off"], | ||
| "no-restricted-imports": [ | ||
| "error", | ||
| { | ||
| name: "pinia", | ||
| message: | ||
| "Please import pinia test utils from `~~/test/unit/test-utils/pinia`. The test-utils version ensures proper setup of universally necessary Nuxt context mocks.", | ||
| }, | ||
| ], | ||
| "no-restricted-syntax": [ | ||
| "error", | ||
| { | ||
| selector: | ||
| "ImportDeclaration[source.value='@vue/test-utils']:has(ImportSpecifier[local.name='shallowMount'])", | ||
| message: | ||
| "Do not use @vue/test-utils' `shallowMount`. Use `~~/test/unit/test-utils/render` instead which includes helpful context setup or @testing-library/vue's `render` directly.", | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Using a single ESLint configuration with overrides to configure sub-folder behaviour is ultimately easier to understand, I think, than nested configurations. We also had incorrectly configured the nested configuration with an unnecessary extends setting, (which causes a new configuration to be created rather than a merged one), showing that it's harder to maintain. It also makes it easier to reason about which rules apply where and which ones are shared without having to jump between multiple files. We can move the other ESLint configuration files into overrides in separate PRs.
| @@ -26,6 +26,7 @@ const pwCommand = | |||
| process.env.FASTSTART !== "false" ? "start:playwright" : "prod:playwright" | |||
|
|
|||
| const config: PlaywrightTestConfig = { | |||
| forbidOnly: !!process.env.CI, | |||
There was a problem hiding this comment.
This allows it to pass locally but fails CI if .only is present.
There was a problem hiding this comment.
Why would we allow it to pass locally if .only is present but fail on CI? Is it time consuming to have this check run locally? And do you think it makes sense to add a comment as to why this was added here?
| expect(playStub).toHaveBeenCalledTimes(1) | ||
| expect(pauseStub).toHaveBeenCalledTimes(1) | ||
| expect(getByText(/Reproduction not allowed./i)).toBeVisible() |
There was a problem hiding this comment.
None of these expect calls returned promises.
| expect(() => { | ||
| screen.getByText(props.audio.title) | ||
| screen.getByLabelText("Attribution-NonCommercial-Share-Alike") | ||
| screen.getByText("Music") | ||
| }).not.toThrow() |
There was a problem hiding this comment.
This is a workaround related to #2198. I think it's better than setting each of these as assert functions using the comment approach in other modules.
A potentially better alternative is to change them to query* functions and assert .not.toBeNull() on them, but I'll let the discussion I linked play out before we make concerted refactors to these. We may decide on a different approach altogether.
| @@ -15,7 +17,7 @@ import VTabPanel from "~/components/VTabs/VTabPanel.vue" | |||
| * @param {string} text | |||
| * @param {boolean?} visibility | |||
| */ | |||
| const expectVisibitility = (screen, text, visibility = true) => { | |||
There was a problem hiding this comment.
This function name was misspelled.
| }) | ||
| describe("actions", () => { |
There was a problem hiding this comment.
This describe block had the same name as the previous block so I merged them.
| const exepectedStoreValue = | ||
| key === "isMobileUi" ? false : initialState[key] | ||
| expect(uiStore[key]).toEqual(exepectedStoreValue) |
There was a problem hiding this comment.
This code is equivalent to the original but prevents the no-conditional-expect rule from needing to be disabled twice (or at all).
| expect(document.getElementsByTagName("a")).not.toHaveLength(0) | ||
| document.body.innerHTML = getAttribution(mediaItem, i18n, { | ||
| isPlaintext: true, | ||
| }) | ||
| expect(document.getElementsByTagName("a").length === 0) | ||
| expect(document.getElementsByTagName("a")).toHaveLength(0) |
There was a problem hiding this comment.
The original version of these assertions were noops. expect() by itself does nothing. Here's an excellent example of the Jest plugin catching incorrect, false positive (and therefore dangerous because they create false confidence) tests.
| "no-restricted-syntax": [ | ||
| "error", | ||
| { | ||
| selector: | ||
| "ImportDeclaration[source.value='@vue/test-utils']:has(ImportSpecifier[local.name='shallowMount'])", | ||
| message: | ||
| "Do not use @vue/test-utils' `shallowMount`. Use `~~/test/unit/test-utils/render` instead which includes helpful context setup or @testing-library/vue's `render` directly.", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
OOOH this is perfect, I need this in #2196
sarayourfriend
force-pushed
the
add/disallow-commit-test-only
branch
from
a969327
to
af43026
Compare
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
sarayourfriend
force-pushed
the
add/disallow-commit-test-only
branch
from
af43026
to
df95273
Compare
| // This test is broken (for some reason clickOutside does not appear to actually cause a click | ||
| // to happen in this case). | ||
| // https://github.com/WordPress/openverse/issues/2220 | ||
| // eslint-disable-next-line jest/no-disabled-tests | ||
| it.skip("should hide the popover if a click happens outside the popover by default", async () => { |
There was a problem hiding this comment.
| // This test is broken (for some reason clickOutside does not appear to actually cause a click | |
| // to happen in this case). | |
| // https://github.com/WordPress/openverse/issues/2220 | |
| // eslint-disable-next-line jest/no-disabled-tests | |
| it.skip("should hide the popover if a click happens outside the popover by default", async () => { | |
| it("should hide the popover if a click happens outside the popover by default", async () => { |
The fix has been merged.
There was a problem hiding this comment.
Thank you for all your work on improving our confidence in tests and code changes, @sarayourfriend . All the changes look correct
There was a problem hiding this comment.
Tested a number of the new linting rules to make sure they work, and also tried setting forbidOnly to true locally and running the suite to confirm it fails. Works perfectly. Errors look like this for anyone curious:
Error: focused item found in the --forbid-only mode: "all results grid keyboard accessibility test should open audio results as links"
|
@sarayourfriend I'm going to merge this in the interest of getting #2196 undrafted and so I can finish it while it's fresh in my memory. |
| @@ -26,6 +26,7 @@ const pwCommand = | |||
| process.env.FASTSTART !== "false" ? "start:playwright" : "prod:playwright" | |||
|
|
|||
| const config: PlaywrightTestConfig = { | |||
| forbidOnly: !!process.env.CI, | |||
There was a problem hiding this comment.
Why would we allow it to pass locally if .only is present but fail on CI? Is it time consuming to have this check run locally? And do you think it makes sense to add a comment as to why this was added here?
| @@ -10,6 +10,7 @@ addAliases({ | |||
| }) | |||
|
|
|||
| const config: PlaywrightTestConfig = { | |||
| forbidOnly: !!process.env.CI, | |||
There was a problem hiding this comment.
Same note here as above
There was a problem hiding this comment.
.only is useful to have locally if you're debugging a single, specific test, for example, in debug mode, where it runs in a "headed" browser that you can poke around the context with: https://playwright.dev/docs/debug
In that setting, if you haven't selected a single test to run, the runner is going to run everything in the module, requiring you to try check which test is running each time and pass it until you get to the one you want.
That's to say: only is useful locally, but harmful if committed and forbidding it in CI is an easy way to catch when it is committed. Additionally, something that isn't necessarily obvious, is that Playwright and Jest are distinct test runners. Even though they look very similar in API, they have certain differences that will make the eslint-plugin-jest incompatible (or at least flaky) with Playwright. It would be better if our configuration didn't let you commit the test selection, though, and luckily there is a Playwright-specific ESLint plugin with the same no-focused-test rule that the Jest plugin provides. I've opened an issue to add it here: #2312. That would give us a whole host of helpful checks, but also prevent .only from being committed at all in our Playwright tests (rather than just failing in CI).
There was a problem hiding this comment.
Woah, okay that makes sense! Thanks for the helpful context!
Fixes
Follow up to #2194 by @obulat
Description
The related PR removes an accidentally committed
.onlyfocused test selector. We can easily prevent accidental commits like that in the future with the Jest ESLint plugin and with Playwright's ownforbidOnlysetting.This PR adds the Jest plugin and enables Playwright's
forbidOnlysetting in CI.Testing Instructions
Check out the changes made to appease the new ESLint plugin. I think they are good rules. I opened a discussion about whether relying on testing-library's
get*function's error handling behaviour as an implicit assertion is a good idea long-term: #2198For now I've just selectively disabled the
expect-expectcheck or expanded the assert function list for modules that heavily rely on that.The plugin caught a handful of tests that truly were incorrect, either because they had no assertions at all (!!) or because they were using
expectincorrectly (likeexpect(predicate)rather thanexpect(someValue).predicate(), the former does nothing).Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin