test(icon-button): add playwright tests #6369

divyajindel · 2023-10-12T14:32:43Z

Proposed behaviour

Refactor cypress tests to playwright

Current behaviour

Tests are currently using cypress

Checklist

QA

Tested in CodeSandbox/storybook
Add new Cypress test coverage if required
Carbon implementation matches Design System/designs
UI Tests GitHub check reviewed if required

Additional context

N/A

Testing instructions

Run Icon-Button tests in Playwright Test Runner to check if the icon-button.pw.tsx file passed
Run all other tests in Playwright Test Runner to check none of the other *.pw.tsx files have regressed

codesandbox-ci · 2023-10-12T14:35:15Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit dff1b79:

Sandbox	Source
carbon-quickstart	Configuration
carbon-quickstart-typescript	Configuration

stephenogorman · 2023-10-17T13:58:45Z

src/components/icon-button/icon-button.pw.tsx

+    const element = await iconLocator;
+    await element.focus();
+    await expect(element).toHaveCSS(
+      "box-shadow",


I don't think the line const element = await iconLocator; is necessary? The following should do the same:
const iconLocator = page.getByTestId("icon").locator("..");
await iconLocator.focus();
await expect(iconLocator).toHaveCSS(

stephenogorman · 2023-10-17T14:01:43Z

src/components/icon-button/icon-button.pw.tsx

+import { HooksConfig } from "../../../playwright";
+import { checkAccessibility } from "../../../playwright/support/helper";
+
+test.describe("when focused", () => {


This describe should probably be changed to something more meaningful like:
test.describe("check IconButton component focus outlines and border radius", () => {

stephenogorman · 2023-10-17T14:02:22Z

src/components/icon-button/icon-button.pw.tsx

+    );
+
+    const iconLocator = page.getByTestId("icon").locator("..");
+    const element = await iconLocator;


Same as previous comment, const element = await iconLocator; is unnecessary.

stephenogorman · 2023-10-17T14:06:38Z

src/components/icon-button/icon-button.pw.tsx

+      "aria-label",
+      CHARACTERS.STANDARD
+    );
+    await iconLocator.isVisible();


I think await expect(iconLocator).toBeVisible() is more reliable than this.

stephenogorman · 2023-10-17T14:07:00Z

src/components/icon-button/icon-button.pw.tsx

+    await mount(<IconButtonComponent />);
+
+    const iconLocator = page.getByTestId("icon");
+    await iconLocator.isVisible();


Same as previous comment.

stephenogorman · 2023-10-17T14:08:32Z

src/components/icon-button/icon-button.pw.tsx

+
+    const iconLocator = page.getByTestId("icon").locator("..");
+    await iconLocator.isDisabled();
+    await iconLocator.getAttribute("disabled");


This would be preferable:
await expect(iconLocator).toBeDisabled()
await expect(iconLocator).toHaveAttribute("disabled", /.*/);

stephenogorman · 2023-10-17T14:09:35Z

src/components/icon-button/icon-button.pw.tsx

+    );
+    const iconLocator = page.getByTestId("icon").locator("..");
+    await iconLocator.focus();
+    await iconLocator.blur();


This line await iconLocator.blur(); is not needed.

stephenogorman · 2023-10-17T14:11:15Z

src/components/icon-button/icon-button.pw.tsx

+  });
+});
+
+test(`render with the expected border radius`, async ({ mount, page }) => {


You could move this test to the first describe, with the border radius tests.

sianford · 2023-10-17T17:46:08Z

src/components/icon-button/icon-button.pw.tsx

+  }) => {
+    await mount(<IconButtonComponent aria-label={CHARACTERS.STANDARD} />);
+
+    const iconLocator = page.getByTestId("icon").locator("..");


This const is used several times. Can it be declared once outside of the tests?

sianford · 2023-10-17T18:10:44Z

src/components/icon-button/icon-button.pw.tsx

+      }
+    );
+
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times. Can it be declared once earlier then just iconLocator called where needed?

Hi Sian, IMO we can't do this as we are using page to create this locator inside the test.

sianford · 2023-10-17T18:11:01Z

src/components/icon-button/icon-button.pw.tsx

+  }) => {
+    await mount(<IconButtonComponent aria-label={CHARACTERS.STANDARD} />);
+
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

sianford · 2023-10-17T18:11:40Z

src/components/icon-button/icon-button.pw.tsx

+  }) => {
+    await mount(<IconButtonComponent disabled />);
+
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

sianford · 2023-10-17T18:11:50Z

src/components/icon-button/icon-button.pw.tsx

+        }}
+      />
+    );
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

sianford · 2023-10-17T18:11:57Z

src/components/icon-button/icon-button.pw.tsx

+        }}
+      />
+    );
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

sianford · 2023-10-17T18:12:04Z

src/components/icon-button/icon-button.pw.tsx

+        }}
+      />
+    );
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

sianford · 2023-10-17T18:12:25Z

src/components/icon-button/icon-button.pw.tsx

+        }}
+      />
+    );
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

sianford · 2023-10-17T18:12:30Z

src/components/icon-button/icon-button.pw.tsx

+        }}
+      />
+    );
+    const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

sianford · 2023-10-17T18:12:46Z

src/components/icon-button/icon-button.pw.tsx

+          }}
+        />
+      );
+      const iconLocator = page.getByTestId("icon").locator("..");


As above, this const is used several times.

src/components/icon-button/icon-button.pw.tsx

tomdavies73 · 2023-10-26T07:28:51Z

src/components/icon-button/icon-button.pw.tsx

+    }) => {
+      await mount(<IconButtonComponent aria-label={CHARACTERS.STANDARD} />);
+
+      const iconLocator = page.getByTestId("icon").locator("..");


suggestion: instead of page.getByTestId("icon").locator(".."); could we not just add a general button index/locator? Infact there already is one in locators.ts (line 18), you'd just have to import the index into the file and do iconButton = button(page) etc. I won't comment this on every instance you've used the above locator but if I were you i'd think about replacing it with this approach.

Also, as the new button(page) locator is fairly short and simple you can probably move away from the const approach and just do await button(page).focus() and then your two expects expect(button(page)...) if you do this test by test we can cut out a good fair few lines and reduce complexity

tomdavies73 · 2023-10-26T07:35:36Z

src/components/icon-button/icon-button.pw.tsx

+);
+
+test.describe("check props for IconButton component", () => {
+  test(`should render IconButton with aria-label prop`, async ({


suggestion: we've already informed future readers what component is being tested via the describe on the line above, I don't think it's necessary to include the component name on every test description within this describe block.

tomdavies73 · 2023-10-26T07:58:14Z

src/components/icon-button/icon-button.pw.tsx

+test.describe(
+  "check IconButton component focus outlines and border radius",
+  () => {
+    test(`should have the expected styling when the focusRedesignOptOut is false`, async ({


praise: I like that you've combined the focus/radius tests into one describe here, makes things a lot clearer

tomdavies73 · 2023-10-26T08:00:59Z

src/components/icon-button/icon-button.pw.tsx

+    await checkAccessibility(page);
+  });
+
+  test(`should pass accessibility tests for children prop`, async ({


suggestion (non-blocking): i know it was written like this in the cypress test, but i'd change this test description. There is no children prop and in the test there's only one child. Something like should pass accessibility tests when rendered with a child should do

tomdavies73 · 2023-10-26T08:03:43Z

src/components/icon-button/icon-button.pw.tsx

+    await expect(iconLocator).toBeVisible();
+  });
+
+  test(`should render IconButton with children prop`, async ({


suggestion (non-blocking): i know it was written like this in the cypress test, but i'd change this test description. There is no children prop and in the test there's only one child. Something like should render with a child should do

tomdavies73 · 2023-10-26T08:05:03Z

src/components/icon-button/icon-button.pw.tsx

+
+    const iconLocator = page.getByTestId("icon").locator("..");
+    await expect(iconLocator).toBeDisabled();
+    await expect(iconLocator).toHaveAttribute("disabled", /.*/);


suggestion: I think the expect above should be enough for this, instead of the attribute tests

tomdavies73 · 2023-10-26T08:12:00Z

src/components/icon-button/icon-button.pw.tsx

+      mount,
+      page,
+    }) => {
+      await mount(<IconButtonComponent aria-label={CHARACTERS.STANDARD} />);


question (non-blocking): I can see we're passing an aria-label for these focus and radius tests, is there a specific reason for this, or can they be removed. I can see we do this in the cypress test too btw, so I understand why you've included them. Just could get rid if they're not neccessary

tomdavies73 · 2023-10-27T10:45:59Z

playwright/components/index.ts

@@ -25,6 +26,10 @@ export const commonDataElementInputPreview = (page: Page) => {
  return page.locator(COMMMON_DATA_ELEMENT_INPUT);
 };

+export const iconButton = (page: Page) => {


suggestion: this can just be called button instead of iconButton as the locator is just finding type="button" and has no overlap with the IconButton component, i'd rename this here at least 👍

tomdavies73 · 2023-10-27T10:48:26Z

src/components/icon-button/icon-button.pw.tsx

+import React from "react";
+import { test, expect } from "@playwright/experimental-ct-react17";
+import IconButtonComponent from "./component.test-pw";
+import { iconButton } from "../../../playwright/components/index";


suggestion: if you've changed the index/locator name from iconButton to button you can then do this for the import import { button as iconButton } from "../../../playwright/components/index". This way you can keep the name of the locator in your tests and ensure they're still clear what's being tested etc.

tomdavies73 · 2023-10-27T10:55:21Z

src/components/icon-button/icon-button.pw.tsx

+});
+
+test.describe("check accessibility tests for IconButton component", () => {
+  test(`should render aria-label prop for accessibility`, async ({


suggestion: I think these test descriptions have been changed and i'm not 100% sure why, I think they should all follow something like should pass accessibility tests when rendered with an aria-label, should pass accessibility tests when rendered with a child, should pass accessibility tests when disabled etc.

carbonci · 2023-11-02T15:44:35Z

🎉 This PR is included in version 123.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

divyajindel requested review from a team as code owners October 12, 2023 14:32

divyajindel marked this pull request as draft October 12, 2023 14:32

divyajindel self-assigned this Oct 12, 2023

divyajindel added Pending QA Squad Front End icon-button playwright labels Oct 12, 2023

stephenogorman requested changes Oct 17, 2023

View reviewed changes

sianford reviewed Oct 17, 2023

View reviewed changes

divyajindel force-pushed the icon-button-playwright-refactor branch from 21cd3ac to e7a227c Compare October 18, 2023 11:00

stephenogorman requested changes Oct 19, 2023

View reviewed changes

src/components/icon-button/icon-button.pw.tsx Outdated Show resolved Hide resolved

divyajindel force-pushed the icon-button-playwright-refactor branch from e7a227c to 4db9127 Compare October 19, 2023 09:57

stephenogorman previously approved these changes Oct 23, 2023

View reviewed changes

tomdavies73 self-requested a review October 25, 2023 09:11

tomdavies73 reviewed Oct 26, 2023

View reviewed changes

divyajindel dismissed stephenogorman’s stale review via 30da757 October 26, 2023 14:48

divyajindel force-pushed the icon-button-playwright-refactor branch from 4db9127 to 30da757 Compare October 26, 2023 14:48

tomdavies73 reviewed Oct 27, 2023

View reviewed changes

test(icon-button): add playwright tests

6dc0a93

divyajindel force-pushed the icon-button-playwright-refactor branch from 30da757 to 6dc0a93 Compare October 27, 2023 13:00

tomdavies73 approved these changes Oct 27, 2023

View reviewed changes

stephenogorman approved these changes Oct 31, 2023

View reviewed changes

divyajindel marked this pull request as ready for review November 1, 2023 12:55

divyajindel added 2 commits November 1, 2023 12:55

Merge branch 'master' into icon-button-playwright-refactor

361d0c5

Merge branch 'master' into icon-button-playwright-refactor

dff1b79

divyajindel marked this pull request as draft November 1, 2023 13:57

divyajindel marked this pull request as ready for review November 1, 2023 13:57

divyajindel added Passed QA and removed Pending QA labels Nov 1, 2023

divyajindel merged commit aeb530d into master Nov 1, 2023
18 checks passed

divyajindel deleted the icon-button-playwright-refactor branch November 1, 2023 14:24

carbonci added the released label Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(icon-button): add playwright tests #6369

test(icon-button): add playwright tests #6369

divyajindel commented Oct 12, 2023 •

edited

Loading

codesandbox-ci bot commented Oct 12, 2023 •

edited

Loading

stephenogorman Oct 17, 2023

stephenogorman Oct 17, 2023

stephenogorman Oct 17, 2023

stephenogorman Oct 17, 2023

stephenogorman Oct 17, 2023

stephenogorman Oct 17, 2023

stephenogorman Oct 17, 2023

stephenogorman Oct 17, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

divyajindel Oct 18, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

sianford Oct 17, 2023

tomdavies73 Oct 26, 2023

tomdavies73 Oct 26, 2023

tomdavies73 Oct 26, 2023

tomdavies73 Oct 26, 2023

tomdavies73 Oct 26, 2023

tomdavies73 Oct 26, 2023 •

edited

Loading

tomdavies73 Oct 26, 2023

tomdavies73 Oct 26, 2023

tomdavies73 Oct 27, 2023

tomdavies73 Oct 27, 2023

tomdavies73 Oct 27, 2023

carbonci commented Nov 2, 2023

test(icon-button): add playwright tests #6369

test(icon-button): add playwright tests #6369

Conversation

divyajindel commented Oct 12, 2023 • edited Loading

Proposed behaviour

Current behaviour

Checklist

QA

Additional context

Testing instructions

codesandbox-ci bot commented Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomdavies73 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonci commented Nov 2, 2023

divyajindel commented Oct 12, 2023 •

edited

Loading

codesandbox-ci bot commented Oct 12, 2023 •

edited

Loading

tomdavies73 Oct 26, 2023 •

edited

Loading