test(serenity): API-level e2e against the Semrush vendor mocks#2709
test(serenity): API-level e2e against the Semrush vendor mocks#2709rainer-friederich wants to merge 9 commits into
Conversation
Aligns the typed Semrush clients with the published Counterfact mock Docker images (only 1.3.0 is published for both), so the serenity IT exercises the SAME contract version we ship. The bump also resolves the `init_status` typed-path that was missing from the prior 1.2.0/1.1.0 client surface. Both are minor, backward-compatible bumps within 1.x. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add usersBaseUrl(env) reading SEMRUSH_USERS_BASE_URL, falling back to SEMRUSH_PROJECTS_BASE_URL when unset, and route the typed User Manager client through it. Factors the trim/https/origin-normalize validation into a shared normalizeBaseUrl() reused by both resolvers (https kept). Lets the User Manager calls point at a separate (mock) host independently of Project Engine — no path-routing reverse proxy — which unblocks local + automated E2E of the serenity sub-workspace flows. Prod/stage/dev Vault config (single host today) keeps working untouched via the fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drive the /serenity/* controller end to end through the public GHCR Counterfact mocks (project-engine + user-manager) in the postgres IT harness, replacing the prior 400/401-only contract suite. - Boot both mocks via the IT docker-compose (own HTTPS port each); the image tag is the INSTALLED client version (setup.js exports SERENITY_*_MOCK_TAG) so the mock can never drift from the client we ship. env.js points SEMRUSH_PROJECTS_BASE_URL / SEMRUSH_USERS_BASE_URL at them with NODE_TLS_REJECT_UNAUTHORIZED=0 for their self-signed certs. - requireImsBearer honours a test-only SERENITY_ALLOW_NON_IMS_AUTH flag so the harness JWT reaches the handlers; sound only vs the mocks, which ignore the forwarded bearer. No deployed environment sets the flag (mirrors SERENITY_ALLOW_WORKSPACE_DELETE). - setup.js waits on the mock control routes and exposes resetSemrushMocks; the factory seeds via resetPostgres. - First increment: route-gate 400s, org-level model/language catalog live via the Project Engine mock, and brand-resolution after the relaxed auth. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…topContainers The IT harness now boots the full stack (Postgres + PostgREST, MinIO, and the two Semrush mocks), so the postgres-specific name was misleading. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
# Conflicts: # package-lock.json # package.json
|
This PR will trigger a patch release when merged. |
…missing mock tag
- requireImsBearer: document that the forwarded IMS bearer is validated AGAIN by
the real Semrush gateway on every upstream call — this proxy is a fail-fast
shape guard, not the auth boundary. Makes explicit why the test-only IMS-type
relaxation never weakens production auth.
- Remove the hardcoded `:-1.3.0` fallback on the mock image tags. The compose now
uses `${...:?}` and fails hard when the tag is unset, and setup.js throws if the
installed client version cannot be resolved. A silent default would test a
different version than the client we ship.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nfig Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @rainer-friederich,
Verdict: Approve - well-structured test infrastructure with sound production changes; only non-blocking notes below.
Complexity: HIGH - large diff; dependency bump + API surface change.
Changes: Adds API-level e2e tests for /serenity/* routes against Semrush vendor mocks, splits the User Manager base URL, and bumps Semrush clients to 1.3.0 (12 files).
Note: CI checks are currently failing - resolve before merge.
Non-blocking (5): minor issues and suggestions
- nit:
resetSemrushMocks()swallows all errors with.catch(() => {})- when the mutating-lifecycle tests land (next increment), a silently failed reset will produce flaky order-dependent tests. Consider checkingresponse.okand throwing on non-2xx. -test/it/postgres/setup.js:141 - suggestion: The
SERENITY_ALLOW_NON_IMS_AUTHenv flag bypasses the IMS-type gate at runtime with no deploy-time structural guard. The defense-in-depth model (Semrush validates upstream) makes this acceptable today, but a debug-level log when the flag is active would make accidental enablement visible in production logs without adding a hard gate. -src/controllers/serenity.js:199 - nit: The old IT suite tested 401 on each endpoint individually (markets, prompts, tags, models, activate, deactivate, DELETE). The new suite proves auth bypass via a single 404 assertion. Unit tests still cover per-endpoint enforcement, so this is a noted tradeoff rather than a gap - consider adding one more brand-level endpoint (e.g. GET prompts) to the e2e suite for breadth. -
test/it/shared/tests/serenity.js - nit:
GET /serenity/languagesasserts onlyexpect(res.body).to.be.an('object')- passes for any JSON object including error responses. Addingexpect(res.body).to.have.property('items')would catch schema drift. -test/it/shared/tests/serenity.js:72 - nit:
usersBaseUrlsilently falls back toSEMRUSH_PROJECTS_BASE_URLwhenSEMRUSH_USERS_BASE_URLis unset. A debug-level log on fallback would help distinguish "intentionally shared host" from "forgot to set the new var" during troubleshooting. -src/support/serenity/rest-transport.js:209
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 59s | Cost: $9.16 | Commit: a467c9c37c5378c132b6fc998089e47f30bd2674
If this code review was useful, please react with 👍. Otherwise, react with 👎.
Add a "Serenity E2E" section explaining how the two Counterfact mock containers are wired here (version-pinned image tags, base-URL split, the test-only auth flag, statefulness/reset, seed alignment) and where they are developed — the upstream spacecat-shared mock docs and source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- resetSemrushMocks: throw on a non-2xx reset instead of swallowing it, so the
mutating-lifecycle increment can't produce flaky order-dependent tests.
- requireImsBearer flag: log once (warn) when SERENITY_ALLOW_NON_IMS_AUTH is
enabled, so an accidental deployed-env enablement is visible in the logs.
- IT: strengthen the /serenity/languages assertion to check the { items: [...] }
shape, and add a second brand-level e2e (GET prompts) for route breadth.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @MysticatBot — addressed the non-blocking notes in
|
There was a problem hiding this comment.
Hey @rainer-friederich,
Verdict: Approve - all prior non-blocking findings addressed; no new issues found on re-review.
Complexity: HIGH - large diff; dependency bump + API surface + infrastructure signals.
Changes: Addresses prior review nits - adds warn log for auth flag, strengthens test assertions, adds GET prompts breadth test, and fixes resetSemrushMocks error propagation (12 files).
Note: CI checks are currently pending - resolve before merge.
Previously flagged, now resolved
resetSemrushMocksnow throws on non-2xx instead of swallowing errorsSERENITY_ALLOW_NON_IMS_AUTHlogs once at warn level when active (surfaces accidental enablement)- Added GET
/serenity/promptsbrand-level e2e test for breadth /serenity/languagesassertion strengthened to check{ items: [...] }envelopeusersBaseUrlfallback log declined (sound reasoning: normal production path, typed helper with no logger, documented in JSDoc)
Non-blocking (3): minor issues and suggestions
- nit: CLAUDE.md documents
--timeout 30000for the full IT glob, but the serenity suite was run with--timeout 60000in the test plan. The harness hook timeouts are independently managed, so the documented command likely works, but consider adding a note for the serenity-specific timeout or updating the value. -CLAUDE.md - nit:
NODE_TLS_REJECT_UNAUTHORIZEDsave/restore inwaitForSemrushMocks()andresetSemrushMocks()is redundant givenbuildEnv()already sets it to'0'for the entire IT process. The save/restore implies scoped mutation that is not actually needed. -test/it/postgres/setup.js:100 - suggestion: The "relaxed auth" 404 tests only assert status code. Asserting the body shape (e.g.,
res.body.error === 'notFound') would distinguish "handler ran and returned a clean 404" from a generic middleware 404 for an unmatched route. -test/it/shared/tests/serenity.js:85
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 41s | Cost: $7.90 | Commit: 40366566cd629331394dd9d0591914237d158317
If this code review was useful, please react with 👍. Otherwise, react with 👎.
1. Abstract
Adds API-level end-to-end tests for the
/serenity/*routes that drive the real controller through the public Semrush vendor mocks (Project Engine + User Manager) in the postgres integration-test harness, replacing the prior 400/401-only contract suite.2. Reasoning
The
/serenity/*surface sits behind IMS-only authentication and a live Semrush gateway, so the existing integration tests could only assert the route gate (400) and the IMS-only contract (401) — never the actual handler behaviour. Now that the Semrush Project Engine and User Manager APIs are published as stateful Counterfact mock Docker images, the routes can be exercised end to end, both locally and in CI, without a real IMS account or a real vendor.3. High-level overview of the changes
requireImsBearerhonours aSERENITY_ALLOW_NON_IMS_AUTHflag that skips the IMS-type gate so the harness's locally-signed JWT can reach the handlers. This is sound only against the mocks, which do not validate the forwarded bearer (the token value never matters). The flag is never written to Vault / any deployed environment — it mirrors the existingSERENITY_ALLOW_WORKSPACE_DELETEopt-in.SEMRUSH_USERS_BASE_URL, falling back toSEMRUSH_PROJECTS_BASE_URLwhen unset. This lets the two mocks live on separate hosts with no path-routing reverse proxy. Production/stage/dev (a single shared host today) are unchanged via the fallback.@adobe/spacecat-shared-project-engine-client/-user-manager-clientfromnode_modulesand exportsSERENITY_PE_MOCK_TAG/SERENITY_UM_MOCK_TAG, which the compose interpolates. The mock is published from the same package as the typed client, so this guarantees the integration test exercises the exact contract version we ship; it can never silently drift. If a client is bumped to a version whose mock image is not yet published, the image pull fails loudly — by design, forcing the mock to be released in lockstep.resetSemrushMocksis already wired for them.4. Required information
5. Affected / used mysticat-workspace projects
6. Additional information outside the code
7. Test plan
npx mocha --require test/it/postgres/harness.js --timeout 60000 test/it/postgres/serenity.test.js— which boots both mocks (tag derived from the installed client version), points the transport at them, and drives the routes through to the live mock responses. Verified green this session.mysticat-cireusable workflow's existingit-postgrestarget — it brings up the same compose (the mock images are public, so no registry credentials are needed) with no workflow change.SEMRUSH_USERS_BASE_URLis unset (fallback to the existing host); the auth flag and the split target are never set in any deployed environment.🤖 Generated with Claude Code