Skip to content

chore(cdp): drive email service tests through a faithful SES fake - #132730

Merged
trunk-io[bot] merged 5 commits into
masterfrom
test/cdp-outside-in-fakes
Nov 10, 2026
Merged

trunk-io[bot] merged 5 commits into
masterfrom
test/cdp-outside-in-fakes

Conversation

@Silthus

@Silthus Silthus commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Note

🔵 Merge risk: low · two-way door

  • No incident path: only tests and test helpers change, so no send in any region runs the diff.
  • Switch-off: none needed: nothing runs in production.

Problem

  • Engineers changing the CDP email send path get tests that check spied SES SDK call arguments, not the mail SES receives.

Changes

  • email.service.test.ts sends through the real SESv2 client to an in-process SES v2 fake, and asserts on the delivered message.
  • The fake rejects what SES rejects, and injects throttles, rejections and dropped connections. The SDK's own retries now run.
  • Email status and message asset rows come back as JSON from recording ingestion outputs.
  • No production code changes.

How did you test this code?

Test rationale: no new behavior. Mutating email.service.ts shows the rewritten tests guard the same regressions:

Stryker, 999 mutants Old tests New tests
Killed 746 745
Kills lost by a rewritten test 0
  • All suite-level differences sit in the unchanged maildev tests. A serial rerun of those lines shows none.
Mutation run details
  • Stryker 9.2 with the Jest runner, perTest coverage and disableBail, so every killing test is recorded.
  • Both runs mutate the same email.service.ts; only the test file differs.
  • Per-test kills are compared by test name. The renamed metric row maps "an unknown error" to "a dropped connection".
  • 60 old tests killed the missing-client mutant only by crashing in spy setup. 113 new tests kill it through a real send.
  • One stub remains: an SES answer without a message id, which SES never sends.
  • Parallel workers raced on one maildev inbox, which explains the maildev differences.
  • Not run: the other CDP suites, which this PR does not touch.

Release status

  • No feature flag controls this change

Docs update

  • None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

Skills: /full-stop, /stamp-pr, /merge-risk, /qa-swarm, /writing-tests, /writing-code-comments, /writing-pr-descriptions

More context for AI agents
  • Session: T3 Code thread; Stryker 9.2 mutation runs on the worktree box.
  • Decisions: fake-ses.ts lives in nodejs/src/cdp/_tests/; recording-ingestion-outputs.ts lives in nodejs/tests/helpers/ beside createMockIngestionOutputs.
  • Decisions: the throttle test freezes Date.now only, so SDK retry timers stay real and the reschedule lands exactly 500 to 1000 ms past it.
  • Decisions: the fake unsets AWS_PROFILE while it runs, because the SDK ignores the env key pair when a profile is set.
  • Review: the first panel round found that the fake accepted a body part without data; the fake now rejects it, and the html-only cases fail against that mutant.
  • Follow-ups: rate-limiter and activation-reporter call assertions in email.service.test.ts; email-status.service.test.ts produce assertions; the CDP consumer suites that use mockQueueInvocations and mockProducer.
  • Read closely: nodejs/src/cdp/_tests/fake-ses.ts, the request validation and fault handling.

🤖 Generated with Claude Code

The email service tests asserted on the arguments of spied SES SDK calls.
They now send through the real SESv2 client to an in-process SES v2 fake
and assert on the delivered message, and they read produced email status
and message asset rows from recording ingestion outputs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Silthus Silthus self-assigned this Nov 10, 2026
@trunk-io

trunk-io Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

😎 Stack merged successfully - details.

@github-actions

github-actions Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PostHog Review · Standard · reviewed fade0df: 1 Should fix.

See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.

Comment thread nodejs/src/cdp/_tests/fake-ses.ts Resolved
Silthus and others added 3 commits November 10, 2026 14:23
The SDK's default credential chain skips the environment key pair when
AWS_PROFILE is set, so a developer with a profile hit their real profile
instead of the fake's static credentials.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SES refuses a subject or body part that has no Data. The fake accepted
one, so an html-only send that also emitted an empty Text part passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PostHog Review · Standard · reviewed e53fb49: no findings.

@Silthus
Silthus marked this pull request as ready for review November 10, 2026 16:34
@parameterai

parameterai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

Risk: No findings

Test-only change: replaces SES SDK call spying in email.service.test.ts with an in-process SES v2 HTTP fake and a helper that records ingestion outputs as JSON. No production code changes, and the fake binds to 127.0.0.1 with dummy credentials, so no new attack surface.

Sentinel reviewed e53fb49 · Review settings

@veria-ai

veria-ai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

PR overview

