MPP-4293 - increase frontend test coverage for Mask Management pages and components (part 2) #5761
Conversation
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
13 times, most recently
from
7f6584a to
469ba07
Compare
vpremamozilla
changed the title
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
2 times, most recently
from
a0b6b67 to
a5431a8
Compare
vpremamozilla
changed the title
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
5 times, most recently
from
6725e1b to
2c44214
Compare
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
7 times, most recently
from
065a5bb to
a622e94
Compare
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
15 times, most recently
from
6f6e80d to
c7cf1a0
Compare
| name: "String for modal-custom-alias-picker-form-submit-label-2", | ||
| }); | ||
| fireEvent.click(submit); | ||
| expect(onPick).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
suggestion (blocking): The test says it shows a validation error, but the only expect call is expect(onPick).not.toHaveBeenCalled(). Should change the test name or add an expectation for the validation error.
| jest.mock("../../../../public/illustrations/holiday.svg", () => ({ | ||
| __esModule: true, | ||
| default: { src: "holiday.svg" }, | ||
| })); |
There was a problem hiding this comment.
question (non-blocking): whoa, I've never seen mocking a static image asset. What does this do?
There was a problem hiding this comment.
yeah this is just so we dont have to load the actual svg (just returns a string path)
| it("calls onSubmit from label editor", () => { | ||
| const { onUpdate } = setup(); | ||
| fireEvent.click(screen.getByText("Submit Label")); | ||
| expect(onUpdate).toHaveBeenCalledWith({ description: "New Label" }); |
There was a problem hiding this comment.
question (non-blocking): This seems odd to hard-code an expected string that's defined in the mock. It makes me realize this test is mocking LabelEditor, AliasDeletionButton, and BlockLevelSlider components too. Which is great for unit-testing this particular Alias component. But I don't see corresponding tests for those other components, and LabelEditor is re-mocked again from MaskCard.test.tsx.
Could this be mocking too much? Again - this is great for isolating these individual components with unit tests. But sometimes it's better to define the "unit" under test not necessarily as each individual component or file, but a "unit of functionality" which might be larger (or smaller) than an individual component or file.
Just something to consider - maybe we don't mock all the additional components and let this test cover them too?
There was a problem hiding this comment.
Yeah, fair point; I mocked the children just to keep <Alias /> focused and easy to test in isolation, but we could definitely add an integration test with the real components if you think its necessary
| fireEvent.click( | ||
| screen.getByRole("button", { name: "profile-details-expand" }), | ||
| ); | ||
| expect(onChangeOpen).toHaveBeenCalledWith(true); |
There was a problem hiding this comment.
praise: Oh, now I see and I like this pattern to have the setup function return an object of the jest function assigned to component properties. Makes it nice to deconstruct the one in which each test is interested, and assert its behavior.
|
|
||
| test("shows composed domain hint using profile subdomain and runtime mozmailDomain", () => { | ||
| renderModal({ aliasGeneratedState: false }); | ||
| expect(screen.getByText("@vanessa.mozmail.com")).toBeInTheDocument(); |
There was a problem hiding this comment.
suggestion (non-blocking): move this hard-coded vanessa value into a const? (Also remove PII from the code? 😜 )
| subdomain: "vanessa", | ||
| aliasGeneratedState: false, | ||
| findAliasDataFromPrefix: (_prefix: string) => | ||
| ({}) as unknown as import("../../../hooks/api/aliases").AliasData, |
There was a problem hiding this comment.
question (blocking): This module has import { AliasData } from "../../hooks/api/aliases"; at the top already. Why do we need to re-import it from this separate path here?
There was a problem hiding this comment.
good call out, updated
| .fn() | ||
| .mockImplementation( | ||
| (_prefix: string) => | ||
| ({}) as unknown as import("../../../hooks/api/aliases").AliasData, |
There was a problem hiding this comment.
question (blocking): This module has import { AliasData } from "../../hooks/api/aliases"; at the top already. Why do we need to re-import it from this separate path here?
| const findAliasDataFromPrefix = jest | ||
| .fn() | ||
| .mockReturnValue( | ||
| {} as unknown as import("../../../hooks/api/aliases").AliasData, |
There was a problem hiding this comment.
question (blocking): This module has import { AliasData } from "../../hooks/api/aliases"; at the top already. Why do we need to re-import it from this separate path here?
| }); | ||
| }); | ||
|
|
||
| describe("isAddressValid", () => { |
There was a problem hiding this comment.
question (non-blocking): should we add a test for the unicode/emoji characters too?
There was a problem hiding this comment.
Some blocking comments.
But also a couple overall themes ...
-
This seems to mock a LOT of code out from each test, which makes it a set of very strongly isolated unit tests. That can be good. But it can also lead to missing regressions later. E.g., if one of the mocked components changes its behavior, the test may continue to work fine because the mock keeps working. We want to balance strongly isolated unit tests with tests that will tell us if we break things between components.
-
There seem to be a number of different techniques used to do the same operations - e.g., mock l10n, or check for text, etc. It looks like a lot of this code was generated with AI assistance, and that the AI model "temperature" made it generate a variety of techniques and not settle on a single technique. Can you run back thru it (an AI assistant might even be good at this part too) and consolidate to make things more consistent?
| type IsFlagActiveFn = ( | ||
| runtimeData: RuntimeData | undefined, | ||
| name: string, | ||
| ) => boolean; |
There was a problem hiding this comment.
question (non-blocking): We can't use typeof isFlagActive for this type?
There was a problem hiding this comment.
we defined it explicitly to stay decoupled from the module and avoid typing errors
| screen.getByText(matchText("Copied!", "profile-label-copied")), | ||
| ).toHaveAttribute("aria-hidden", "false"); | ||
| act(() => { | ||
| jest.advanceTimersByTime(1000); |
There was a problem hiding this comment.
comment (non-blocking): TIL jest.advanceTimersByTime. neat.
| }); | ||
|
|
||
| test("replies are hidden for free users; promo option shows lock messaging and upgrade link", () => { | ||
| isFlagActiveMock.mockReturnValue(false); |
There was a problem hiding this comment.
question (non-blocking): Why does this test use isFlagActiveMock.mockReturnValue(false); while the previous test does:
isFlagActiveMock.mockImplementation(
(_rd, name) => name === "tracker_removal",
);
?
There was a problem hiding this comment.
one needs all the flags, while the other only needs one
| }); | ||
| }); | ||
|
|
||
| test("block level label reflects current state for all / promotions / none", () => { |
There was a problem hiding this comment.
suggestion (non-blocking): Could these expectations be moved into the previous test?
There was a problem hiding this comment.
keeping them separate makes it easier to diagnose errors
| screen.getByText(matchText("Created", "profile-label-created")), | ||
| ).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByText(/^Rendered\(2024-01-15T10:00:00Z\)$/), |
There was a problem hiding this comment.
question (non-blocking): Why is the ^Rendered in this regex?
There was a problem hiding this comment.
The ^...$ anchors ensure an exact text match (Rendered(2024-01-15T10:00:00Z)) instead of a partial/substring match
| it("renders welcome header and continue button", () => { | ||
| renderView(); | ||
| expect(screen.getByText("phone-masking-splash-header")).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByText("phone-masking-splash-subheading"), | ||
| ).toBeInTheDocument(); | ||
| expect( | ||
| screen.getByText("phone-masking-splash-continue-btn"), | ||
| ).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
suggestion (non-blocking): combine with previous test.
| expect(screen.getByText(/\(234\)\s*567\s*-\s*890/)).toBeInTheDocument(); | ||
| expect(screen.getByText(/\(987\)\s*654\s*-\s*321/)).toBeInTheDocument(); |
There was a problem hiding this comment.
question (non-blocking): why regex here instead of strict hard-coded values? is it really possible that space characters could legitimately end up in the number?
There was a problem hiding this comment.
regex is added as a safeguard against formatting differences
| @@ -18,7 +18,7 @@ export type FlagNames = | |||
| | "holiday_promo_2023" | |||
| | "four_mask_limit_upsell"; | |||
|
|
|||
| type WaffleFlag = [FlagNames, boolean]; | |||
| export type WaffleFlag = [FlagNames, boolean]; | |||
There was a problem hiding this comment.
comment (non-blocking): this is likely the only thing causing the conflict.
| await userEvent.click(copyButton); | ||
|
|
||
| await userEvent.click(copyButton); |
There was a problem hiding this comment.
question (blocking): why call click twice?
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
3 times, most recently
from
e001aa3 to
d76a79a
Compare
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
3 times, most recently
from
c163fb6 to
2e1158e
Compare
… Mask Management pages and components
vpremamozilla
force-pushed
the
MPP-4193-test-code-coverage-mask-management-part-2
branch
from
2e1158e to
4b0bb01
Compare
CLOSED - PR MOVED
MPP-4293 - Part Two
Test code coverage improved on the following:
OLD COVERAGE
NEW COVERAGE