Skip to content

test(snapshots): support local-registry cases in the PTY runner#2080

Open
fengmk2 wants to merge 3 commits into
snap-linux-parallelfrom
snap-local-registry
Open

test(snapshots): support local-registry cases in the PTY runner#2080
fengmk2 wants to merge 3 commits into
snap-linux-parallelfrom
snap-local-registry

Conversation

@fengmk2

@fengmk2 fengmk2 commented Jul 7, 2026

Copy link
Copy Markdown
Member

Implement local-registry = true in crates/vite_cli_snapshots (previously
a "not implemented yet" stub). The runner now packs the checkout's
vite-plus and @voidzero-dev/vite-plus-core once per run, then for each
local-registry case starts local-npm-registry.ts --serve (cwd = the staged
fixture, so its mock-manifest.json/tarballs are served), reads the JSON
handshake, and folds the per-package-manager registry env into every step.
The registry is torn down when the case ends. Steps default to a 120s
timeout for these cases (installs are slower), still overridable per step.

Adds a --pack-to <dir> mode to local-npm-registry.ts so the pack logic
stays in one place, and a local_registry smoke fixture that installs a
package served only by the fixture's mock-manifest to prove the path.

This unblocks the local-registry create cases parked as ignore = true
in #2071, which rebases on top of this.

@netlify

netlify Bot commented Jul 7, 2026

Copy link
Copy Markdown

Deploy Preview for viteplus-preview canceled.

Name Link
🔨 Latest commit eae9fc4
🔍 Latest deploy log https://app.netlify.com/projects/viteplus-preview/deploys/6a4c9a85c8aa9d00081c4b2a

fengmk2 commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label auto-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@fengmk2

fengmk2 commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bd1e932b2a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +703 to +704
.env_clear()
.envs(case_env)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve the user's npmrc for registry upstreams

In environments where snapshot installs rely on a user-level .npmrc for an internal npm mirror, starting the helper with only case_env gives local-npm-registry.ts the isolated case HOME from baseline_env. Its resolveUpstreamRegistry() reads homedir()/.npmrc, so these local-registry cases now fall back to https://registry.npmjs.org instead of the configured upstream, and create/install snapshots with real dependencies fail behind such mirrors. Please keep the step HOME isolated but pass the runner's upstream registry/Home through to the registry process.

Useful? React with 👍 / 👎.

Comment on lines +660 to +662
let _ = std::process::Command::new("taskkill")
.args(["/PID", &self.child.id().to_string(), "/T", "/F"])
.output();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Badge Clean registry cache dirs on Windows teardown

On Windows local-registry cases, this forcibly terminates the Node registry, so the SIGINT/SIGTERM shutdown handler in local-npm-registry.ts never runs. That handler is the only normal path that removes the per-case YARN_GLOBAL_FOLDER and BUN_INSTALL_CACHE_DIR, so every local-registry case leaks temp cache directories on Windows, and yarn/bun install cases can leave sizable caches in CI. Please either delete those env paths from the Rust handle or use a teardown path the script can clean up.

Useful? React with 👍 / 👎.

Comment on lines +1221 to +1222
let local_registry_pack = if case.local_registry {
match runtimes.local_registry_pack() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep global-only snapshots independent of JS builds

When developers use just snapshot-test-global/VP_SNAP_SKIP_FLAVORS=local on a Rust-only checkout, global local-registry trials still enter this branch and call packLocalVitePlusPackages, which requires packages/cli/dist and a built NAPI binding. The new vp = "global" smoke case therefore makes the documented global-only workflow fail even though the local flavor is skipped; either skip/gate local-registry cases when the local package build is unavailable or avoid packing for cases that do not install the checkout packages.

Useful? React with 👍 / 👎.

Comment on lines +741 to +742
let parsed: serde_json::Value = serde_json::from_str(line.trim())
.map_err(|e| format!("could not parse local registry handshake `{}`: {e}", line.trim()))?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Badge Tear down the registry on handshake parse errors

If the helper emits a non-JSON first stdout line, for example a warning or accidental log before the handshake, this ? returns before the child is wrapped in RegistryHandle, unlike the no-handshake branch above. In that scenario the registry can keep running after the failed trial and leave its temp caches/listening process around for later tests; explicitly kill/drop the child on parse and validation errors as well.

Useful? React with 👍 / 👎.

@fengmk2 fengmk2 changed the base branch from main to graphite-base/2080 July 7, 2026 12:14
@fengmk2 fengmk2 force-pushed the snap-local-registry branch from eae9fc4 to 005954b Compare July 7, 2026 12:14
@fengmk2 fengmk2 changed the base branch from graphite-base/2080 to snap-linux-parallel July 7, 2026 12:14
@fengmk2 fengmk2 force-pushed the snap-local-registry branch 2 times, most recently from 9920f40 to cd2fe6d Compare July 7, 2026 13:14
@fengmk2 fengmk2 force-pushed the snap-linux-parallel branch from e046ea4 to 4a77a5b Compare July 7, 2026 13:14
@fengmk2 fengmk2 marked this pull request as ready for review July 7, 2026 13:16
@fengmk2 fengmk2 requested a review from wan9chi July 7, 2026 13:16
@fengmk2 fengmk2 self-assigned this Jul 7, 2026
fengmk2 added 3 commits July 7, 2026 21:31
Implement `local-registry = true` in crates/vite_cli_snapshots (previously
a "not implemented yet" stub). The runner now packs the checkout's
vite-plus and @voidzero-dev/vite-plus-core once per run, then for each
local-registry case starts local-npm-registry.ts --serve (cwd = the staged
fixture, so its mock-manifest.json/tarballs are served), reads the JSON
handshake, and folds the per-package-manager registry env into every step.
The registry is torn down when the case ends. Steps default to a 120s
timeout for these cases (installs are slower), still overridable per step.

Adds a `--pack-to <dir>` mode to local-npm-registry.ts so the pack logic
stays in one place, and a `local_registry` smoke fixture that installs a
package served only by the fixture's mock-manifest to prove the path.

This unblocks the local-registry create cases parked as `ignore = true`
in #2071, which rebases on top of this.
…he suite

The per-case registry handle only sent SIGTERM to the server's process
group and then blocked on wait(). On the Linux CI runner that group signal
did not reach the node server, so wait() hung until the 60-minute job
timeout. Signal the process directly as well as its group (group-leader
setup varies by platform), and cap the wait: after a 3s grace period,
force-kill the child via its handle and reap. Drop now always returns.
libtest-mimic (0.8) has no --report-time, so the runner records each
case's duration and prints a slowest-first summary once the run finishes.
Every case's cost is visible (e.g. the install-heavy approve-builds cases)
and a slow or stuck case stands out at a glance.

fengmk2 commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@codex review

@fengmk2 fengmk2 force-pushed the snap-local-registry branch from cd2fe6d to a8ac013 Compare July 7, 2026 13:43
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