No security issues were found in this pull request.

Reviews

@stamphog
stamphog Bot requested review from a team November 10, 2026 16:35

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The review did not complete.

Stamphog hit an error and produced no verdict for this commit.

Push a new commit to try again.

stamphog[bot]

This comment was marked as outdated.

@Silthus

Silthus commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

/trunk merge

@Silthus

Silthus commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

/trunk cancel

@Silthus

Silthus commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

/trunk merge

@Silthus

Silthus commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

/trunk cancel

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stamphog
stamphog Bot dismissed their stale review November 10, 2026 19:04

New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PostHog Review · Standard · reviewed 401c9e8: no findings.

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approved.

Contained test-only changes with no production-path edits, guard suppressions, or unresolved substantive review concerns. A current-head agent review also reports no findings.

  • Author wrote 25% of the modified lines and has 8 merged PRs in these paths (familiarity MODERATE).
  • PostHog Review (Standard) reviewed the current head: no findings.
  • posthog[bot] reviewed the current head.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 0L, 0F substantive, 809L/3F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1d-complex (809L, 3F, single-area, chore)
stamphog 2.7.0 .stamphog/policy.yml @ 401c9e8 · reviewed head 401c9e8

@Silthus

Silthus commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

/trunk merge

@trunk-io
trunk-io Bot merged commit 3aabc92 into master Nov 10, 2026
216 checks passed
@trunk-io
trunk-io Bot deleted the test/cdp-outside-in-fakes branch November 10, 2026 19:31
@deployment-status-posthog

deployment-status-posthog Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-11-13 14:46 UTC Run
prod-us ✅ Deployed 2026-11-13 15:05 UTC Run
prod-eu ✅ Deployed 2026-11-13 15:07 UTC Run

This table shows the posthog release only. Hogstation shows every service that this PR reaches, Kargo services included.

Silthus added a commit that referenced this pull request Nov 13, 2026
…ocess SES fake

Master's email service suite now sends through the real SESv2 client to an
in-process SES fake (#132730) and already covers most of this outcome
matrix. Add the cases it lacks there: shadow mode charges the team budget
and sends past an empty one, every to, cc and bcc copy is charged and a
send with more copies than the budget takes all of it and still sends, and
a send that a team suspension, a workflow pause or the suppression list
skips spends neither the workflow pacing nor the team budget.

Drop the Docker SES fake, its CI step, compose service, package scripts
and handbook line, which only served the separate suite.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
trunk-io Bot pushed a commit that referenced this pull request Nov 13, 2026
…fake (#127175)

* chore(workflows): cover the email send outcome matrix

Send test email through a local SES fake, so the send outcome matrix runs
against real SDK requests without mocking SES.

* test(workflows): tighten the email outcome matrix after review

- Use the production Redis pool and drop the TestRedisV2 copy.
- Cap and pacing cases assert their own log reason and a drained bucket, so a limiter outage cannot pass for a denial.
- Rows carry their own setup and expected values; tests with two flows are split.
- Titles name the behavior only, without matrix numbers or fork references.
- Drop the harness self-test and the duplicate SES throttle row.
- Keep the handbook to how to run the suite.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(workflows): name the start command when the local SES fake is down

The SES suite runs in the default Jest set, but the fake sits behind the
compose `ses` profile. A run without it now fails with the command that
starts it, instead of an error that reads like a regression.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(workflows): share one Valkey pool across the SES suite

RedisV2 has no close, so a pool per test left one connection per test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(workflows): start the SES fake in test:full and services:start

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(workflows): close the SES suite's redis client so jest exits

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* chore(cdp): pass the activation reporter in the local SES email tests

EmailService now takes a required WorkflowsActivationReporter (#127135).
The local SES suite built the service without it and stopped compiling
after the master merge. It passes a stub, as the email service suite does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(workflows): move the uncovered email budget cases onto the in-process SES fake

Master's email service suite now sends through the real SESv2 client to an
in-process SES fake (#132730) and already covers most of this outcome
matrix. Add the cases it lacks there: shadow mode charges the team budget
and sends past an empty one, every to, cc and bcc copy is charged and a
send with more copies than the budget takes all of it and still sends, and
a send that a team suspension, a workflow pause or the suppression list
skips spends neither the workflow pacing nor the team budget.

Drop the Docker SES fake, its CI step, compose service, package scripts
and handbook line, which only served the separate suite.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* test(workflows): pin each skip gate's own metric and the full starting budget

Assert that each skipped send records its own gate's metric with no error,
and that the shadow test starts from a full budget, so a send that stops
early for another reason cannot pass as a gate skip.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant