Skip to content

Refactor to data driven http mocking using msw/data - #2411

Draft
shikanime wants to merge 1 commit into
mainfrom
wphetsinorath/push-pwkotpylwpnp
Draft

Refactor to data driven http mocking using msw/data#2411
shikanime wants to merge 1 commit into
mainfrom
wphetsinorath/push-pwkotpylwpnp

Conversation

@shikanime

Copy link
Copy Markdown
Member

Change-Id: Ia3fb6a1e3acaa141721e1f06a2dd6a656a6a6964

Issues liées

Issues numéro:


Quel est le comportement actuel ?

Quel est le nouveau comportement ?

Cette PR introduit-elle un breaking change ?

Autres informations

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr>
Change-Id: Ia3fb6a1e3acaa141721e1f06a2dd6a656a6a6964
@github-actions github-actions Bot added the built label Aug 4, 2026
@shikanime

Copy link
Copy Markdown
Member Author

Review: PR #2411 — Refactor to data-driven HTTP mocking using msw/data

Verdict: REQUEST CHANGES

CI unit-tests are currently FAILING (the only red check on the PR), and my local reproduction confirms the suite does not even load. I ran the two changed spec files against the head revision in an isolated worktree.

Blocker — tests don't run (this is the CI failure)

apps/server-nestjs/src/modules/sonarqube/sonarqube-client.service.spec.ts:19-24

Invariant Violation: Failed to parse a model definition for "group":
model is missing a primary key. Did you forget to mark one of its
properties using the "primaryKey" function?

Root cause: the model objects are built as { id: primaryKey(String), ...makeSonarqubeGroup() }. makeSonarqubeGroup() (and makeSonarqubeUser/makeSonarqubeProject) return their own id field, and the spread overwrites the primaryKey marker with a plain string. @mswjs/data then sees no primary key and throws at module load. This crashes the whole file (0 tests collected).

Blocker (logic) — once the crash is fixed, 3 tests still fail

Even after fixing the spread order, the data-driven model has two real correctness defects:

  1. Auto-generated primary key leaks into returned records. With a real PK, db.user.getAll() / db.project.getAll() include an id field the fixtures don't have, so toEqual([user]) / toEqual([project]) fail (usersSearch + projectsSearch). Fix: assert against { ...user, id: expect.any(String) } or strip id.
  2. Token-generate assertion is impossible to satisfy. The data-driven handler regenerates a fresh token server-side (makeSonarqubeGeneratedToken() inside the handler), but the test asserts result.token === generated.token (the test's local fixture). These never match → userTokensGenerate fails. Fix: assert result.token is a string and that it was persisted (db.token.count({ where: { token: { equals: result.token } } })).

Net: with only the spread-order fix, 4/12 sonarqube tests fail. All 12 (and the 5 vault tests) pass only after the PK-order + PK-generator + assertion fixes.

Blocker (test isolation) — module-level db is never reset

sonarqube-client.service.spec.ts:19 and vault-client.service.spec.ts:16 declare db = factory({...}) at module scope. There is no db.seed/db rebuild between tests, so records created in one it persist into the next. Tests currently pass only because their data happens not to collide; this is order-dependent and will flake. Fix: rebind db = buildDb() in beforeEach (vault has the same issue with secret/token).

Warning — assertion-coverage regression

The original server.use(...) tests asserted the request params the client actually sent (e.g. usersCreate checked login/email/local; usersDeactivate checked anonymize; userGroupsCreate checked name; permissionsAddGroup checked projectKey absence). The data-driven rewrite drops most of these — handlers assert params internally, but those expect()s live in the handler closures and only run on the happy path, and several params (email, local, anonymize, projectKey) are no longer asserted at all. Keeping request-contract assertions in the test body is safer than burying them in mocks.

Nit — import order (auto-fixed by CI --fix, but worth noting)

import { factory, primaryKey } from '@mswjs/data' must precede @nestjs/testing per perfectionist/sort-imports; eslint --fix (lint script) handles this, so it's non-blocking.

Nit — @mswjs/data@0.16.2 is deprecated

The lockfile records deprecated: Package no longer supported. Fine for test-only tooling, but flagging it in case a non-deprecated alternative is preferred.


Suggested minimal fix (verified green: 17/17)

const buildDb = () => factory({
  group:   { ...makeSonarqubeGroup(),        id: primaryKey(() => faker.string.uuid()) },
  user:    { ...makeSonarqubeUser(),         id: primaryKey(() => faker.string.uuid()) },
  project: { ...makeSonarqubeProject(),      id: primaryKey(() => faker.string.uuid()) },
  token:   { ...makeSonarqubeGeneratedToken(), id: primaryKey(() => faker.string.uuid()) },
})
let db = buildDb()
// in beforeEach: db = buildDb()

Plus the two assertion adjustments (id: expect.any(String) on search equals, and result.token instead of generated.token on generate).

Re-request review once CI unit-tests pass.

@shikanime shikanime moved this to Backlog in Cloud Pi Native Aug 4, 2026
@shikanime shikanime moved this from Backlog to Experimental in Cloud Pi Native Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants