test: remove remaining coverage ignores and tighten cross-bundler runtime coverage#512
Merged
Conversation
Review Summary by QodoRemove coverage ignores and add executable tests with native URL module
WalkthroughsDescription• Removed v8 coverage ignores and replaced with executable tests • Replaced custom fileURLToPath with native Node.js url module • Simplified ESM/CJS compatibility logic across bundler packages • Added comprehensive test coverage for previously ignored code paths • Refactored resolver handling in turbopack and webpack packages Diagramflowchart LR
A["v8 ignore comments"] -->|"Remove"| B["Executable test coverage"]
C["Custom fileURLToPath"] -->|"Replace with"| D["Native url module"]
E["Conditional __dirname logic"] -->|"Simplify to"| F["path.dirname + import.meta.url"]
G["Webpack/Turbopack resolvers"] -->|"Refactor"| H["Unified resolveWebpackEntry"]
B --> I["Test suite expansion"]
D --> I
F --> I
H --> I
File Changes1. packages/code-inspector-plugin/src/index.ts
|
Code Review by Qodo
1. Null WebpackEntry crash
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+47
to
52
| const WebpackEntry = resolveWebpackEntry({ | ||
| requireResolve: globalThis.require?.resolve, | ||
| importMetaResolve: import.meta.resolve, | ||
| }); | ||
| const WebpackDistDir = path.resolve(WebpackEntry, '..'); | ||
|
|
There was a problem hiding this comment.
1. Null webpackentry crash 🐞 Bug ≡ Correctness
TurbopackCodeInspectorPlugin passes a possibly-null WebpackEntry into path.resolve(), which can throw and prevent the plugin from initializing. resolveWebpackEntry is explicitly able to return null, and the test suite asserts that behavior.
Agent Prompt
### Issue description
`TurbopackCodeInspectorPlugin` calls `path.resolve(WebpackEntry, '..')` even when `resolveWebpackEntry(...)` can return `null`, causing a runtime throw and preventing the plugin from loading.
### Issue Context
`resolveWebpackEntry({})` is expected to return `null` (covered by tests), so the plugin must either handle `null` (e.g., return `{}` / disable plugin) or throw a clear, actionable error before calling `path.resolve`.
### Fix Focus Areas
- packages/turbopack/src/index.ts[15-52]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+15
to
+24
| export function resolveWebpackEntry(params: { | ||
| requireResolve?: (id: string) => string; | ||
| importMetaResolve?: (id: string) => string | Promise<string>; | ||
| }) { | ||
| if (typeof params.importMetaResolve === 'function') { | ||
| const resolved = params.importMetaResolve( | ||
| '@code-inspector/webpack', | ||
| ) as unknown as string; | ||
| return fileURLToPath(resolved); | ||
| } |
There was a problem hiding this comment.
2. Async resolver mishandled 🐞 Bug ≡ Correctness
resolveWebpackEntry casts the result of importMetaResolve to a string and calls fileURLToPath on it, but its own type allows Promise<string> and the function does not await or detect Promises. If a Promise is returned, fileURLToPath will receive a non-string and can throw at runtime.
Agent Prompt
### Issue description
`resolveWebpackEntry` accepts an `importMetaResolve` that may return a `Promise<string>`, but the implementation treats it as a synchronous string and calls `fileURLToPath` immediately.
### Issue Context
This can lead to runtime failures if a Promise is returned. Since plugin configuration is typically synchronous, you can either:
- Restrict the accepted type to synchronous `string` only (and update types accordingly), OR
- Detect Promise-like values and throw a clear error / fall back to `requireResolve`.
### Fix Focus Areas
- packages/turbopack/src/index.ts[15-29]
- packages/turbopack/types/index.d.ts[6-10]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+6
to
10
| export declare function resolveWebpackEntry(params: { | ||
| requireResolve?: (id: string) => string; | ||
| importMetaResolve?: (id: string) => string | Promise<string>; | ||
| }): string; | ||
| export declare function TurbopackCodeInspectorPlugin(options: Options): Record<string, any>; |
There was a problem hiding this comment.
3. Wrong resolver return typing 🐞 Bug ≡ Correctness
packages/turbopack/types/index.d.ts declares resolveWebpackEntry returns string, but the implementation (and tests) return null when no resolver is provided. This makes TS consumers believe a string is always returned and can hide null-handling bugs.
Agent Prompt
### Issue description
The `resolveWebpackEntry` declaration file claims it returns `string`, but the implementation returns `string | null`.
### Issue Context
You should either:
- Update the declaration to `string | null`, and ensure call sites handle `null`, OR
- Change implementation to never return `null` (throw with message) so the `string` type is correct.
### Fix Focus Areas
- packages/turbopack/types/index.d.ts[6-10]
- packages/turbopack/src/index.ts[15-52]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #512 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 23 23
Lines 2751 2821 +70
Branches 692 695 +3
=========================================
+ Hits 2751 2821 +70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR removes several remaining /* v8 ignore */ branches by replacing them with executable
coverage and small runtime-safe refactors where needed.
What changed
vue.ts
conditional fallbacks, and undefined columns
globalThis.require?.resolve
default record initialization paths
Validation
Executed targeted tests covering the updated areas:
pnpm vitest run test/core/server/transform/transform-vue.test.ts
pnpm vitest run test/core/server/transform/transform-vue-pug.test.ts
pnpm vitest run test/core/server/use-client/get-code-with-web-component.test.ts test/core/
server/use-client/nextjs-integration.test.ts test/core/server/use-client/get-injected-
code.test.ts
pnpm vitest run test/core/server/server/project-root.test.ts
pnpm vitest run test/code-inspector-plugin/index.test.ts
pnpm vitest run test/turbopack/index.test.ts
pnpm --filter @code-inspector/turbopack build
Notes
logic more explicit and testable.
assertions.