fix(resource-deleter): await integration teardown and self-heal setup - #63
Merged
Merged
Conversation
`afterAll` passed a callback to `Promise.each` that called `clearData`
without returning it, so nothing was awaited. Teardown resolved while
the deletes were still in flight and Jest tore the process down
mid-request, leaving fixtures behind in the shared test project.
Once leaked, the residue was permanent: `beforeAll` seeded without
clearing first, so every later run failed with
BadRequest: A duplicate value '"fooCatKey"' exists for field 'key'
and that cascaded to every test in the suite, including ones that
never touch the API.
Return the promise so teardown actually completes, and clear before
seeding so the suite recovers on its own from an interrupted run --
the same pattern personal-data-erasure already uses. The setup timeout
goes to 60s to cover the added clear step.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Awaiting the teardown exposed a second defect it had been hiding: the
clear loop walks resources in declaration order, but commercetools
refuses to delete a resource that is still referenced, so removing
product-types before products fails with
Can not delete a product-type while it is referenced by at least
one product
Previously those rejections were discarded along with the un-returned
promise, so the ordering was never exercised.
Extract a single `clearAllResources` helper that walks the keys in
reverse, removing dependents first -- the order the per-resource
delete tests already assume via `Object.keys(resources).reverse()`.
Teardown timeout matches setup at 60s.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With teardown ordering fixed, the remaining failure was
BadRequest: Product cannot be deleted as long as it is published
in a catalog
`clearData` deleted by id without unpublishing, so any published
product survived cleanup and poisoned later runs. The deleter itself
already handles this (src/main.ts:117), but the test helper did not.
Unpublish first when `masterData.published` is set, then delete with
the bumped version. The unpublish overrides a built request rather
than calling `.post()`, because the update-action union across every
resource builder is too complex for TypeScript to represent -- the
same workaround src/main.ts uses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
packages/resource-deleter/test/integration/cli.spec.tsdiscarded the promise in its teardown:Promise.eachtherefore had nothing to await. Teardown resolved immediately while the deletes were still in flight, and Jest tore the process down mid-request, leaving fixtures behind in the shared test project.The
beforeAlldirectly above it returns correctly (return createData(...), line 64), as does the other teardown at line 278 — this one was the outlier.Why it was not self-correcting
beforeAllseeded without clearing first, so once residue existed it was permanent. Every subsequent run died at fixture setup with:Because the failure is in a
before*hook, it cascades to every test in the suite — includingshould print the module version given the version flag, which never calls the API. That is why recent runs showed 23 failures across 3 suites with a single underlying cause.The fix
returnthe promise inafterAllso teardown actually completes.beforeAll, so an interrupted run heals itself on the next attempt. This mirrors whatpersonal-data-erasurealready does (await Promise.all([clearData(...)])before creating).Verification
yarn typecheck→ exit 0yarn lint→ exit 0The integration tests themselves need project credentials, which are only injected in CI, so they could not be exercised locally — CI on this PR is the real check.
Note
This fixes the leak going forward. Any
fooCatKeycategory (and the published product behindProduct cannot be deleted as long as it is published in a catalog) already stranded in the shared test project still needs a one-time manual purge, or the new clear-before-seed step will handle it on the first run — whichever lands first.🤖 Generated with Claude Code