Skip to content

Conversation

@vpremamozilla
Copy link
Collaborator

@vpremamozilla vpremamozilla commented Dec 1, 2025

MPP-4186

also added CI PR check for frontend test coverage:

  • branches: 70%
  • functions: 70%
  • lines: 80%
  • statements: 80%

OLD COVERAGE

Screenshot 2025-12-01 at 2 20 49 PM

NEW COVERAGE

  • pages/vpn-relay (waitlist*) --> 100%
  • pages/_app --> 85.94%
  • pages/contains-tracker-warning-page --> 97.73%
  • pages/flags --> 94.67%
  • pages/tracker-report --> 100%
  • pages/vpn-relay-welcome --> 95%
  • components/localized --> 100%
  • components/reactariall8nprovidor --> 100%
  • components/googleanalyticsworkaround --> 90.32%
  • components/layout --> 87.72%
  • components/icons --> 51.53%
  • components/button --> 96.3%
  • components/image --> 89.29%
  • functions --> 69.66%
  • hooks --> 67.74%
Screenshot 2025-12-08 at 7 48 55 AM

@vpremamozilla vpremamozilla force-pushed the MPP-4186-frontend-test-coverage-misc branch 4 times, most recently from 5c09394 to 0e24053 Compare December 6, 2025 07:20
@vpremamozilla vpremamozilla marked this pull request as ready for review December 6, 2025 07:24
@vpremamozilla vpremamozilla force-pushed the MPP-4186-frontend-test-coverage-misc branch 4 times, most recently from c1db0f3 to 1c13ee4 Compare December 8, 2025 15:27
@EMMLynch EMMLynch requested a review from groovecoder December 9, 2025 16:33
@vpremamozilla vpremamozilla force-pushed the MPP-4186-frontend-test-coverage-misc branch 7 times, most recently from 0710856 to 7cca45c Compare December 11, 2025 18:00
@vpremamozilla vpremamozilla force-pushed the MPP-4186-frontend-test-coverage-misc branch from 7cca45c to 4cf9457 Compare December 11, 2025 19:28
import { LocalizedProps } from "@fluent/react";
import { Localized } from "./Localized";

jest.unmock("./Localized");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this line do anything? Isn't it unmocked when we bring it in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is necessary because Localized is globally mocked in jest.setup.ts, so we need to unmock it here to test the actual component implementation instead of the mock

Copy link
Collaborator

Choose a reason for hiding this comment

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

Appreciate the explanation

Copy link
Collaborator

@joeherm joeherm left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for getting the coverage to a better place!

Copy link
Member

@groovecoder groovecoder left a comment

Choose a reason for hiding this comment

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

I started to go thru this quite a bit, but it seems there's still a lot of LLM-generated duplicate code in here?

Before a human review, it needs some trimming down. For example, with a few prompts I was able to start moving a lot of duplicate code into a new frontend/__mock__/testHelpers.ts which also makes the tests easier to read too. E.g.:

    mockFirstSeenOnce(7);
    mockCookieDismissal("free-7days");

Here's a branch I started to do some refactoring, for some inspiration or ideas: https://github.com/mozilla/fx-private-relay/compare/mozilla:fx-private-relay:MPP-4186-frontend-test-coverage-misc...groovecoder:fx-private-relay:MPP-4186-frontend-test-coverage-misc?expand=1

Comment on lines 811 to 925
it("tracks GA event when user submits 'Very Satisfied' answer", async () => {
const user = userEvent.setup();
const mockProfileData = getMockProfileData({ has_premium: false });
render(<CsatSurvey profile={mockProfileData} />);

const verySatisfiedButton = screen.getByRole("button", {
name: /survey-csat-answer-very-satisfied/,
});
await user.click(verySatisfiedButton);

expect(global.gaEventMock).toHaveBeenCalledWith({
category: "CSAT Survey",
action: "submitted",
label: "Very Satisfied",
value: 5,
dimension3: "Satisfied",
dimension4: "Very Satisfied",
metric10: 1,
metric11: 5,
metric12: 1,
});
});

it("tracks GA event when user submits 'Satisfied' answer", async () => {
const user = userEvent.setup();
const mockProfileData = getMockProfileData({ has_premium: false });
render(<CsatSurvey profile={mockProfileData} />);

const allButtons = screen.getAllByRole("button");
const satisfiedButton = allButtons.find((btn) =>
btn.textContent?.includes("[survey-csat-answer-satisfied]"),
)!;
await user.click(satisfiedButton);

expect(global.gaEventMock).toHaveBeenCalledWith({
category: "CSAT Survey",
action: "submitted",
label: "Satisfied",
value: 4,
dimension3: "Satisfied",
dimension4: "Satisfied",
metric10: 1,
metric11: 4,
metric12: 1,
});
});

it("tracks GA event when user submits 'Neutral' answer", async () => {
const user = userEvent.setup();
const mockProfileData = getMockProfileData({ has_premium: false });
render(<CsatSurvey profile={mockProfileData} />);

const neutralButton = screen.getByRole("button", {
name: /survey-csat-answer-neutral/,
});
await user.click(neutralButton);

expect(global.gaEventMock).toHaveBeenCalledWith({
category: "CSAT Survey",
action: "submitted",
label: "Neutral",
value: 3,
dimension3: "Neutral",
dimension4: "Neutral",
metric10: 1,
metric11: 3,
metric12: 0,
});
});

it("tracks GA event when user submits 'Dissatisfied' answer", async () => {
const user = userEvent.setup();
const mockProfileData = getMockProfileData({ has_premium: false });
render(<CsatSurvey profile={mockProfileData} />);

const dissatisfiedButton = screen.getAllByRole("button", {
name: /survey-csat-answer-dissatisfied/,
})[0];
await user.click(dissatisfiedButton);

expect(global.gaEventMock).toHaveBeenCalledWith({
category: "CSAT Survey",
action: "submitted",
label: "Dissatisfied",
value: 2,
dimension3: "Dissatisfied",
dimension4: "Dissatisfied",
metric10: 1,
metric11: 2,
metric12: -1,
});
});

it("tracks GA event when user submits 'Very Dissatisfied' answer", async () => {
const user = userEvent.setup();
const mockProfileData = getMockProfileData({ has_premium: false });
render(<CsatSurvey profile={mockProfileData} />);

const veryDissatisfiedButton = screen.getByRole("button", {
name: /survey-csat-answer-very-dissatisfied/,
});
await user.click(veryDissatisfiedButton);

expect(global.gaEventMock).toHaveBeenCalledWith({
category: "CSAT Survey",
action: "submitted",
label: "Very Dissatisfied",
value: 1,
dimension3: "Dissatisfied",
dimension4: "Very Dissatisfied",
metric10: 1,
metric11: 1,
metric12: -1,
});
});
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (blocking): there are 5 separate "tracks GA event when user submits" tests with duplicated code. try this refactor:

  test.each([
    { rating: 1, category: "detractor", metric3: -1 },
    { rating: 6, category: "detractor", metric3: -1 },
    { rating: 7, category: "passive", metric3: 0 },
    { rating: 8, category: "passive", metric3: 0 },
    { rating: 9, category: "promoter", metric3: 1 },
    { rating: 10, category: "promoter", metric3: 1 },
  ])("tracks GA event as $category when rating $rating", async ({ rating, category, metric3 }) => {
    const user = userEvent.setup();
    render(<NpsSurvey />);

    const button = screen.getByRole("button", { name: rating.toString() });
    await user.click(button);

    expect(global.gaEventMock).toHaveBeenCalledWith({
      category: "NPS Survey",
      action: "submitted",
      label: category,
      value: rating,
      dimension1: category,
      metric1: 1,
      metric2: rating,
      metric3,
    });
  });

Copy link
Member

@groovecoder groovecoder left a comment

Choose a reason for hiding this comment

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

Please address the few blocking comments.

But to be honest, I changed my tactic from leaving blocking comments to leaving dozens of non-blocking suggestions just so we could get this merged and then refactor in follow-ups.

This is just way too much code. It's very clearly LLM-generated in a bad way - with too much code duplication, no consolidation, and it leans far too strongly towards unit tests when the same coverage could be achieved with less code in a few integration tests.

It became very difficult and exhausting to review so much code.

| `handlers.ts` | HTTP request handlers for all API endpoints |
| `mockData.ts` | Pre-defined mock data for all user states |

**Mock Users Available**: `"demo"`, `"empty"`, `"onboarding"`, `"some"`, `"full"`
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): This line seems a bit out of place. It should move closer to content about mock data or testing user interactions.

Comment on lines 70 to 76
| File | Purpose |
| --------------- | ------------------------------------------- |
| `initialise.ts` | Entry point for MSW setup |
| `browser.ts` | MSW browser worker for component tests |
| `server.ts` | MSW Node.js server for unit tests |
| `handlers.ts` | HTTP request handlers for all API endpoints |
| `mockData.ts` | Pre-defined mock data for all user states |
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): Don't list out every file and purpose or we will have to maintain this list. Just provide single sentence about ALL the files.

Comment on lines 82 to 91
| File | Purpose |
| ----------------------- | ------------------------------------ |
| `l10n.ts` | Localization mock with test matchers |
| `api/profile.ts` | Profile data mock factory |
| `api/aliases.ts` | Alias data mock factory |
| `api/runtimeData.ts` | Runtime data mock factory |
| `api/realPhone.ts` | Real phone number mocks |
| `api/relayNumber.ts` | Relay number mocks |
| `api/inboundContact.ts` | Inbound contact mocks |
| `api/user.ts` | User data mocks |
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): Don't list out every file and purpose or we will have to maintain this list. Just provide single sentence about ALL the files.

Comment on lines 95 to 99
| File | Purpose |
| --------------- | ------------------------------- |
| `Localized.tsx` | Mockable localization component |
| `ImageMock.tsx` | Next.js Image component mock |
| `IconsMock.tsx` | SVG icon mocks |
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): Don't list out every file and purpose or we will have to maintain this list. Just provide single sentence about ALL the files.

Comment on lines 103 to 108
| File | Purpose |
| -------------- | ------------------------------ |
| `flags.ts` | Feature flag testing utilities |
| `getLocale.ts` | Locale function mock |
| `getPlan.ts` | Plan availability mock |
| `cookies.ts` | Cookie handling mock |
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): Don't list out every file and purpose or we will have to maintain this list. Just provide single sentence about ALL the files.

Comment on lines 83 to 89
describe("under axe accessibility testing", () => {
it("passes axe accessibility testing", async () => {
const { baseElement } = render(<Flags />);
const results = await act(() => axe(baseElement));
expect(results).toHaveNoViolations();
}, 10000);
});
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): seems like we could have a single axe test that tests all pages?

Comment on lines 332 to 414
it("displays sender email", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-meta-from-heading], with vars: {}",
),
).toBeInTheDocument();
expect(screen.getByText("[email protected]")).toBeInTheDocument();
});

it("displays received at timestamp", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-meta-receivedat-heading], with vars: {}",
),
).toBeInTheDocument();

const date = new Date(1609459200000).toLocaleString();
expect(screen.getByText(date)).toBeInTheDocument();
});

it("displays total tracker count", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {
"tracker1.com": 5,
"tracker2.com": 3,
"tracker3.com": 2,
},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-meta-count-heading], with vars: {}",
),
).toBeInTheDocument();
});

it("calculates total count correctly", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {
"tracker1.com": 5,
"tracker2.com": 3,
"tracker3.com": 2,
},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
'l10n string: [trackerreport-trackers-value], with vars: {"count":10}',
),
).toBeInTheDocument();
});
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): could be combined into single test.

Comment on lines 418 to 535
it("displays report title", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText("l10n string: [trackerreport-title], with vars: {}"),
).toBeInTheDocument();
});

it("displays logo", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByAltText("l10n string: [logo-alt], with vars: {}"),
).toBeInTheDocument();
});

it("displays confidentiality notice", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-confidentiality-notice], with vars: {}",
),
).toBeInTheDocument();
});

it("displays removal explainer section", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-removal-explainer-heading], with vars: {}",
),
).toBeInTheDocument();
expect(
screen.getByText(
"l10n string: [trackerreport-removal-explainer-content], with vars: {}",
),
).toBeInTheDocument();
});

it("displays trackers explainer section", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-trackers-explainer-heading], with vars: {}",
),
).toBeInTheDocument();
expect(
screen.getByText(
"l10n string: [trackerreport-trackers-explainer-content-part1], with vars: {}",
),
).toBeInTheDocument();
expect(
screen.getByText(
"l10n string: [trackerreport-trackers-explainer-content-part2], with vars: {}",
),
).toBeInTheDocument();
});

it("displays breakage warning", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-breakage-warning-2], with vars: {}",
),
).toBeInTheDocument();
});
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): single test?

Comment on lines 539 to 605
it("displays FAQ section heading", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [trackerreport-faq-heading], with vars: {}",
),
).toBeInTheDocument();
});

it("displays link to full FAQ page", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

const link = screen.getByRole("link", {
name: "l10n string: [trackerreport-faq-cta], with vars: {}",
});
expect(link).toHaveAttribute("href", "/faq");
});

it("displays FAQ questions", () => {
window.location.hash = encodeURIComponent(
JSON.stringify({
sender: "[email protected]",
received_at: 1609459200000,
trackers: {},
}),
);

render(<TrackerReport />);

expect(
screen.getByText(
"l10n string: [faq-question-define-tracker-question], with vars: {}",
),
).toBeInTheDocument();
expect(
screen.getByText(
"l10n string: [faq-question-disable-trackerremoval-question], with vars: {}",
),
).toBeInTheDocument();
expect(
screen.getByText(
"l10n string: [faq-question-bulk-trackerremoval-question], with vars: {}",
),
).toBeInTheDocument();
expect(
screen.getByText(
"l10n string: [faq-question-trackerremoval-breakage-question], with vars: {}",
),
).toBeInTheDocument();
});
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): single test

