Skip to content

Added documented stubs for Visitor ID migration functional tests#1552

Draft
carterworks wants to merge 1 commit into
migrate-integration/00-infrafrom
migrate-integration/20-visitor
Draft

Added documented stubs for Visitor ID migration functional tests#1552
carterworks wants to merge 1 commit into
migrate-integration/00-infrafrom
migrate-integration/20-visitor

Conversation

@carterworks

@carterworks carterworks commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Changed Packages

  • core
  • reactor-extension

Description

All 4 visitor tests preserved as test.skip — require Visitor.js injection infrastructure and OptIn mock not available in the current harness.

Related Issue

Part of the functional test → integration test migration. See packages/browser/test/FUNCTIONAL_MIGRATION_PLAN.md.

Motivation and Context

The existing TestCafe functional test suite is being migrated to Vitest+Playwright+MSW to enable faster, more reliable CI testing without a running server. This PR is part of a stacked series — each PR migrates one test file.

Functional tests replaced:

  • packages/browser/test/functional/specs/Visitor/C35448.js
  • packages/browser/test/functional/specs/Visitor/C35450.js
  • packages/browser/test/functional/specs/Visitor/C36908.js
  • packages/browser/test/functional/specs/Visitor/C36909.js

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have added a Changeset file with a consumer-facing description of my changes.
  • I have signed the Adobe Open Source CLA or I'm an Adobe employee.
  • I have made any necessary test changes and all tests pass.
  • I have run the Sandbox successfully.

Stack

  1. Added MSW handlers, mock fixtures, and Vitest+Playwright test harness for functional test migration #1532
  2. Migrated Install SDK functional tests to Vitest+Playwright+MSW #1533

@carterworks carterworks added the ignore-for-release Do not include this PR in release notes label Jun 12, 2026
@carterworks carterworks self-assigned this Jun 12, 2026
@changeset-bot

changeset-bot Bot commented Jun 12, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: bece161

The changes in this PR will be included in the next version bump.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@greptile-apps

greptile-apps Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds a new integration test spec file for Visitor ID migration (visitor.spec.js) as part of the ongoing migration from the TestCafe functional suite to Vitest+Playwright+MSW. All four tests (C35448, C35450, C36908, C36909) are registered as test.skip stubs because the Visitor.js page-injection infrastructure and OptIn mock needed to exercise them are not yet available in the integration harness.

  • Adds packages/browser/test/integration/specs/Visitor/visitor.spec.js with four skipped stubs preserving test case IDs and skip rationale from the original functional tests.
  • No production code is changed; the file is purely additive to the test suite and does not affect runtime behavior.

Confidence Score: 4/5

Safe to merge — the change is purely additive, touching only a new test file with no production code impact.

The file is a straightforward test stub with no runtime effects. The only concerns are that test.skip with empty bodies is slightly less idiomatic than test.todo for unwritten tests, and there is no tracking reference linking back to when the required Visitor.js infrastructure will land to un-skip these tests.

packages/browser/test/integration/specs/Visitor/visitor.spec.js — consider adding a ticket/issue link in the skip comment and evaluating whether test.todo better expresses intent.

Important Files Changed

Filename Overview
packages/browser/test/integration/specs/Visitor/visitor.spec.js New file registering four fully-skipped Visitor ID migration test stubs; tests require Visitor.js injection and OptIn mock not yet available in the Vitest+MSW harness.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Visitor ID Migration Tests<br/>C35448, C35450, C36908, C36909] -->|needs| B{Required Infrastructure}
    B -->|missing| C[Visitor.js Page Injection]
    B -->|missing| D[VisitorAPI MSW Handler]
    B -->|missing| E[OptIn Mock]
    C & D & E --> F[test.skip stubs registered no execution]
    F --> G[Integration Test Suite visitor.spec.js]
Loading

Reviews (1): Last reviewed commit: "test(integration): migrate visitor funct..." | Re-trigger Greptile

Comment on lines +16 to +47

