Skip to content

test(integration): pre-clean fixture rows by stable marker, not in-memory ids - #2012

Open
jakebromberg wants to merge 1 commit into
mainfrom
chore/issue-2011
Open

test(integration): pre-clean fixture rows by stable marker, not in-memory ids#2012
jakebromberg wants to merge 1 commit into
mainfrom
chore/issue-2011

Conversation

@jakebromberg

Copy link
Copy Markdown
Member

Closes #2011

Problem

tests/integration/enrichment-worker-streaming-reask.spec.js cleans up its fixture rows only in afterAll, keyed on an in-memory insertedAlbumIds array. If the process dies before afterAll runs — routine under jest.config.json's forceExit: true — the array is lost and the rows survive.

The orphans aren't inert. They're library rows with a Various-Artists-shaped artist_name joined to album_metadata rows carrying a non-null apple_music_url, which satisfies jobs/va-apple-music-url-remediation's albumMetadataNet exactly. On a persistent dev database, a later local --dry-run sizing of that job silently counts leftover test fixtures as production candidates — defeating the entire point of a dry-run.

Fix

Added a beforeAll pre-clean to each affected describe block that deletes by the distinctive fixture marker already present in the seeded data, instead of relying on the in-memory id array:

  • enrichment-worker-streaming-reask.spec.js (both describe blocks) — library.album_title LIKE 'bs1915-reask-test-%'. album_metadata.album_id cascades from library.id (verified against information_schema, not assumed), so one DELETE FROM library covers both tables.
  • artist-unicode-dedup.spec.jsartists.artist_name LIKE 'ZZDEDUP %'
  • artist-unicode-dedup-merge.spec.jsartists.artist_name LIKE 'ZZMERGE%'

The existing afterAll/afterEach cleanup is untouched — the pre-clean is a recovery path, not a replacement for cleaning up after yourself. No change to what any spec asserts.

Why the two artist-unicode-dedup specs

The issue named enrichment-worker-streaming-reask.spec.js as the donor and asked for a sweep of tests/integration/ for the same shape. Most afterAll/afterEach-only specs don't correspond to a coarse, unwindowed candidate net the way this pattern does. Two clear matches turned up:

  • artist-unicode-dedup.spec.js and artist-unicode-dedup-merge.spec.js seed genuinely fold-duplicate artists rows (that's the fixture under test) for jobs/artist-unicode-dedup — a one-shot job with the identical dry-run-then---execute shape as va-apple-music-url-remediation. A crashed run leaves real merge candidates behind for the next local --dry-run sizing to silently count.
  • flowsheet-ghost-row-sweep.spec.js looked like a candidate (same job shape, same table family) but turned out to already be immune: every test runs inside withRollback, so a crashed process loses the whole transaction along with the rows. No change needed.

The remaining ~65 files matching a bare afterAll/afterEach grep are almost all recurring-cron fixtures (concerts, catalog-popularity, flowsheet-metadata-backfill, etc.) whose production jobs run on a schedule rather than via an operator-eyeballed --dry-run count, or already scope their own candidate queries with an explicit id/scope predicate. Left alone here rather than ballooning this diff; flagging as a candidate follow-up if the convention proves worth generalizing further.

A finding along the way

The issue said to verify the album_metadata-cascades-from-library claim against the migration rather than assume it — that held. Applying the same discipline to the artists-referencing tables in the two dedup specs surfaced a real discrepancy: shared/database/src/schema.ts declares genre_artist_crossreference.artist_id as onDelete: 'cascade', but querying information_schema.referential_constraints against a real migrated database shows NO ACTION. I didn't chase that down further — the two new pre-cleans sidestep it entirely by deleting children before parents explicitly (mirroring the order each file's own existing cleanup hook already uses), the same defensive posture the artist-unicode-dedup-merge.spec.js file's existing comment already took for exactly this reason. Worth its own look if the schema/migration drift is unexpected.

Verification

Ran directly against real Postgres (migrations applied, real FK constraints) rather than trusting the SQL by inspection: inserted fixture rows matching each new marker to simulate a crashed run, ran each new pre-clean statement, and confirmed the rows (and cascaded/child rows) are gone, the statements are idempotent on a second run, and unrelated seed data is untouched. Separately confirmed a naive parent-first delete on the dedup fixture genuinely fails with a foreign_key_violation, so the child-then-parent ordering is load-bearing, not decorative.

Could not run the actual Jest integration suite end to end. npm run ci:env's Docker image build for apps/backend/apps/auth fails at npm install --omit=dev with 401 Unauthorized … npm.pkg.github.com/@wxyc/shared — a stale local NPM_TOKEN, not a code problem, and it does not predict CI. jest's globalSetup gates every integration spec (including a pure-DB one) on backend:8081 + auth:8083 health, so there was no way to run the actual spec files without those images. The Postgres-only verification above is a real substitute for correctness of the SQL, but it is not the same as the specs passing, and I did not run the "twice in a row with afterAll disabled" acceptance check against the real spec files.

Checks run

Check Result
npm run format:check pass
npm run lint pass (0 errors, 835 pre-existing warnings)
npm run typecheck pass
node --check on all three touched files pass
Manual Postgres simulation (see above) pass
npm run ci:testmock / real Jest integration run not run — local NPM_TOKEN auth failure blocks the Docker image build (see above)

GitHub Actions is in a confirmed major_outage as of 2026-08-06 ~11:30 PT — this PR will show zero checks and none were re-triggered or polled.

Follow-up for PR #2008

#2008 adds tests/integration/va-apple-music-url-remediation-invalidate.spec.js, which inherits the same afterAll-only pattern from this donor. Left commented on that PR to adopt this convention on rebase, per the sequencing in #2011 — not stacked, since #2008 is already blocked on the CI outage.

…mory ids

Integration specs that only clean up in afterAll, keyed on an in-memory id
array, lose that array if the process dies first — routine under
jest.config.json's forceExit: true. The orphans aren't inert: a crashed
enrichment-worker-streaming-reask run leaves library rows with a
Various-Artists-shaped artist_name joined to album_metadata rows carrying a
non-null apple_music_url, which satisfies jobs/va-apple-music-url-remediation's
albumMetadataNet exactly, so a later --dry-run sizing of that job on a
persistent dev database silently counts leftover test fixtures as production
candidates.

Add a beforeAll pre-clean to each affected describe block that deletes by the
distinctive fixture marker already present in the seeded data (album_title
LIKE 'bs1915-reask-test-%', artist_name LIKE 'ZZDEDUP %' / 'ZZMERGE%') instead
of the in-memory array, so a prior crashed run's rows are found with no prior
knowledge. The existing afterAll/afterEach cleanup is left in place; the
pre-clean is a recovery path, not a replacement.

Verified against real Postgres rather than assumed: album_metadata.album_id
does cascade from library.id, so the streaming-reask pre-clean is one
statement. artists is NOT uniformly cascaded, though — genre_artist_
crossreference.artist_id and library.artist_id are both NO ACTION despite
schema.ts's genre_artist_crossreference declaration reading cascade, an
apparent schema/migration drift caught by querying information_schema
directly. The two artist-unicode-dedup pre-cleans delete children before
parents explicitly, in the same order their own existing cleanup hooks
already use, rather than relying on cascade.

Swept tests/integration/ for the same shape (afterAll/afterEach cleanup keyed
on an in-memory id array with no predicate-based pre-clean). Most don't
correspond to a coarse, unwindowed candidate net the way this pattern does;
flowsheet-ghost-row-sweep.spec.js looked like a candidate but turned out to
already be immune (every test runs inside withRollback, so a crashed process
loses the transaction along with the rows). artist-unicode-dedup.spec.js and
artist-unicode-dedup-merge.spec.js are the one clear match found: both seed
genuinely fold-duplicate artists rows for jobs/artist-unicode-dedup, a
one-shot job with the identical dry-run-then-execute shape as
va-apple-music-url-remediation.
@jakebromberg

Copy link
Copy Markdown
Member Author

One correction to the rationale in the PR description, so a cold reader doesn't inherit a wrong premise: the enrichment-worker-streaming-reask.spec.js orphans were not va-apple-music-url-remediation dry-run candidates. That spec hardcodes artist_name = 'Chuquimamani-Condori', which folds to chuquimamani-condori and fails albumMetadataNet's V/A regex. The candidate-inflation argument holds for the spec PR #2008 adds (which seeds V/A rows), not for this donor.

The change is still correct and worth landing — for a better reason than the issue gave. Both artist-unicode-dedup specs were genuinely non-re-runnable: matchArtistId is a LIMIT 1 with no ORDER BY, so a leftover row can win it; and findDuplicateGroups() is a global fold-key scan, so leftover NFC/NFD rows join the new fold group and displace survivorId. Both break under forceExit: true and both are fixed here. Full reasoning on #2011.

Nothing to change in the diff.

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.

Integration specs clean up only in afterAll, so a crashed run leaves fixtures that match a job's production candidate net

1 participant