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

Chore: introduce Fishery as object factory and yarn dev:msw #1023

Open
wants to merge 21 commits into
base: alerts-per-check
Choose a base branch
from

Conversation

VikaCep
Copy link
Contributor

@VikaCep VikaCep commented Dec 19, 2024

Closes: #850

This PR introduces the following changes:

Fishery: Simplifies mocking data in tests, removing the need for hard-coded objects. It works alongside Faker to automatically generate dynamic test data for more flexible testing scenarios.

It defines factories for:

  • All different check types
  • Probes
  • Check alerts

These factories are used to create fixtures in src/test/fixtures/probes.ts, src/test/fixtures/checks.ts, and src/test/fixtures/checkAlerts.ts.

Mocked API responses: Adds the ability to start the app with mocked API responses. To use this, run yarn dev:msw. This runs a service worker that uses the same handlers as in test .

Example of running the app using MSW mocked responses:

Screen.Recording.2024-12-20.at.15.44.15.mov

@VikaCep VikaCep self-assigned this Dec 19, 2024
@VikaCep VikaCep marked this pull request as ready for review December 19, 2024 16:30
@VikaCep VikaCep requested a review from a team as a code owner December 19, 2024 16:30
@VikaCep VikaCep requested review from ckbedwell and removed request for a team December 19, 2024 16:30
@VikaCep VikaCep changed the title Chore: introduce Fishery as object factory Chore: introduce Fishery as object factory and yarn dev:msw Dec 20, 2024
@VikaCep VikaCep requested a review from a team as a code owner January 7, 2025 15:16
@VikaCep VikaCep force-pushed the sm-fixtures-fishery branch from 786a5e2 to a5be00b Compare January 7, 2025 19:35
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

This is ace but there are some things we can tighten up! 🔧

src/components/App.tsx Outdated Show resolved Hide resolved
src/test/db.ts Outdated Show resolved Hide resolved
src/test/db.ts Outdated Show resolved Hide resolved
src/test/db.ts Outdated
probe: Factory.define<Probe>(() => ({
id: faker.number.int({ min: 1, max: 999999 }),
name: faker.string.uuid(),
public: faker.datatype.boolean(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be opinionated and generate Private probes by default. I was experimenting with the probe definitions and found this was a source of flaky tests because the app behaviour changes dependent on if they are public or private probes.

Copy link
Contributor

Choose a reason for hiding this comment

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

As per my comment below on reflecting upon this, I think we should have two factories. One that creates public probes and one that creates private probes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried creating two separate factories—one for PrivateProbes and one for PublicProbes—but since the IDs for public and private probes can’t overlap, we need to manage their creation within a single factory. This requires using a transient parameter, similar to how it’s done with checks. But having two separate factories was complicating things, so I found it more straightforward to just use one factory that creates private probes by default. For fixtures that require a public probe, we can simply set the public: true property.

src/test/db.ts Outdated Show resolved Hide resolved
src/test/db.ts Outdated Show resolved Hide resolved
src/test/fixtures/checkAlerts.ts Outdated Show resolved Hide resolved
src/test/fixtures/checks.ts Outdated Show resolved Hide resolved
src/test/fixtures/probes.ts Outdated Show resolved Hide resolved
src/test/fixtures/probes.ts Show resolved Hide resolved
- move worker initialization to module.ts
- use sequence for Fishery ids
- simplify probes and checks mocks creation
@VikaCep VikaCep requested a review from ckbedwell January 10, 2025 21:47
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

One thing to change then let's get this in! 🎉

src/module.ts Outdated
Comment on lines 32 to 34
if (process.env.NODE_ENV === 'development' && process.env.REACT_APP_MSW) {
setupWorker(...handlers).start();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just needs a small modification so it gets tree-shaken properly and doesn't affect the production bundle size:

  1. Move line 33 and associated imports to a file like startServerWorker.ts:
import { setupWorker } from 'msw';
import { handlers } from 'test/handlers';

setupWorker(...handlers).start();
  1. change line 33 to: await import(./startServiceWorker);

Then we don't include the additional 850kb overhead our mocks have 😄

Comparing the webpack console output before and after making this change. Before 2.87MiB. After 2.02MiB

@VikaCep VikaCep requested a review from ckbedwell January 23, 2025 14:58
Copy link
Contributor

@ckbedwell ckbedwell left a comment

Choose a reason for hiding this comment

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

LGTM ❤️

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