describe("Visitor ID migration", () => {
test.skip(
"C35448 - ID migration enabled: Alloy waits for Visitor to get ECID and uses that value " +
"(skipped: requires Visitor.js injection infrastructure and VisitorAPI MSW handler)",
// eslint-disable-next-line no-empty-function
async () => {},
);

test.skip(
"C35450 - ID migration + consent pending: when consent is given to both, " +
"Alloy waits for Visitor ECID " +
"(skipped: requires Visitor.js injection infrastructure and OptIn mock)",
// eslint-disable-next-line no-empty-function
async () => {},
);

test.skip(
"C36908 - ID migration + consent pending: Visitor denied, Alloy approved — " +
"Alloy ECID matches Visitor ECID " +
"(skipped: requires Visitor.js injection infrastructure and OptIn mock)",
// eslint-disable-next-line no-empty-function
async () => {},
);

test.skip(
"C36909 - ID migration disabled + consent pending: Visitor denied, Alloy approved — " +
"Alloy goes ahead with its own ECID " +
"(skipped: requires Visitor.js injection infrastructure and OptIn mock)",
// eslint-disable-next-line no-empty-function
async () => {},
);

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.

P2 test.todo more idiomatic for unwritten stubs than test.skip with empty bodies

Vitest's test.todo is purpose-built for "planned but not yet written" tests — it takes only a title string and signals to Vitest (and readers) that no implementation exists yet. test.skip with async () => {} bodies implies an implementation that has been temporarily disabled, which is semantically misleading. Using test.todo would also remove the need for the four // eslint-disable-next-line no-empty-function suppressions.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

governing permissions and limitations under the License.
*/

// Skipped: requires Visitor.js page injection and VisitorAPI MSW — not available in this harness.

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.

P2 No tracking reference for unblocking these tests

The comment documents why the tests are skipped but gives no pointer to a ticket, issue, or follow-up PR that will provide Visitor.js injection infrastructure and the OptIn mock. Without a reference, these stubs risk becoming permanently skipped — especially notable given that C35448 was tagged SEVERITY: "P0" in the original functional suite. Consider linking a tracking issue so the infrastructure gap stays visible.

@carterworks carterworks force-pushed the migrate-integration/19-migration branch from e439ac3 to bd0c3d7 Compare June 12, 2026 17:32
@carterworks carterworks force-pushed the migrate-integration/20-visitor branch from 20efce8 to 2219b1e Compare June 12, 2026 17:32

@greptile-apps greptile-apps 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.

carterworks has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@greptile-apps greptile-apps 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.

carterworks has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@carterworks carterworks changed the base branch from migrate-integration/19-migration to migrate-integration/00-infra June 12, 2026 17:58

@greptile-apps greptile-apps 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.

carterworks has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.

@carterworks carterworks force-pushed the migrate-integration/20-visitor branch 4 times, most recently from b36c72a to 003c8db Compare June 12, 2026 21:20
@carterworks carterworks marked this pull request as draft June 15, 2026 16:38
@carterworks carterworks force-pushed the migrate-integration/20-visitor branch from 003c8db to e4a5ce2 Compare June 15, 2026 19:59
@carterworks carterworks force-pushed the migrate-integration/20-visitor branch from e4a5ce2 to bece161 Compare June 26, 2026 20:32
@carterworks

Copy link
Copy Markdown
Collaborator Author

Stub PR review — Visitor ID migration (visitor.spec.js)

All four cases are confirmed test.skip stubs. No functional files deleted. One finding; everything else is clean.


Stub traceability

C-number Old functional file Skip reason in stub
C35448 Visitor/C35448.js Visitor.js injection + VisitorAPI MSW handler
C35450 Visitor/C35450.js Visitor.js injection + OptIn mock
C36908 Visitor/C36908.js Visitor.js injection + OptIn mock
C36909 Visitor/C36909.js Visitor.js injection + OptIn mock

Checks

🟢 All stubs genuine — each test.skip body is async () => {}. No test logic, no assertions, no false "migrated" claim.

🟢 Skip rationale documented — file-level comment at visitor.spec.js:13 gives the shared reason; each stub name repeats the specific blocker inline.

🟢 C-number traceability — all four C-numbers from migrate-integration/00-infra are present and map 1:1.

🟢 Harness wiring — import ../../helpers/testsSetup/extend.js resolves correctly from specs/Visitor/. Registration is glob-based (packages/browser/test/integration/**/*.spec.js in vitest.projects.js:61); the new file is auto-included.

🟢 License header — present, year 2026 matches surrounding new files.

🟢 Functional files not deleted — correct for a stub PR.

🟡 Inherited bug in source material (informational, not a PR defect): C35450.js, C36908.js, and C36909.js in the old functional suite all carry test.meta({ ID: "C35448" }) — a copy-paste error in the originals. The stubs correctly use the right C-numbers in their names, so traceability is preserved. Worth tracking when these get fully implemented.


N/A for stub PRs: mock fidelity, timing, network handler coverage, parity with functional assertions — none applicable until implementation.


Looks good to merge as-is. The stub-then-implement pattern is being followed correctly here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ignore-for-release Do not include this PR in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant