Conversation
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review posted successfully to PR #14463. Summary of what I found and flagged:
|
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
c3ed386 to
e735d81
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if ( | ||
| typeof id !== "string" || | ||
| !id.startsWith("/") || | ||
| id.includes("\0") | ||
| ) { | ||
| return originalRunInlinedModule(context, code, module); | ||
| } |
There was a problem hiding this comment.
🚩 Windows absolute paths may not be recognized by the evaluator patch
The check at packages/vitest-pool-workers/src/worker/index.ts:289 uses id.startsWith("/") to detect absolute paths that need conversion to file:// URLs. On Windows, absolute paths start with a drive letter (e.g. C:/Users/... or C:\Users\...), so this check would be false and the conversion would be skipped. This means V8 coverage would silently report 0% for user source files on Windows.
Using path.isAbsolute(id) (from node:path) would handle both Unix and Windows paths. However, since this code runs inside workerd and the AGENTS.md notes tests are "Skipped on Windows CI due to flakiness", this may be an accepted limitation. Worth confirming whether Windows support for V8 coverage is intended.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This comment seems legitimate to me 🤔
| ensureFeature( | ||
| runnerWorker.compatibilityFlags, | ||
| "nodejs_inspector_local_dev" | ||
| ); |
There was a problem hiding this comment.
🚩 enable_nodejs_inspector_local_dev flag is not referenced anywhere else in the repo
The nodejs_inspector_local_dev feature name (producing enable_nodejs_inspector_local_dev via ensureFeature at packages/vitest-pool-workers/src/pool/index.ts:415) has no other occurrences in the repository — unlike nodejs_inspector_module which appears in the unenv-preset, wrangler e2e tests, etc. This flag appears to be a new workerd-side flag. If the flag name is incorrect or not yet landed in workerd, the pool would silently set an unrecognized compatibility flag. Worth confirming the flag name matches what workerd expects.
Was this helpful? React with 👍 or 👎 to provide feedback.
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
There was a problem hiding this comment.
If not super difficult I think that testing the coverage in an e2e or fixture test would be pretty ideal here 🤔
|
Closing in favour of #15591 |
3 participants
Fixes #[insert GH or internal issue link(s)].
Describe your change...
A picture of a cute animal (not mandatory, but encouraged)