fix(build): stop deno task test failing on the orphaned #dnt import - #3505
Conversation
`deno task test` has been unable to start since #3212. It discovers `scripts/build/*.test.ts`, which reach `build-npm-extension-packages.ts`, which imports `#dnt`. That specifier lives in `scripts/test.deno.json`, not the root import map, so the run dies at module resolution: error: Import "#dnt" not a dependency and not in import map from scripts/build/build-npm-extension-packages.ts Scripts tests were never meant to run under the root config -- they have their own `test:scripts` task that passes `--config=scripts/test.deno.json`. The root task simply never excluded them. So exclude `scripts` from the root run, and chain `test:scripts` into `verify` so the coverage moves rather than disappears. Excluding without that second half would trade a loud failure for a silent gap. This is what forces `deno task release` to be run with `--no-test` `--no-build`: the release script's own gates call `deno task test` and `deno task verify:dist`, and both died here. Closes veryfront-issue-inbox#425. `deno task test` now runs to completion: 4217 passed, 3 failed. Those 3 are pre-existing and unrelated -- they were simply never reachable before, because the task could not get past module resolution. Verified by running them on a clean tree, e.g. `extensions/ext-node-websocket-ws/src/package-boundary.test.ts` fails identically without this change.
📝 WalkthroughWalkthroughThe Deno ChangesDeno test discovery
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deno.json (1)
467-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused regression test for the task split.
This change alters test discovery. Add or update a test that parses
deno.jsonand verifies thattestexcludesscripts. Also verify thatverifyrunstest:scripts.As per coding guidelines, for behavior changes, add or update a focused failing test before changing the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deno.json` at line 467, Add a focused regression test that parses the Deno task configuration and asserts the test task excludes scripts while the verify task invokes test:scripts. Ensure the test fails against the current configuration before updating the task definitions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@deno.json`:
- Line 467: Add a focused regression test that parses the Deno task
configuration and asserts the test task excludes scripts while the verify task
invokes test:scripts. Ensure the test fails against the current configuration
before updating the task definitions.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87c6eddd55
ℹ️ 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".
| "build:npm": "deno run -A scripts/build/generate-integrations-module.ts && deno task generate && deno run --config=scripts/test.deno.json --frozen -A scripts/build/build-npm-dnt.ts", | ||
| "release": "deno run -A scripts/release.ts", | ||
| "test": "deno task generate && DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 SSR_TRANSFORM_PER_PROJECT_LIMIT=0 REVALIDATION_PER_PROJECT_LIMIT=0 NODE_ENV=production LOG_FORMAT=text deno test --preload=src/schemas/_test-setup.ts --no-check --parallel --allow-all '--ignore=tests/e2e,tests/integration/compiled-binary-e2e.test.ts' --unstable-worker-options --unstable-net", | ||
| "test": "deno task generate && DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 SSR_TRANSFORM_PER_PROJECT_LIMIT=0 REVALIDATION_PER_PROJECT_LIMIT=0 NODE_ENV=production LOG_FORMAT=text deno test --preload=src/schemas/_test-setup.ts --no-check --parallel --allow-all '--ignore=tests/e2e,tests/integration/compiled-binary-e2e.test.ts,scripts' --unstable-worker-options --unstable-net", |
There was a problem hiding this comment.
When deno task test runs, this directory-wide exclusion removes every test under scripts/, but the compensating test:scripts task has a fixed file list that omits existing tests such as scripts/build/browser-safe-exports.test.ts, scripts/build/generate-integrations-module.test.ts, and scripts/lint/extension-manifest-reader.test.ts. These tests were previously part of automatic discovery, so neither test nor the updated verify task now exercises them, allowing browser-export, generated-module, and extension-manifest regressions through the advertised replacement coverage. Add the omitted tests or make test:scripts discover the compatible script tests.
Useful? React with 👍 / 👎.
1 participant
Closes veryfront-issue-inbox#425.
Problem
deno task testhas been unable to start since #3212. It discoversscripts/build/*.test.ts, which reachbuild-npm-extension-packages.ts, which imports#dnt. That specifier lives inscripts/test.deno.json, not the root import map, so the run dies at module resolution before a single test executes:Scripts tests were never meant to run under the root config — they have their own
test:scriptstask that passes--config=scripts/test.deno.json. The root task simply never excluded them.Why it matters
This is what forces
deno task releaseto be invoked with--no-test --no-build: the release script's own gates calldeno task testanddeno task verify:dist, and both died here. Every recent release has been cut with its gates disabled.Fix
Two task edits, no runtime code:
testexcludesscripts.verifychainsdeno task test:scripts, so that coverage moves rather than disappears. Excluding without this second half would trade a loud failure for a silent gap, which is the worse outcome.Result
deno task testnow runs to completion:The 3 failures are pre-existing and unrelated — they were simply never reachable before, because the task could not get past module resolution. Verified by running them on a clean tree:
extensions/ext-node-websocket-ws/src/package-boundary.test.tsfails identically without this change. They look environmental (S3BlobStoragetransport, extension package boundary) and are left for separate triage rather than bundled here.Note the pre-push gate runs a different task, so this does not change what that gate enforces.
Summary by CodeRabbit