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

Upgrade eslint-plugin-playwright to get new checks; fix small checkbox bug #4088

Merged
merged 6 commits into from
May 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/src/components/VCheckbox/VCheckbox.vue
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
<VIcon
v-else
v-show="localCheckedState"
class="absolute inset-0 transform text-white"
class="pointer-events-none absolute inset-0 transform text-white"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move this to VIcon and set this as a default there because most of the times, we don't need pointer events on an icon.

name="check"
:size="5"
/>
Expand Down
38 changes: 23 additions & 15 deletions frontend/src/components/VCheckbox/meta/VCheckbox.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,31 @@ const checkboxArgTypes = {
disabled: { control: { type: "boolean" } },
}

const defaultArgs = {
id: "default",
name: "Code is Poetry",
value: "codeIsPoetry",
checked: false,
isSwitch: false,
}

const Template = (args) => ({
template: `<VCheckbox v-bind="args" v-on="args" class="mb-4">Code is Poetry</VCheckbox>`,
template: `<VCheckbox v-bind="args" v-on="args" class="mb-4">{{ args.name }}</VCheckbox>`,
components: { VCheckbox },
setup() {
return { args }
},
})

const LicenseCheckboxTemplate = (args) => ({
template: `<fieldset>
<legend>License</legend>
<VCheckbox v-bind="args" v-on="args" class="mb-4">
<VLicense license="by-nc" class="me-4"/>
</VCheckbox>
</fieldset>`,
template: `
<fieldset>
<legend>License</legend>
<VCheckbox v-bind="args" v-on="args" class="mb-4">
<VLicense license="by-nc" class="me-4"/>
</VCheckbox>
</fieldset>
`,
components: { VCheckbox, VLicense },
setup() {
return { args }
Expand All @@ -37,7 +47,10 @@ export default {
components: VCheckbox,
decorators: [WithScreenshotArea],

args: defaultArgs,

argTypes: {
...checkboxArgTypes,
change: {
action: "change",
},
Expand All @@ -48,11 +61,7 @@ export const Default = {
render: Template.bind({}),
name: "Default",

args: {
id: "default",
name: "storybook",
value: "codeIsPoetry",
},
args: defaultArgs,

argTypes: checkboxArgTypes,
}
Expand All @@ -62,9 +71,7 @@ export const Switch = {
name: "Switch",

args: {
id: "default",
name: "storybook",
value: "codeIsPoetry",
...defaultArgs,
isSwitch: true,
},

Expand All @@ -76,6 +83,7 @@ export const LicenseCheckbox = {
name: "License Checkbox",

args: {
...defaultArgs,
id: "cc-by",
name: "license",
value: "cc-by",
Expand Down
2 changes: 2 additions & 0 deletions frontend/test/playwright/e2e/filters.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,13 +216,15 @@ breakpoints.describeMobileAndDesktop(({ breakpoint }) => {
const filterButtonText = await page
.locator('[aria-controls="filters"] span:visible')
.textContent()
// eslint-disable-next-line playwright/no-conditional-expect
expect(filterButtonText).toContain("Filters")
} else {
const filtersAriaLabel =
// eslint-disable-next-line playwright/no-conditional-in-test
(await page
.locator('[aria-controls="content-settings-modal"]')
.getAttribute("aria-label")) ?? ""
// eslint-disable-next-line playwright/no-conditional-expect
expect(filtersAriaLabel.trim()).toEqual("Menu. 1 filter applied")
}

Expand Down
2 changes: 2 additions & 0 deletions frontend/test/playwright/e2e/preferences.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,10 @@ test.describe("switchable features", () => {

// eslint-disable-next-line playwright/no-conditional-in-test
if (checked) {
// eslint-disable-next-line playwright/no-conditional-expect
await expect(featureFlag).not.toBeChecked()
} else {
// eslint-disable-next-line playwright/no-conditional-expect
await expect(featureFlag).toBeChecked()
}
})
Expand Down
2 changes: 1 addition & 1 deletion frontend/test/playwright/e2e/search-types.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ async function checkPageMeta(page: Page, searchType: SearchTypeConfig) {
await expect(page).toHaveURL(expectedURL)
}

/* eslint playwright/expect-expect: ["warn", { "additionalAssertFunctionNames": ["checkSearchResult"] }] */
/* eslint playwright/expect-expect: ["warn", { "assertFunctionNames": ["checkSearchResult"] }] */
async function checkSearchResult(page: Page, searchType: SearchTypeConfig) {
await checkSearchMetadata(page, searchType)
await checkLoadMore(page, searchType)
Expand Down
56 changes: 56 additions & 0 deletions frontend/test/storybook/functional/v-checkbox.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { expect, test } from "@playwright/test"

import { makeGotoWithArgs } from "~~/test/storybook/utils/args"

test.describe.configure({ mode: "parallel" })
;["default", "switch"].forEach((type) => {
const goToStory = makeGotoWithArgs(`components-vcheckbox--${type}`)
const goToAndWait = async (
...[page, ...args]: Parameters<typeof goToStory>
) => {
await goToStory(page, ...args)
await page.getByRole("checkbox").waitFor()
}

test.describe(`VCheckbox-${type}`, () => {
test("should load with checked state", async ({ page }) => {
const name = "loaded with checked state"
await goToAndWait(page, { checked: true, name })
const checkboxes = page.getByLabel(name)
expect(await checkboxes.all()).toHaveLength(1)
await expect(checkboxes).toBeChecked()
})

test("should load with unchecked state", async ({ page }) => {
const name = "loaded with unchecked state"
await goToAndWait(page, { checked: false, name })
const checkboxes = page.getByLabel(name)
expect(await checkboxes.all()).toHaveLength(1)
await expect(checkboxes).not.toBeChecked()
})

test("should toggle to unchecked when loaded as checked", async ({
page,
}) => {
const name = "loaded with checked state"
await goToAndWait(page, { checked: true, name })
const checkboxes = page.getByLabel(name)
expect(await checkboxes.all()).toHaveLength(1)
await expect(checkboxes).toBeChecked()
await checkboxes.click()
await expect(checkboxes).not.toBeChecked()
})

test("should toggle to checked when loaded as unchecked", async ({
page,
}) => {
const name = "loaded with unchecked state"
await goToAndWait(page, { checked: false, name })
const checkboxes = page.getByLabel(name)
expect(await checkboxes.all()).toHaveLength(1)
await expect(checkboxes).not.toBeChecked()
await checkboxes.click()
await expect(checkboxes).toBeChecked()
})
})
})
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
/* eslint playwright/expect-expect: ["warn", { "additionalAssertFunctionNames": ["expectSnapshot"] }] */

import { Page, test, expect } from "@playwright/test"

import { makeGotoWithArgs } from "~~/test/storybook/utils/args"
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"node": "18.20.2"
},
"devDependencies": {
"@openverse/eslint-plugin": "workspace:0.0.0",
"@openverse/eslint-plugin": "workspace:*",
"prettier": "3.2.5",
"prettier-plugin-tailwindcss": "0.5.14",
"typescript": "5.4.5",
Expand Down
30 changes: 15 additions & 15 deletions packages/js/eslint-plugin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,31 +11,31 @@
},
"dependencies": {
"@intlify/eslint-plugin-vue-i18n": "^2.0.0",
"@typescript-eslint/eslint-plugin": "^6.5.0",
"@typescript-eslint/parser": "^6.5.0",
"@typescript-eslint/utils": "^6.5.0",
"eslint": "^8.48.0",
"@typescript-eslint/eslint-plugin": "^7.8.0",
"@typescript-eslint/parser": "^7.8.0",
"@typescript-eslint/utils": "^7.8.0",
"eslint": "^8.57.0",
"eslint-config-prettier": "^9.1.0",
"eslint-import-resolver-typescript": "^3.6.0",
"eslint-import-resolver-typescript": "^3.6.1",
"eslint-plugin-eslint-comments": "^3.2.0",
"eslint-plugin-import": "^2.28.1",
"eslint-plugin-jsonc": "^2.13.0",
"eslint-plugin-jest": "^27.2.3",
"eslint-plugin-playwright": "^0.22.0",
"eslint-plugin-import": "^2.29.1",
"eslint-plugin-jest": "^28.5.0",
"eslint-plugin-jsonc": "^2.15.1",
"eslint-plugin-playwright": "^1.6.0",
"eslint-plugin-tsdoc": "^0.2.17",
"eslint-plugin-unicorn": "^48.0.1",
"eslint-plugin-vue": "^9.17.0",
"eslint-plugin-vuejs-accessibility": "^2.2.0",
"eslint-plugin-unicorn": "^52.0.0",
"eslint-plugin-vue": "^9.25.0",
"eslint-plugin-vuejs-accessibility": "^2.3.0",
"jsonc-eslint-parser": "^2.4.0",
"typescript": "^5.2.2",
"vue-eslint-parser": "^9.3.1"
"vue-eslint-parser": "^9.4.2"
},
"devDependencies": {
"@eslint/eslintrc": "^2.1.2",
"@eslint/eslintrc": "^3.0.2",
"@swc/cli": "^0.3.0",
"@swc/core": "^1.3.82",
"@swc/jest": "^0.2.29",
"@typescript-eslint/rule-tester": "^6.5.0",
"@typescript-eslint/rule-tester": "^7.8.0",
"babel-plugin-add-module-exports": "^1.0.4",
"jest": "^29.6.4",
"jest-cli": "^29.6.4",
Expand Down
28 changes: 16 additions & 12 deletions packages/js/eslint-plugin/src/configs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { TSESLint } from "@typescript-eslint/utils"
* This configuration is split into sub-modules in this directory
* that are included using `require.resolve`.
*/
export const project: TSESLint.Linter.Config = {
export const project: TSESLint.Linter.ConfigType = {
env: {
browser: true,
node: true,
Expand Down Expand Up @@ -77,18 +77,22 @@ export const project: TSESLint.Linter.Config = {
rules: {
// Superseded by `@openverse/no-unexplained-disabled-test`
"playwright/no-skipped-test": "off",
// Duplicates TypeScript functionality. All our Playwright tests are in TypeScript and type checks will already catch non-string titles.

// The following duplicate TypeScript functionality. All our Playwright tests are in TypeScript and type checks will already catch non-string titles.
"playwright/valid-title": "off",
},
settings: {
playwright: {
additionalAssertFunctionNames: [
// Shared assertion for confirming sent events
"expectEventPayloadToMatch",
// Shared assertion for visual regression tests
"expectSnapshot",
],
},
"playwright/valid-describe-callback": "off",

"playwright/expect-expect": [
"error",
{
assertFunctionNames: [
// Shared assertion for confirming sent events
"expectEventPayloadToMatch",
// Shared assertion for visual regression tests
"expectSnapshot",
],
},
],
},
},
{
Expand Down
Loading