initial e2e tests #113
initial e2e tests #113
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces an E2E app and Vitest-based test flow, extracts Fastify app creation into a createApp() factory, updates CI to run unit and e2e jobs in parallel, upgrades Node to v24, and adds pnpm catalog entries and workspace/typegen tooling for OpenAPI types. Changes
Sequence Diagram(s)mermaid Tester->>E2E_Script: run E2E tests (vitest) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 35-56: The e2e-tests job is missing the required .env and isn't
waiting for Docker healthchecks; add a step before "Start Docker services" to
copy .env.example to .env (or create .env from required secrets) so services can
read POSTGRES_USER/S3_* etc., replace the "docker compose up -d" command in the
"Start Docker services" step with "docker compose up -d --wait" to block until
healthchecks pass, and remove any unnecessary full build step (e.g., build:all)
from this job while ensuring the job runs the minimal pre-step "typegen" (pnpm
run typegen) if the e2e target depends on generated API types.
In `@apps/e2e/scripts/typegen.ts`:
- Around line 47-51: The current CLI runner swallows errors in the catch
handler; update the invocation that runs generateTypes() (the block checking
process.argv[1] === __filename) to capture the caught error (e.g. .catch((err)
=> { ... })) and log it before exiting so failures from generateTypes, execSync,
fetch, or spawnApp aren’t lost; use a clear output like
console.error("generateTypes failed:", err) (or the project logger) then call
process.exit(1).
In `@apps/e2e/tsconfig.json`:
- Around line 4-12: In apps/e2e/tsconfig.json update the "module" compiler
option from "commonjs" to "NodeNext" (or "ESNext") so the ESM imports/exports in
the e2e sources and the package.json "type": "module" align; ensure
"verbatimModuleSyntax": true remains and run tsc --noEmit to verify IDE
diagnostics no longer treat ESM imports as errors.
In `@package.json`:
- Around line 56-57: Update the Vitest dependency version in package.json to be
compatible with Vite 7; replace the current "vitest": "^3.0.0" entry with
"vitest": "^3.2.0" (or "^4.0.0" if you prefer upgrading to v4) so that Vitest
supports Vite 7, and ensure your CI/dev environment meets Vite 7's Node.js
requirement (Node 20.19+ or 22.12+).
🧹 Nitpick comments (4)
nx.json (1)
17-24: Redundant glob pattern on Line 19.
!{projectRoot}/**/*.spec.ts(Line 19) is a subset of the broader glob!{projectRoot}/**/?(*.)+(spec|test).[jt]s?(x)?(.snap)(Line 21), which already excludes all.spec.tsfiles. Consider removing Line 19 to reduce confusion.♻️ Proposed cleanup
"production": [ "default", - "!{projectRoot}/**/*.spec.ts", "!{projectRoot}/eslint.config.js", "!{projectRoot}/**/?(*.)+(spec|test).[jt]s?(x)?(.snap)", "!{projectRoot}/tsconfig.spec.json", "!{projectRoot}/src/test-setup.[jt]s" ]apps/e2e/src/lib/spawn-app.ts (1)
19-21: Misleading fallback whenaddressis a string.When
server.address()returns a string (Unix domain socket), assigning it toportand later callingNumber(port)on Line 26 yieldsNaN. This won't happen in practice since you bind to127.0.0.1, but the logic is misleading. Consider narrowing the type guard:♻️ Suggested improvement
const address = app.server.address(); - const port = typeof address === "string" ? address : address?.port; - if (!port) throw new Error("Failed to get server port"); + if (!address || typeof address === "string") { + throw new Error("Failed to get server address"); + } + const port = address.port;apps/e2e/src/smoke.spec.ts (1)
4-16: Consider sharing the app instance across health-check tests.Each
it.eachiteration spawns and tears down a fresh app (including any DB connections). For read-only health checks, a single shared instance viabeforeAll/afterAllwould reduce startup overhead noticeably.That said, the current approach maximizes isolation, which is a valid trade-off for a small test suite.
apps/e2e/project.json (1)
25-34: Typegen inputs could be narrowed for better cache hit rates.
{workspaceRoot}/apps/api/**/*includes test files, docs, and config that don't affect the OpenAPI spec. Narrowing to source files (e.g.,{workspaceRoot}/apps/api/src/**/*) would avoid unnecessary typegen re-runs.♻️ Suggested narrowing
- "inputs": ["{workspaceRoot}/apps/api/**/*"] + "inputs": ["{workspaceRoot}/apps/api/src/**/*"]
package.json
Outdated
| "vite": "^7.0.0", | ||
| "vitest": "^3.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
vitest 3 compatibility with vite 7
💡 Result:
Vitest v3 is compatible with Vite 7 starting from Vitest 3.2 (so Vitest 3.0 / 3.1 are not the supported combo). Upgrade to vitest@^3.2.0 (or newer, e.g. Vitest 4.x) when moving to Vite 7. [1]
Also note Vite 7 requires Node.js 20.19+ or 22.12+, which your test environment must meet as well. [1]
Sources:
[1] Vite blog: “Vite 7.0 is out!” (Vitest support from 3.2; Node requirements) (main.vitejs.dev)
Citations:
Upgrade Vitest to 3.2.0 or higher for Vite 7 compatibility.
vitest@^3.0.0 is incompatible with vite@^7.0.0. Vitest 3.x support for Vite 7 begins at version 3.2.0. Change to vitest@^3.2.0 or upgrade to vitest@^4.0.0. Additionally, Vite 7 requires Node.js 20.19+ or 22.12+.
🤖 Prompt for AI Agents
In `@package.json` around lines 56 - 57, Update the Vitest dependency version in
package.json to be compatible with Vite 7; replace the current "vitest":
"^3.0.0" entry with "vitest": "^3.2.0" (or "^4.0.0" if you prefer upgrading to
v4) so that Vitest supports Vite 7, and ensure your CI/dev environment meets
Vite 7's Node.js requirement (Node 20.19+ or 22.12+).
There was a problem hiding this comment.
Thank you rabbit, this is helpful
There was a problem hiding this comment.
@20jasper You're welcome! Happy to help. Let me know if you need anything else! 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/e2e/scripts/typegen.ts`:
- Line 16: Replace the unsupported `await using app = await spawnApp();` pattern
with explicit async resource management: call `await spawnApp()` to get `app`,
wrap the test logic in a `try` block and perform cleanup in a `finally` block by
invoking the resource's async disposer (e.g. `await app[Symbol.asyncDispose]()`)
or a fallback like `await app.dispose()` if present; update references to `app`
accordingly and ensure `spawnApp` result is awaited before entering the try so
the resource is always cleaned up even on error.
| export default async function generateTypes() { | ||
| await fs.mkdir(outputDir, { recursive: true }); | ||
|
|
||
| await using app = await spawnApp(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js 22 explicit resource management await using support version
💡 Result:
Node.js 22.x does not support the Explicit Resource Management syntax (using / await using) because Node 22 ships with V8 12.4. [1]
Support arrives with Node.js 24.0.0 (released May 6, 2025), which upgrades to V8 13.6 and lists “Explicit resource management” among the newly supported JavaScript features—this is the first Node line where await using is available natively. [2][3]
Sources: [1] [2] [3]
🌐 Web query:
Node.js Symbol.asyncDispose support which version
💡 Result:
Symbol.asyncDispose landed in Node.js v20.4.0 (initial Symbol.dispose / Symbol.asyncDispose support was added there). [1] Core APIs then started exposing [Symbol.asyncDispose]() methods (for example https.Server[Symbol.asyncDispose]() is marked “Added in: v20.4.0”). [2]
So: Node.js v20.4.0+.
Sources: [1] [2]
🌐 Web query:
Node.js --experimental-strip-types flag purpose and capabilities
💡 Result:
--experimental-strip-types enables Node.js to execute .ts files by “erasing” (stripping) TypeScript type syntax at runtime, turning the file into valid JavaScript without doing full TypeScript compilation. It was added in Node v22.6.0. [1][2]
What it does
- Strips inline type annotations (and related erasable-only TS syntax) by replacing them with whitespace. [1]
- Performs no type-checking (you still need
tsc/a typechecker for that). [1] - Is designed to be lightweight and avoid needing source maps, because it doesn’t do codegen-heavy transforms. [1]
What it does not do / key limits
- No TS-to-JS “transform” features that require runtime code generation (commonly called “non-erasable” syntax), e.g.
enumandnamespace/modulewith runtime code; those require--experimental-transform-types. [1][3] - Ignores
tsconfig.json-dependent behaviors like path mapping (paths) or downleveling newer JS syntax. [1] - Early docs/release notes also call out operational constraints like explicit file extensions in imports/requires, requiring
type-only imports to be marked withtype, and being disabled for TypeScript insidenode_modulesby default (at least at introduction). [2]
Related flags / current behavior
--experimental-transform-typesextends support to TypeScript syntax that needs transformation/codegen (e.g., enums/namespaces). [1]- Some Node releases expose
--no-experimental-strip-typesto disable running TypeScript via stripping. [4]
Sources: Node.js TypeScript docs and release notes; TypeScript team notes on “erasable” syntax expectations. [1][2][3][4]
🏁 Script executed:
#!/bin/bash
cd apps/e2e/scripts
# Find and read the spawnApp function implementation
rg -A 20 "function spawnApp|const spawnApp|export.*spawnApp" --type tsRepository: playfulprogramming/hoof
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if the file imports or defines spawnApp
cat -n typegen.ts | head -20Repository: playfulprogramming/hoof
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Find the typegen.ts file
fd -t f "typegen.ts"Repository: playfulprogramming/hoof
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for spawnApp definition anywhere in the codebase
rg -l "spawnApp" --type ts --type jsRepository: playfulprogramming/hoof
Length of output: 151
🏁 Script executed:
#!/bin/bash
cat -n apps/e2e/scripts/typegen.ts | head -30Repository: playfulprogramming/hoof
Length of output: 1244
🏁 Script executed:
#!/bin/bash
cat -n apps/e2e/src/lib/spawn-app.tsRepository: playfulprogramming/hoof
Length of output: 1489
await using syntax is not supported by the Node.js runtime (22.x ships with V8 12.4), causing a pipeline SyntaxError.
The await using declaration is a TC39 Explicit Resource Management proposal that only landed in Node.js 24.0.0 (V8 13.6). Since the engines field requires Node >=22.6.0, this code will fail with SyntaxError: Unexpected identifier 'app'. The --experimental-strip-types flag only removes TypeScript type annotations—it does not enable unsupported JavaScript syntax proposals.
Refactor to use a try/finally block with explicit cleanup via Symbol.asyncDispose:
🐛 Proposed fix
- await using app = await spawnApp();
- const swaggerUrl = `${app.baseUrl}/openapi.json`;
+ const app = await spawnApp();
+ try {
+ const swaggerUrl = `${app.baseUrl}/openapi.json`;
- const response = await fetch(swaggerUrl);
- if (!response.ok) {
- throw new Error(
- `Failed to fetch OpenAPI spec: ${response.status} ${response.statusText}`,
- );
- }
- const spec = await response.json();
+ const response = await fetch(swaggerUrl);
+ if (!response.ok) {
+ throw new Error(
+ `Failed to fetch OpenAPI spec: ${response.status} ${response.statusText}`,
+ );
+ }
+ const spec = await response.json();
- console.log("Generating types");
- const tempSpecFile = path.join(outputDir, ".api-spec.json");
- try {
- await fs.writeFile(tempSpecFile, JSON.stringify(spec, null, 0));
- execSync(
- `pnpm exec openapi-typescript "${tempSpecFile}" -o "${outputFile}"`,
- {
- stdio: "inherit",
- },
- );
+ console.log("Generating types");
+ const tempSpecFile = path.join(outputDir, ".api-spec.json");
+ try {
+ await fs.writeFile(tempSpecFile, JSON.stringify(spec, null, 0));
+ execSync(
+ `pnpm exec openapi-typescript "${tempSpecFile}" -o "${outputFile}"`,
+ {
+ stdio: "inherit",
+ },
+ );
+ } finally {
+ await fs.unlink(tempSpecFile);
+ }
} finally {
- await fs.unlink(tempSpecFile);
+ await app[Symbol.asyncDispose]();
}🧰 Tools
🪛 GitHub Actions: Test
[error] 16-16: SyntaxError: Unexpected identifier 'app'.
🤖 Prompt for AI Agents
In `@apps/e2e/scripts/typegen.ts` at line 16, Replace the unsupported `await using
app = await spawnApp();` pattern with explicit async resource management: call
`await spawnApp()` to get `app`, wrap the test logic in a `try` block and
perform cleanup in a `finally` block by invoking the resource's async disposer
(e.g. `await app[Symbol.asyncDispose]()`) or a fallback like `await
app.dispose()` if present; update references to `app` accordingly and ensure
`spawnApp` result is awaited before entering the try so the resource is always
cleaned up even on error.
package.json
Outdated
| "packageManager": "pnpm@10.6.1", | ||
| "devDependencies": { | ||
| "@eslint/js": "^9.37.0", | ||
| "@nx/js": "21.5.3", |
There was a problem hiding this comment.
Nx added a bunch of these deps and I'm not sure we need them all
There was a problem hiding this comment.
@coderabbitai can you confirm or deny if the dependencies nx added are needed? I'm not sure of the purpose of many of these
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking `@nx/js` usage ==="
rg --type=json -n "@nx/js"
echo -e "\n=== Checking `@nx/vite` usage ==="
rg --type=json -n "@nx/vite"
echo -e "\n=== Checking `@swc` usage ==="
rg --type=json -n "@swc"
echo -e "\n=== Checking jsonc-eslint-parser usage ==="
rg -n "jsonc-eslint-parser"
echo -e "\n=== Checking `@vitest/coverage-v8` usage ==="
rg -n "coverage-v8|provider.*v8"
echo -e "\n=== Checking jiti usage ==="
rg -n "jiti"
echo -e "\n=== Checking tslib usage ==="
rg -n "tslib"
echo -e "\n=== Checking vite config files ==="
fd -e ts -e js -e mjs "vite.config"
echo -e "\n=== Checking vitest usage ==="
fd -e ts -e js "vitest.config"
rg --type=json -n "vitest"
echo -e "\n=== Checking nx.json targetDefaults ==="
cat nx.json | jq '.targetDefaults'
Length of output: 15840
🌐 Web query:
Nx `@nx/js` peer dependencies required
💡 Result:
For @nx/js, the peer dependencies you may need to provide (depending on which features you use) are:
nx(keep@nx/jsandnxon matching versions)@swc/core(optional peer; used for SWC-based flows)@swc-node/register(optional peer; used for SWC-based TS execution)@babel/traverse(peer dependency used by some JS/TS tooling paths in Nx) (github.com)
nx itself documents @swc/core and @swc-node/register as optional peers (via peerDependenciesMeta.optional). (app.unpkg.com)
If you want to confirm for your exact installed version, run one of:
npm view `@nx/js` peerDependencies peerDependenciesMeta
# or
pnpm view `@nx/js` peerDependencies peerDependenciesMeta
# or
yarn info `@nx/js` peerDependencies peerDependenciesMetaCitations:
- 1: Peer dependency issue with @nx/rspackv21.2.3 nrwl/nx#31952
- 2: https://app.unpkg.com/nx%4021.2.3/files/package.json?utm_source=openai
🌐 Web query:
Nx `@nx/vite` peer dependencies vitest vite tslib
💡 Result:
For current Nx releases (v22.x), the dependency expectations are roughly:
-
vite: treated as a peer dependency of@nx/vite(your workspace/app/library is expected to provide it). Nx migrations show updates targeting Vite^6.0.0(and later Nx versions also work with newer Vite majors). [1]
Also, Nx’s dependency-checks/lint autofix commonly adds"vite": "^6.0.0"alongside@nx/viteto a project’speerDependencies, reflecting that expectation. [2] -
vitest: not a peer dependency of@nx/viteanymore—Vitest support was split into the separate@nx/vitestpackage. [3]
Nx migrations indicate Vitest upgrades (e.g. to^4.0.0) as part of the tooling updates, but that’s now under the Vitest-specific integration. [1] -
tslib: generally a normal dependency (not a peer dependency) in the Nx ecosystem (e.g., the mainnxpackage depends ontslib). [4]
Sources: Nx Vite migrations [1], Nx GitHub issue showing peerDependencies autofix adding vite [2], Nx docs on splitting Vitest into @nx/vitest [3], Aikido Intel showing nx depends on tslib [4].
🏁 Script executed:
#!/bin/bash
echo "=== Checking if SWC is configured in nx.json or tsconfig files ==="
rg -n "@swc|swcrc" nx.json tsconfig*.json 2>/dev/null || echo "No SWC config found in nx.json or root tsconfig"
echo -e "\n=== Checking package.json peer dependencies metadata ==="
pnpm view `@nx/js`@21.5.3 peerDependencies peerDependenciesMeta 2>/dev/null || echo "Could not fetch peer deps info"
echo -e "\n=== Checking package.json peer dependencies for `@nx/vite` ==="
pnpm view `@nx/vite`@21.5.3 peerDependencies peerDependenciesMeta 2>/dev/null || echo "Could not fetch peer deps info"
echo -e "\n=== Checking if tslib is used directly in the codebase ==="
rg -n "from ['\"]tslib['\"]|require\(['\"]tslib['\"]" --type=ts --type=js | head -20
echo -e "\n=== Checking nx.json for task runner settings ==="
cat nx.json | jq '.tasksRunnerOptions // "No tasksRunnerOptions found"'
Length of output: 556
@20jasper I've analyzed all the dependencies that Nx added. Here's a breakdown:
✅ Definitely needed:
@nx/js(21.5.3) - Used as the executor for building the e2e app (apps/e2e/project.json)@nx/vite(21.5.3) - Used as the executor for running e2e tests (apps/e2e/project.json)vite- Required peer dependency of@nx/viteand configured inapps/e2e/vite.config.tsvitest- Required peer dependency of@nx/viteand used throughout the test suite@vitest/coverage-v8- Explicitly configured inapps/e2e/vite.config.tsfor test coveragejsonc-eslint-parser- Explicitly imported inapps/e2e/eslint.config.mjsfor linting JSON files
❓ Potentially unnecessary:
-
@swc-node/registerand@swc/core- These are optional peer dependencies of the mainnxpackage used for faster TypeScript execution. However, I found no SWC configuration in yournx.jsonor tsconfig files. You could try removing these and see if everything still works. -
tslib- This is a TypeScript runtime helper library. While it appears extensively inpnpm-lock.yamlas a transitive dependency of Nx packages, it's not directly imported anywhere in your codebase. It's likely unnecessary as a direct devDependency since it will be pulled in transitively when needed. -
jiti- This appears as an optional peer dependency for vite, vitest, and eslint to improve performance with TypeScript config files. It's probably providing a performance benefit but might not be strictly required.
💡 Recommendation:
You could try removing @swc-node/register, @swc/core, and tslib first and run your tests to see if everything still works. If tests pass, those dependencies weren't needed. The jiti dependency likely provides performance improvements, so I'd keep it unless you're trying to minimize dependencies aggressively.
There was a problem hiding this comment.
Rabbit I believe this is not a correct assessment, the @nx/js seems to be only for the @nx/js:tsc executor, and as this is an e2e test suite with an empty index.ts, it's not doing anything for me. Are you sure about these things?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@coderabbitai is there anything you see that's suspicious with my nx changes? Does everything look reasonable? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1417 🏁 Script executed: Length of output: 1044 🏁 Script executed: Length of output: 21 🏁 Script executed: Length of output: 91 🏁 Script executed: Length of output: 1489 🏁 Script executed: Length of output: 1093 🏁 Script executed: Length of output: 32
✅ What looks good:
🤔 Potential optimizations:
🎯 Minor observation:Only the e2e app has a Would you like me to help optimize the typegen inputs pattern, or are there any specific concerns about the current setup? |
Thank you for the confirmation rabbit, I think this is fine. Spawning the server only takes around 100ms |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nx.json (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting to unblock the pipeline.
The CI pipeline reports a Prettier formatting error for this file. Run
prettier --write nx.jsonto resolve it.
🧹 Nitpick comments (2)
nx.json (1)
17-24: Redundant glob inproductionnamedInput.Line 19 (
!{projectRoot}/**/*.spec.ts) is fully covered by the broader glob on line 21 (!{projectRoot}/**/?(*.)+(spec|test).[jt]s?(x)?(.snap)). You can safely remove line 19.Proposed fix
"production": [ "default", - "!{projectRoot}/**/*.spec.ts", "!{projectRoot}/eslint.config.js", "!{projectRoot}/**/?(*.)+(spec|test).[jt]s?(x)?(.snap)", "!{projectRoot}/tsconfig.spec.json", "!{projectRoot}/src/test-setup.[jt]s" ]package.json (1)
30-31: Node ≥24 is a significant requirement bump.Requiring Node 24+ limits contributor compatibility. The PR description notes this was chosen for
AsyncDisposablesupport and--experimental-strip-types, but also mentions the option to transpile down to ES2022 to stay on Node 22. If broader contributor support matters, consider revisiting this trade-off.
2 participants
Creates a new app for e2e testing via nx. I'm new to it, so feel free to correct me! My design uses AsyncDisposables to handle spawning app instances, so the per test servers will be handled without a try finally block or after each, just a
await using app = await spawnApp()!The initial PR runs basic E2E smoke tests that don't rely on store isolation
I updated to node24 in order to use AsyncDispose, which as far as I can tell is not a breaking change. I can always transpile down to ES2022 if we'd like to stay on node22, but we should at least update the node types to match the engine version
E2E tests including pulling docker images take 38s on the last run
Summary by CodeRabbit
New Features
Tests
Chores
Documentation