Comment on lines 68 to 243
describe("page content", () => {
it("displays the headline", () => {
render(<VpnRelayWelcome />);

expect(
screen.getByText(
"l10n string: [vpn-relay-welcome-headline], with vars: {}",
),
).toBeInTheDocument();
});

it("displays the subheadline", () => {
render(<VpnRelayWelcome />);

expect(
screen.getByText(
"l10n string: [vpn-relay-welcome-subheadline], with vars: {}",
),
).toBeInTheDocument();
});
});

describe("Relay panel", () => {
it("displays Relay logo", () => {
render(<VpnRelayWelcome />);

const logos = screen.getAllByAltText(
"l10n string: [logo-premium-alt], with vars: {}",
);
expect(logos.length).toBeGreaterThan(0);
});

it("displays Relay body text", () => {
render(<VpnRelayWelcome />);

expect(
screen.getByText(
"l10n string: [vpn-relay-go-relay-body-3], with vars: {}",
),
).toBeInTheDocument();
});

it("displays link to Relay dashboard", () => {
render(<VpnRelayWelcome />);

const relayLink = screen.getByRole("link", {
name: "l10n string: [vpn-relay-go-relay-cta], with vars: {}",
});

expect(relayLink).toBeInTheDocument();
expect(relayLink).toHaveAttribute("href", "/");
});
});

describe("VPN panel", () => {
it("displays VPN logo", () => {
render(<VpnRelayWelcome />);

const vpnLogos = screen.getAllByAltText(
"l10n string: [logo-premium-alt], with vars: {}",
);
expect(vpnLogos.length).toBeGreaterThan(0);
});

it("displays VPN body text", () => {
render(<VpnRelayWelcome />);

expect(
screen.getByText(
"l10n string: [vpn-relay-go-vpn-body-2], with vars: {}",
),
).toBeInTheDocument();
});

it("displays VPN download link", () => {
render(<VpnRelayWelcome />);

const vpnLink = screen.getByRole("link", {
name: "l10n string: [vpn-relay-go-vpn-cta], with vars: {}",
});

expect(vpnLink).toBeInTheDocument();
});

it("includes correct UTM parameters in VPN link", () => {
render(<VpnRelayWelcome />);

const vpnLink = screen.getByRole("link", {
name: "l10n string: [vpn-relay-go-vpn-cta], with vars: {}",
});

const href = vpnLink.getAttribute("href");
expect(href).toContain("utm_source=");
expect(href).toContain("utm_medium=referral");
expect(href).toContain("utm_campaign=vpn-relay-welcome");
expect(href).toContain("utm_content=download-button");
});

it("opens VPN link in new tab", () => {
render(<VpnRelayWelcome />);

const vpnLink = screen.getByRole("link", {
name: "l10n string: [vpn-relay-go-vpn-cta], with vars: {}",
});

expect(vpnLink).toHaveAttribute("target", "_blank");
});

it("points to Mozilla VPN download page", () => {
render(<VpnRelayWelcome />);

const vpnLink = screen.getByRole("link", {
name: "l10n string: [vpn-relay-go-vpn-cta], with vars: {}",
});

const href = vpnLink.getAttribute("href");
expect(href).toContain("https://vpn.mozilla.org/vpn/download/");
});
});

describe("referring site URL", () => {
it("includes utm_source parameter in VPN link", () => {
render(<VpnRelayWelcome />);

const vpnLink = screen.getByRole("link", {
name: "l10n string: [vpn-relay-go-vpn-cta], with vars: {}",
});

const href = vpnLink.getAttribute("href");
expect(href).toContain("utm_source=");
});

it("properly encodes referring site URL", () => {
render(<VpnRelayWelcome />);

const vpnLink = screen.getByRole("link", {
name: "l10n string: [vpn-relay-go-vpn-cta], with vars: {}",
});

const href = vpnLink.getAttribute("href");
const url = new URL(href!);
const utmSource = url.searchParams.get("utm_source");

expect(utmSource).toBeTruthy();
expect(typeof utmSource).toBe("string");
});
});

describe("layout", () => {
it("uses premium theme", () => {
render(<VpnRelayWelcome />);

expect(screen.getByRole("main")).toBeInTheDocument();
});

it("displays panel art image", () => {
render(<VpnRelayWelcome />);

const images = screen.getAllByRole("img");
expect(images.length).toBeGreaterThan(2);
});

it("renders two panels", () => {
render(<VpnRelayWelcome />);

const relayPanel = screen.getByText(
"l10n string: [vpn-relay-go-relay-body-3], with vars: {}",
);
const vpnPanel = screen.getByText(
"l10n string: [vpn-relay-go-vpn-body-2], with vars: {}",
);

expect(relayPanel).toBeInTheDocument();
expect(vpnPanel).toBeInTheDocument();
});
});
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): so much of this could be in a single test.

@vpremamozilla vpremamozilla force-pushed the MPP-4186-frontend-test-coverage-misc branch from a7392a6 to 82e382c Compare January 6, 2026 07:47
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.

4 participants