feat(project-engine-client): add tag: dimension mock fixtures#1780
feat(project-engine-client): add tag: dimension mock fixtures#1780byteclimber wants to merge 4 commits into
Conversation
Adds seed/fixture support for the `tag:` open tag dimension, a sibling of `category:` with the same 1-level-tree shape (serenity-docs#26). The mock's `createAIOTagMock` is already dimension-agnostic; this wires a `tag:` root + bare child through `peHierarchy`/`buildSeed` into both seed hierarchies and `SEED_IDS`, and documents `tag:` alongside `category:`. Deliberately narrow scope: does NOT fix the `sha256(name)`-alone tag-id derivation (gated on the unrun serenity-docs#26 §9 G1 live probe). Adds cross-referencing "pending live-probe confirmation" comments in tag-id.js, stateful.js, and the aio/tags route header. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Request changes - E2E tests break because the new seed data is visible to existing root-listing assertions.
Complexity: LOW - small fixtures-only diff, single package.
Changes: Adds tag: dimension seed fixtures (root + child) alongside existing category: fixtures in the project-engine-client Counterfact mock (7 files).
Note: CI checks are currently failing - resolve before merge.
Must fix before merge
- [Critical] E2E tests not updated for new
tag:root in seed - 6 assertions fail -mock/seeds.js:221(details inline) - [Important]
TWO_HIERARCHIESseed's newtagTagshas no test assertions -mock/seeds.js:299(details inline)
Non-blocking (1): minor issues and suggestions
- nit:
SeedProjecttypedef'stagsdescription still only mentionscategory:but the collection now holds multiple dimensions -mock/seeds.js:~116
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 42s | Cost: $7.04 | Commit: 2a9ba34e8904ef32d064af242a066c1db7b838a1
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| @@ -208,7 +221,7 @@ const peHierarchy = (cfg) => buildSeed({ | |||
| domain: cfg.domain, | |||
There was a problem hiding this comment.
issue (blocking): The spread [...cfg.categoryTags, ...cfg.tagTags] adds the new tag:Trail Running root to WORKSPACE_WITH_DATA. The E2E test suite (project-engine-mock.e2e.js) queries roots via parent_id: "" and hardcodes expected counts/names based on only category: roots existing.
6 failing E2E assertions:
- Line 835: expects
total: 2, gets 3 (tag:Trail Runningroot now included) - Line 873: expects
total: 3, gets 4 (same root-count off-by-one) - Line 910: expects
total: 1after reset, gets 2 (both seeded roots remain) - Lines 1013/1098:
have.memberslists onlycategory:roots, actual includestag:Trail Running - Line 1225: expects only
category:Running Shoesafter delete, getstag:Trail Runningtoo
Fix: Update the 6 E2E test assertions to include the new tag:Trail Running root in their expected counts and member lists. Also rebase onto main and regenerate package-lock.json (lockfile drift is causing the Test job to fail independently).
| benchmarkId: 'e7f8a9b0-c1d2-4e3f-8a4b-6c7d8e9f0112', | ||
| brandUrlId: 'f8a9b0c1-d2e3-4f4a-8b5c-7d8e9f011223', | ||
| categoryTags: aioTags(['category:Laufschuhe', 'category:Trailrunning']), | ||
| tagTags: aioTags(['tag:Laufschuhe', 'tag:Trailrunning']), |
There was a problem hiding this comment.
issue (blocking): The second hierarchy gains tagTags: aioTags(["tag:Laufschuhe", "tag:Trailrunning"]) but the two-hierarchies test block in seeds.test.js is not updated to assert that these tags appear in the collection. The WORKSPACE_WITH_DATA path gets thorough assertions (root/child structure, parent_id linkage, SEED_IDS verification), but the second hierarchy's new tags are unverified.
Fix: Add assertions in the two-hierarchies test block verifying ops.tags.list(...) for the second hierarchy includes the tag:Laufschuhe and tag:Trailrunning entries alongside the existing category tags.
…or tag: seed Addresses MysticatBot review on #1780: - Critical: the boot seed's new `tag:Trail Running` root now appears in every empty-parent_id root listing, so the 6 E2E assertions that asserted an exact root set are updated to include it; adds an E2E test reading the baked tag: nested taxonomy (root -> child Ultra) end-to-end. - Important: TWO_HIERARCHIES seed's tagTags now has unit assertions. - Nit: SeedProject `tags` typedef documents both open dimensions. - Also syncs package-lock.json (data-access 4.1.0) to satisfy the CI drift gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
This PR will trigger a minor release when merged. |
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Approve - all prior findings addressed, clean re-review.
Complexity: HIGH - medium diff; dependency signal from lockfile change.
Changes: Adds tag: dimension seed fixtures (root + child) alongside existing category: fixtures in the project-engine-client Counterfact mock (9 files).
Previously flagged, now resolved
- E2E assertions now account for the new
tag:Trail Runningroot across all 6 root-listing scenarios TWO_HIERARCHIESseed now has dedicated test assertions verifying both dimensions coexistSeedProjecttypedef description updated to mention both open tag dimensions
Non-blocking (0): no issues found
No issues identified on re-review. The fix commit correctly bumps all assertion counts, adds a dedicated E2E test for the tag: nested taxonomy read-back, and adds TWO_HIERARCHIES coverage for both dimensions. Test coverage is thorough across all three levels (factory unit, seed integration, E2E contract).
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 58s | Cost: $4.53 | Commit: 521b67786bfe6a099985e7df79aa88e71b79bcf6
If this code review was useful, please react with 👍. Otherwise, react with 👎.
…fixtures # Conflicts: # package-lock.json
There was a problem hiding this comment.
Hey @byteclimber,
Verdict: Approve - clean re-review, no new issues introduced by rebase.
Complexity: HIGH - medium diff; API surface signal (routes path pattern).
Changes: Adds tag: dimension seed fixtures (root + child) alongside existing category: fixtures in the project-engine-client Counterfact mock (8 files).
Previously flagged, now resolved
- E2E assertions now account for the new
tag:Trail Runningroot across all 6 root-listing scenarios TWO_HIERARCHIESseed now has dedicated test assertions verifying both dimensions coexistSeedProjecttypedef description updated to mention both open tag dimensions
Non-blocking (2): minor issues and suggestions
- nit:
seeds.test.jsassertion usesdeep.equalon tag name array, coupling to insertion order - if order is not a contract,have.memberswould be more resilient -test/mock/seeds.test.js:80 - nit:
tag-id.jspending-probe comment references "two other spots" by description only - grep-friendly but could benefit from the function/symbol names inline (already partially done viaops.tags.upsertMany) -mock/tag-id.js:~37
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 53s | Cost: $2.97 | Commit: 6ca5c0061b80b07adf0590b2e2eb0b8af0ed8cdb
If this code review was useful, please react with 👍. Otherwise, react with 👎.
1. Abstract
Adds seed/fixture support for the
tag:open tag dimension to thespacecat-shared-project-engine-clientCounterfact mock.tag:is a sibling of the existingcategory:dimension (serenity-docs#26) — the same 1-level-tree shape (atag:<name>root owning bareparent_id-linked children), living in the same per-projecttagscollection. This is a deliberately narrow-scope, fixtures-only change.2. Reasoning
The Semrush "serenity" AIO tag taxonomy exposes an OPEN
<dimension>:prefix.category:is already modelled end-to-end in the mock;tag:is a new sibling dimension (serenity-docs#26) that the api-service tag-dimension work needs to read back from seeded projects. Rather than hand-write literals, the mock's factory/seed machinery is extended so atag:root + child seed exactly mirrors thecategory:fixture and is usable in cross-repo integration tests the same way.3. High-level overview of the changes
mock/factories.js—createAIOTagMock's doc comment now documents thetag:dimension alongsidecategory:: they are sibling OPEN dimensions with the same shape (not sub-types of one another), and the factory is dimension-agnostic (it only storesname/parent_id; the dimension lives in the name prefix). No code change — the factory already builds either dimension.mock/seeds.js— addsTAG_ROOT_NAME/TAG_CHILD_NAME/TAG_ROOT_TAG_ID/TAG_CHILD_TAG_IDmirroring theCATEGORY_*constants; threads a newtagTagsparam throughpeHierarchy(concatenated into the one per-projecttagscollection next tocategoryTags); seeds atag:root + bare child inWORKSPACE_WITH_DATAand a flattag:list in the secondtwo-hierarchieshierarchy; exposestagRootTagId/tagChildTagIdonSEED_IDS. Thetag:child name is deliberately DISTINCT from thecategory:child (UltravsTrail) to avoid the name-alone tag-id collapse (see below).sha256(name)-alone id-derivation limitation, cross-referencing each other and the serenity-docs#26 §9 G1 probe:mock/tag-id.js,mock/stateful.js(ops.tags.upsertMany), and theaio/tagsroute header.test/mock/factories.test.js(provescreateAIOTagMockbuilds atag:root/child dimension-agnostically) andtest/mock/seeds.test.js(asserts both dimensions coexist in the seededtagscollection with correct root/child parenting).Deliberately NOT in this PR (scope gate)
The known
sha256(name)-alone id-derivation bug — which would cause a same-named child under two DIFFERENT roots to collapse to ONE mock id — is not fixed here. It is gated on an unrun live probe (serenity-docs#26 §9 G1) confirming whether real Semrush actually salts a child tag's id byparent_idor keys bynamealone. Fixing the mock before that probe runs would bet on the unconfirmed answer and make the mock diverge from reality in exactly the way that would hide the bug the probe exists to catch. The three pending-probe comments flag this so a future contributor fixing one spot doesn't miss the others.There is also no
GET .../aio/tags/{tag_id}single-tag-by-id operation (onlyPATCH) — confirmed absent in both the swagger and the mock routes; this is a known, accepted gap, not touched here.4. Required information
category:1-level tree), feat(project-engine-client): model 1-level nested AIO tags in the mock #1758/project-engine-client mock: AIO tag ids embed %3A (name-derived) and don't round-trip through URLs — blocks nested children-drill / PATCH-by-id IT #1760 (nested taxonomy + opaque tag ids)5. Affected / used mysticat-workspace projects
spacecat-api-service— the matchingtag:dimension work depends on this fixture. Its integration tests will need to bump theirpackage.jsondependency on@adobe/spacecat-shared-project-engine-clientonce this is released/versioned, to pick up the newtag:seed fixture.6. Additional information outside the code
seeds.test.js). The full package unit suite,tsctype-check (// @ts-checkgate), and lint were run locally.7. Test plan
npm run lint,npm run test:types, andnpm testfor the package — all green, 100% coverage retained (branches 100 as the package enforces).tag:fixture (SEED_IDS.tagRootTagId/tagChildTagId) in their integration tests.8. Deployment & merge order
tag:-dimension PR. After this merges and releases, api-service bumps its@adobe/spacecat-shared-project-engine-clientdependency to pick up the new fixture.🤖 Generated with Claude Code