-
Notifications
You must be signed in to change notification settings - Fork 2
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
Mh/more info feedback component testing #2810
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
91f4bce
chore: add explicit data-testid to aid in testing
Mike-Heneghan 9d2ea6a
chore: add explicit aria-label for feedback form input
Mike-Heneghan a228bb9
chore: add id to the input view title
Mike-Heneghan eff4e54
test: the MoreInfoFeedback component functionality and accessibility
Mike-Heneghan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
155 changes: 155 additions & 0 deletions
155
editor.planx.uk/src/components/Feedback/MoreInfoFeedback.test.tsx
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
import "@testing-library/jest-dom/extend-expect"; | ||
|
||
import { fireEvent, waitFor } from "@testing-library/react"; | ||
// eslint-disable-next-line no-restricted-imports | ||
import userEvent from "@testing-library/user-event"; | ||
import { | ||
getInternalFeedbackMetadata, | ||
insertFeedbackMutation, | ||
} from "lib/feedback"; | ||
import React from "react"; | ||
import { axe, setup } from "testUtils"; | ||
|
||
import MoreInfoFeedbackComponent from "./MoreInfoFeedback"; | ||
|
||
jest.mock("lib/feedback", () => { | ||
return { | ||
getInternalFeedbackMetadata: jest.fn(), | ||
insertFeedbackMutation: jest.fn(), | ||
}; | ||
}); | ||
|
||
const scrollIntoView = jest.fn(); | ||
window.Element.prototype.scrollIntoView = scrollIntoView; | ||
|
||
describe("MoreInfoFeedbackComponent presentation and functionality", () => { | ||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
// Initial load | ||
test('Initial loads renders "Yes" and "No" buttons initially', () => { | ||
const { getByText } = setup(<MoreInfoFeedbackComponent />); | ||
expect(getByText("Yes")).toBeInTheDocument(); | ||
expect(getByText("No")).toBeInTheDocument(); | ||
}); | ||
|
||
test("Does not scroll into view on initial render", () => { | ||
setup(<MoreInfoFeedbackComponent />); | ||
expect(scrollIntoView).not.toBeCalled(); | ||
}); | ||
|
||
// Sentiment selection | ||
test("Clicking Yes input form scrolls into view", async () => { | ||
const { getByText } = setup(<MoreInfoFeedbackComponent />); | ||
fireEvent.click(getByText("Yes")); | ||
expect(scrollIntoView).toBeCalled(); | ||
await waitFor(() => { | ||
expect( | ||
getByText("Please help us to improve this service by sharing feedback"), | ||
).toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
test("Clicking No input form scrolls into view", async () => { | ||
const { getByText } = setup(<MoreInfoFeedbackComponent />); | ||
fireEvent.click(getByText("No")); | ||
expect(scrollIntoView).toBeCalled(); | ||
await waitFor(() => { | ||
expect( | ||
getByText("Please help us to improve this service by sharing feedback"), | ||
).toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
// Form submission | ||
test("Submitting feedback changes view to thank you message", async () => { | ||
const { getByText, getByTestId } = setup(<MoreInfoFeedbackComponent />); | ||
|
||
fireEvent.click(getByText("Yes")); | ||
await waitFor(() => { | ||
expect(getByTestId("userCommentTextarea")).toBeInTheDocument(); | ||
}); | ||
|
||
await userEvent.type( | ||
getByTestId("userCommentTextarea"), | ||
"Great help, thanks!", | ||
); | ||
|
||
fireEvent.click(getByText("Send feedback")); | ||
await waitFor(() => { | ||
expect(getInternalFeedbackMetadata).toBeCalled(); | ||
expect(insertFeedbackMutation).toBeCalled(); | ||
}); | ||
|
||
await waitFor(() => { | ||
expect(getByText("Thank you for your feedback.")).toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
/* | ||
We use the `required` property to validate that a user can't submit an empty | ||
comment. | ||
|
||
It doesn't seem to be possible to test that the Browser stops form | ||
submit in the Jest environment. | ||
|
||
Checking for `required` property currently but we could add explicit | ||
validation. | ||
*/ | ||
test("Feedback form requires a comment before submitting", async () => { | ||
const { getByTestId, getByText } = setup(<MoreInfoFeedbackComponent />); | ||
|
||
fireEvent.click(getByText("Yes")); | ||
await waitFor(() => { | ||
expect(getByTestId("userCommentTextarea")).toBeInTheDocument(); | ||
}); | ||
|
||
expect(getByTestId("userCommentTextarea")).toHaveAttribute("required"); | ||
}); | ||
}); | ||
|
||
describe("MoreInfoFeedbackComponent accessibility", () => { | ||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
test("Initial load should have no accessibility violations", async () => { | ||
const { container } = setup(<MoreInfoFeedbackComponent />); | ||
const results = await axe(container); | ||
expect(results).toHaveNoViolations(); | ||
}); | ||
|
||
test("Form view should have no accessability violations", async () => { | ||
const { container, getByText } = setup(<MoreInfoFeedbackComponent />); | ||
fireEvent.click(getByText("Yes")); | ||
|
||
const results = await axe(container); | ||
expect(results).toHaveNoViolations(); | ||
}); | ||
|
||
test("Thank you view should have no accessibility violations", async () => { | ||
const { container, getByText, getByTestId } = setup( | ||
<MoreInfoFeedbackComponent />, | ||
); | ||
|
||
fireEvent.click(getByText("Yes")); | ||
await waitFor(() => { | ||
expect(getByTestId("userCommentTextarea")).toBeInTheDocument(); | ||
}); | ||
|
||
await userEvent.type( | ||
getByTestId("userCommentTextarea"), | ||
"Great help, thanks!", | ||
); | ||
|
||
fireEvent.click(getByText("Send feedback")); | ||
|
||
await waitFor(() => { | ||
expect(getByText("Thank you for your feedback.")).toBeInTheDocument(); | ||
}); | ||
|
||
const results = await axe(container); | ||
expect(results).toHaveNoViolations(); | ||
}); | ||
}); |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test highlights something interesting - while we can't test that the browser stops form submit in Jest, we'd normally test that an input error message becomes visible when trying to proceed without filling out required fields - but it doesn't seem like we've accounted for that with any of these components and are relying on default browser behavior?
I'd expect us to the use the same GOV.UK pattern of red left border and message that we use for all other public-facing inputs - is there a reason we aren't? I think it's very likely this will be picked up in a11y testing as inconsistent and insufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the time this feature was being built I think there were conversations about using the native
required
although to be fair that might have been scoped to the editor rather than the public view.I did have a version with the gov uk style errors but they could never actually be seen by users as the
required
attribute stopped form submission which would be run to show these validation messages so that state could never be reached.As this was the case at the time I thought I'd reduce complexity but not showing any code which I thought a user could never see.
It'll be really interesting to see what the a11y testing comes back with because on one hand not using
required
for a required field seems somewhat off? Although as you say it's inconsistent 🤔