Skip to content

test(projects): Harden regression coverage for delete_project and delete_project_key#113792

Merged
BYK merged 1 commit intomasterfrom
BYK/test/harden-stripe-projects-rpcs
Apr 24, 2026
Merged

test(projects): Harden regression coverage for delete_project and delete_project_key#113792
BYK merged 1 commit intomasterfrom
BYK/test/harden-stripe-projects-rpcs

Conversation

@BYK
Copy link
Copy Markdown
Member

@BYK BYK commented Apr 23, 2026

Follow-up to #113596. A post-merge retrospective review surfaced three
modest coverage gaps — all on test hardening, no source changes. The
prior review rounds caught the genuinely nasty bugs (QuerySet.delete
bypassing outbox, missing rename_on_pending_deletion); this PR
strengthens the guards around those fixes so a future "optimization"
that re-introduces either class of bug fails loudly.

Changes

1. test_delete_project_rolls_back_status_and_schedule_when_rename_fails

Renamed from test_delete_project_rolls_back_on_failure. The
transaction wrapper's stated purpose is to prevent partial state on
failure, but the original test only asserted the status rollback — not
that the CellScheduledDeletion row was also rolled back. Without
this, a phantom scheduled deletion could fire against a still-ACTIVE
row 30 days later. Now asserts both invariants.

2. test_delete_project_rolls_back_when_schedule_fails (new)

The original test only covered the rename-raises path. This covers
the other failure point inside the transaction —
CellScheduledDeletion.schedule() raising. Without this, an
optimization that moves schedule() outside the transaction could
silently regress the guard.

3. test_delete_project_refuses_internal_project (new)

is_internal_project() protects settings.SENTRY_PROJECT from
deletion (deleting it would break Sentry's own error reporting). The
branch was completely untested — a future refactor removing the guard
would pass CI. Uses mock.patch.object(Project, 'is_internal_project')
since SENTRY_PROJECT is a Django settings integer that doesn't match
freshly-created test project ids.

4. test_delete_project_key_emits_outbox (new)

The entire reason we fetch-then-delete (rather than
ProjectKey.objects.filter(...).delete()) is that QuerySet.delete()
bypasses the per-instance override on CellOutboxProducingModel that
emits the replication outbox. Explicitly asserts the CellOutbox row
is produced so a "performance optimization" back to QuerySet.delete()
fails loudly — replica tables on the control silo would otherwise
retain stale rows and keep validating deleted DSNs.

Creation-side outbox test was skipped: ReplicatedCellModel flushes
outboxes immediately on save() by default (flush=None → outbox_context → flush=True), so the row never lingers long enough
for a post-call assertion. The delete path explicitly uses
flush=False which is what makes the assertion viable there.

Process note

This PR is the result of a post-merge subagent review pass done to
catch anything the three prior reviews missed (self-review, sentry-seer,
@dashed). The reviewer surfaced one Important item (I1, now split into
tests #1 and #2) and two minor items (M3, M4 → tests #3 and #4), plus
9 verified-clean items. See
#113596 for full context.

No source changes, no behavior changes. 20/20 tests pass locally.

…ete_project_key

Follow-up to #113596. A post-merge retrospective review surfaced three modest coverage gaps — all on test-hardening, no source changes. The prior review rounds caught the genuinely nasty bugs (QuerySet.delete bypassing outbox, missing rename_on_pending_deletion); this PR strengthens the guards around those fixes so a future 'optimization' that re-introduces either class of bug fails loudly.

Changes:

1. test_delete_project_rolls_back_status_and_schedule_when_rename_fails (renamed from test_delete_project_rolls_back_on_failure): the transaction wrapper's stated purpose is to prevent partial state on failure, but the original test only asserted the status rollback — not that the CellScheduledDeletion row was also rolled back. Without this, a 'phantom' scheduled deletion could fire against a still-ACTIVE row. Now asserts both invariants.

2. test_delete_project_rolls_back_when_schedule_fails (new): the original test only covered the rename-raises path. This covers the OTHER failure point inside the transaction — CellScheduledDeletion.schedule raising — so an optimization that moves schedule() outside the transaction can't silently regress the guard.

3. test_delete_project_refuses_internal_project (new): is_internal_project() protects settings.SENTRY_PROJECT from deletion (deleting it would break Sentry's own error reporting). The branch was completely untested — a future refactor removing the guard would pass CI.

4. test_delete_project_key_emits_outbox (new): the entire reason we fetch-then-delete (rather than ProjectKey.objects.filter(...).delete()) is that QuerySet.delete() bypasses the per-instance override on CellOutboxProducingModel that emits the replication outbox. Explicitly asserts the CellOutbox row is produced so a 'performance optimization' back to QuerySet.delete() fails loudly. Creation-side outbox test was skipped because ReplicatedCellModel flushes outboxes immediately on save by default.

No source changes; no behavior changes. 20/20 tests pass locally.

Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
@BYK BYK requested a review from a team as a code owner April 23, 2026 13:57
@BYK BYK requested a review from dashed April 23, 2026 13:57
@BYK BYK merged commit 4ba7c4c into master Apr 24, 2026
51 checks passed
@BYK BYK deleted the BYK/test/harden-stripe-projects-rpcs branch April 24, 2026 08:32
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.

3 participants