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: add vitest-axe & update frontend tests #3549

Merged
merged 9 commits into from
Aug 26, 2024
Merged

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Aug 22, 2024

What is working here

  • Re-adds editor package.json script test:silent which CI Github workflows expect
  • Swaps out jest-axe for vitest-axe & updates all a11y tests in src/@planx/components & beyond
    • Please note we have to install the pre-release version in order to access toHaveNoViolations (see thread)
    • Adds import "vitest-axe/extend-expect"; to src/setupTests.ts
    • Imports axe directly from vitest-axe and drops special config from src/testUtils.ts - does this seem okay? I don't think I've hit the SVG issue described in the code comment there yet / guessing it's old & since been resolved?
  • Updates LOTS of other jest -> vi mocks along the way
  • Adds per-test timeouts where necessary (see especially some public List component tests or editor Pay tests)
  • Un-comments tests & .skip()s problematic ones so they're easier to globally search for in the final migration stretch!

Screenshot from 2024-08-23 20-37-59

What's not working yet / let's address in a follow up PR ?

  • All tests relying on mockedAxios are skipped - getting a mockResolvedValueOnce function not defined error (FileUploadAndLabel, Send)
  • All tests relying on vi.spyOn() for the "default" function are skipped (see thread, haven't quite sorted solution yet) (PlanningConstraints)
  • Airbrake notifier tests - again a more complicated mock / spy to figure out!

Some unexpected linting stuff picked up with these changes has added extra noise to an already big PR, sorry!

Copy link

github-actions bot commented Aug 22, 2024

Removed vultr server and associated DNS entries

@jessicamcinchak jessicamcinchak marked this pull request as ready for review August 22, 2024 19:44
@jessicamcinchak jessicamcinchak requested a review from a team August 22, 2024 19:44
@jessicamcinchak jessicamcinchak marked this pull request as draft August 23, 2024 08:11
@jessicamcinchak jessicamcinchak removed the request for review from a team August 23, 2024 08:11
@jessicamcinchak
Copy link
Member Author

@jessicamcinchak jessicamcinchak changed the title wip: vitest-axe feat: add vitest-axe & update frontend tests Aug 23, 2024
@jessicamcinchak jessicamcinchak marked this pull request as ready for review August 23, 2024 18:40
@@ -107,10 +106,10 @@ describe("Pay component - Editor Modal", () => {
expect(getByDisplayValue("myValue")).toBeInTheDocument();
});

it("allows new values to be added", async () => {
it("allows new values to be added", { timeout: 20000 }, async () => {
Copy link
Member Author

@jessicamcinchak jessicamcinchak Aug 26, 2024

Choose a reason for hiding this comment

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

rather than a single jest.setTimeout(2000) at the top of the file, I've gone for per-test timeouts like this as it seems useful/helpful to keep a more specific pulse on which frontend interactions are slow running / easier to work through them & optimise over time?

if preferred, we still have a per-file option of vi.setConfig({ testTimeout: 20000 })

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach works well for me! Easier to come back and revisit a test at a time, than a whole suite at a time.

Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

Thanks so much for this - a whole lot covered and tidied up here.

A lot of the linting issues fixed have come from some of my recent PRs, will check what's going on with Husky and try to run git commit from a standard directory.

Let's merge this to dp/vite and then fix outstanding issues from there ✅

state.saveToEmail,
state.$public,
]);
const [sessionId, saveToEmail, $public, passport, govUkPayment, flowName] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite a few Confirmation related changes in this PR - expecting these to go away once dp/vite has rebased to main 👍

@@ -41,7 +41,7 @@ describe("FileUploadAndLabel - Editor Modal", () => {
expect(screen.getAllByText("File")).toHaveLength(1);
});

it.skip("allows an Editor to add multiple rules", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! Worth us probably checking out all existing skips() once this vite + vitest migration is done and maybe opening GitHub issues for them.

@@ -107,10 +106,10 @@ describe("Pay component - Editor Modal", () => {
expect(getByDisplayValue("myValue")).toBeInTheDocument();
});

it("allows new values to be added", async () => {
it("allows new values to be added", { timeout: 20000 }, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach works well for me! Easier to come back and revisit a test at a time, than a whole suite at a time.


expect(result).toEqual({
intersectingConstraints: defaultIntersectingConstraints,
nots: defaultNots,
});
});

// Intentional skip, not handling these edge cases yet!
Copy link
Contributor

Choose a reason for hiding this comment

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

Helpful comment!

Comment on lines +160 to +162
vi.spyOn(window, "location", "get").mockReturnValue(
mockWindowLocationObject,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This might be something we can put in a beforeEach() (not for this PR, just a reminder to come back and take a look at this!)

Also getting a few linting issues in this file, will flag with @RODO94 tomorrow or open a quick PR here.

image

Comment on lines +22 to +29
// export const axe = configureAxe({
// rules: {
// // Currently, jest-axe does not correctly evaluate this rule due to an issue with jsdom
// // https://github.com/dequelabs/axe-core/issues/2587
// // To pass this test, non-decorative MUI icons should always use the 'titleAccess' prop
// "svg-img-alt": { enabled: false },
// },
// });
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to know we don't need this - if we see an error with titleAccess again or failing a11y tests we can revisit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will properly delete in follow-up PRs rather than leave commented-out here !

@jessicamcinchak jessicamcinchak merged commit 35b02c3 into dp/vite Aug 26, 2024
9 of 10 checks passed
@jessicamcinchak jessicamcinchak deleted the jess/vite-axe branch August 26, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants