Add a smoke test to generated fedify init apps - #990
Open
Palcimer wants to merge 4 commits into
Open
Conversation
Scaffolded projects had no quick way to confirm that their federation setup actually serves an actor. Verifying it meant starting the dev server by hand and looking an actor up separately. Added a smoke-test script that starts the dev server, reads the port, waits for the server to answer, and looks an actor up with `lookupObject()`. fedify-dev#898 Assisted-by: Claude Code:claude-sonnet-5
Every framework now writes the smoke-test script and exposes it as a `test` task, so a scaffolded project can be verified with one command. The task runs the script with the runtime matching the package manager, and Node.js projects gain `tsx` as a dev dependency to execute it. fedify-dev#898 Assisted-by: Claude Code:claude-sonnet-5
Added a test covering that `patchFiles()` writes the script to the initializer's `testFile` path with the dev command baked in, and filled in `testFile` in the existing fixtures now that it is required. fedify-dev#898 Assisted-by: Claude Code:claude-sonnet-5
fedify-dev#898 Assisted-by: Claude Code:claude-sonnet-5
✅ Deploy Preview for fedify-json-schema canceled.
|
📝 WalkthroughWalkthrough
ChangesGenerated smoke-test task
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SmokeTest as Generated smokeTest.ts
participant DevServer as Development server
participant ActivityPub as ActivityPub endpoint
participant Loader as Document loader
SmokeTest->>DevServer: Spawn development command
DevServer-->>SmokeTest: Emit listening URL
SmokeTest->>ActivityPub: Poll readiness endpoint
ActivityPub-->>SmokeTest: Return ActivityPub JSON
SmokeTest->>Loader: Resolve configured actor
Loader-->>SmokeTest: Return actor document
SmokeTest->>DevServer: Terminate process group
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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.
Inline comments:
In `@packages/init/src/action/patch.test.ts`:
- Around line 95-105: Add coverage for the Deno branch of patchFiles by creating
init data with packageManager set to "deno", reading the generated smokeTest.ts,
and asserting it contains the expected Deno command substitution. Keep the
existing npm smoke-test coverage unchanged.
In `@packages/init/src/action/templates.ts`:
- Around line 60-70: Remove the JSDoc block immediately above the internal
loadTest helper in templates.ts, leaving the loadTest implementation unchanged.
In `@packages/init/src/templates/defaults/smokeTest.ts.tpl`:
- Around line 69-80: Update the onData handler to accumulate decoded server
output across chunks before applying portPatterns, so port values split between
chunks are matched correctly. Preserve the existing port parsing, timeout
cleanup, resolve behavior, and return once a match is found.
- Around line 115-118: Update checkActor to accept or obtain the startup abort
signal used by waitForServer, and pass that signal in the lookupObject options
so actor resolution is bounded by the same 1000ms timeout. Preserve the existing
documentLoader configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7c0cfeac-c825-4dfd-bde1-4900b109e4d3
📒 Files selected for processing (19)
CHANGES.mdchanges.d/init/smoke-test.mdpackages/init/src/action/configs.test.tspackages/init/src/action/patch.test.tspackages/init/src/action/patch.tspackages/init/src/action/templates.tspackages/init/src/templates/defaults/smokeTest.ts.tplpackages/init/src/types.tspackages/init/src/webframeworks/astro.tspackages/init/src/webframeworks/bare-bones.tspackages/init/src/webframeworks/elysia.tspackages/init/src/webframeworks/express.tspackages/init/src/webframeworks/hono.tspackages/init/src/webframeworks/next.tspackages/init/src/webframeworks/nitro.tspackages/init/src/webframeworks/nuxt.tspackages/init/src/webframeworks/solidstart.tspackages/init/src/webframeworks/sveltekit.tspackages/init/src/webframeworks/utils.ts
Comment on lines
+95
to
+105
| test("patchFiles writes the smoke-test script", async () => { | ||
| await withTempDir(async (dir) => { | ||
| await patchFiles(createInitData(dir, false)); | ||
|
|
||
| const testScript = await readFile( | ||
| join(dir, "scripts", "smokeTest.ts"), | ||
| "utf8", | ||
| ); | ||
| assert.match(testScript, /\["npm","run","dev"\]/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect tests for Deno and Node.js or Bun smoke-test generation.
rg -n -C 4 \
'smokeTest|packageManager: "(deno|bun|npm|pnpm|yarn)"|test task|run.*dev' \
packages/init/src --glob '*.test.ts'Repository: fedify-dev/fedify
Length of output: 15893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant patch.test.ts slices:"
sed -n '1,150p' packages/init/src/action/patch.test.ts
echo
echo "Tests mentioning smokeTest/runs/patchFiles/packageManager deno:"
rg -n -C 3 --glob '*.test.ts' \
'smokeTest|patchFiles|packageManager:\s*"?deno"?"|packageManager:\s*"?bun"?"|packageManager:\s*"?npm"?"|run.*dev|test task|tasks:\s*' packages/init/src/action packages/init/src/action/patch.test.tsRepository: fedify-dev/fedify
Length of output: 25321
Add smoke-test command coverage for Deno.
patchFiles has one smoke-test case that checks ["npm","run","dev"], but no case uses packageManager: "deno" or checks the Deno command. Add a Deno case, or ensure another patch test covers smokeTest.ts command substitution for Deno.
🤖 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 `@packages/init/src/action/patch.test.ts` around lines 95 - 105, Add coverage
for the Deno branch of patchFiles by creating init data with packageManager set
to "deno", reading the generated smokeTest.ts, and asserting it contains the
expected Deno command substitution. Keep the existing npm smoke-test coverage
unchanged.
Comment on lines
+60
to
+70
| /** | ||
| * Loads the smoke-test script content for the initializer. | ||
| * | ||
| * Every framework shares the same *defaults/smokeTest.ts* template, so unlike | ||
| * {@link loadLogging} there is no per-framework template override. The | ||
| * template spawns the project's own dev server, so it needs the dev command | ||
| * for the chosen package manager baked in at generation time. | ||
| * | ||
| * @param param0 - {@link InitCommandData} containing `packageManager` | ||
| * @returns The complete smoke-test script content as a string | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the JSDoc from loadTest.
loadTest is an internal helper in the initializer action layer. Keep the implementation comment-free unless it becomes part of the package entry-point API.
Based on learnings, internal helpers that are not re-exported from a package entry point should not have JSDoc.
🤖 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 `@packages/init/src/action/templates.ts` around lines 60 - 70, Remove the JSDoc
block immediately above the internal loadTest helper in templates.ts, leaving
the loadTest implementation unchanged.
Source: Learnings
Comment on lines
+69
to
+80
| const onData = (chunk: Buffer) => { | ||
| const text = chunk.toString("utf8"); | ||
| for (const pattern of portPatterns) { | ||
| const match = text.match(pattern); | ||
| if (match && match[1]) { | ||
| const port = Number.parseInt(match[1], 10); | ||
| clearTimeout(timeout); | ||
| resolve(port); | ||
| return; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Buffer server output before matching the port.
data chunks do not preserve line boundaries. If a server writes http://localhost:5173 across two chunks, none of these patterns match and the smoke test fails after 15 seconds although the server is ready.
Proposed fix
+ let portOutput = "";
const onData = (chunk: Buffer) => {
- const text = chunk.toString("utf8");
+ portOutput += chunk.toString("utf8");
for (const pattern of portPatterns) {
- const match = text.match(pattern);
+ const match = portOutput.match(pattern);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const onData = (chunk: Buffer) => { | |
| const text = chunk.toString("utf8"); | |
| for (const pattern of portPatterns) { | |
| const match = text.match(pattern); | |
| if (match && match[1]) { | |
| const port = Number.parseInt(match[1], 10); | |
| clearTimeout(timeout); | |
| resolve(port); | |
| return; | |
| } | |
| } | |
| }; | |
| let portOutput = ""; | |
| const onData = (chunk: Buffer) => { | |
| portOutput += chunk.toString("utf8"); | |
| for (const pattern of portPatterns) { | |
| const match = portOutput.match(pattern); | |
| if (match && match[1]) { | |
| const port = Number.parseInt(match[1], 10); | |
| clearTimeout(timeout); | |
| resolve(port); | |
| return; | |
| } | |
| } | |
| }; |
🤖 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 `@packages/init/src/templates/defaults/smokeTest.ts.tpl` around lines 69 - 80,
Update the onData handler to accumulate decoded server output across chunks
before applying portPatterns, so port values split between chunks are matched
correctly. Preserve the existing port parsing, timeout cleanup, resolve
behavior, and return once a match is found.
Comment on lines
+115
to
+118
| async function checkActor(url: string): Promise<Actor> { | ||
| const object = await lookupObject(url, { | ||
| documentLoader: getDocumentLoader({ allowPrivateAddress: true }), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the local Fedify implementation for timeout or abort support.
rg -n -C 5 --glob '*.ts' \
'lookupObject|function getDocumentLoader|const getDocumentLoader' .Repository: fedify-dev/fedify
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== template excerpt =="
sed -n '80,140p' packages/init/src/templates/defaults/smokeTest.ts.tpl
echo
echo "== lookup API excerpt =="
sed -n '90,285p' packages/vocab/src/lookup.ts
echo
echo "== all lookupObject calls in smoke template =="
rg -n "lookupObject|signal|AbortController|setTimeout|checkActor|test\\(" packages/init/src/templates/defaults/smokeTest.ts.tplRepository: fedify-dev/fedify
Length of output: 8802
Apply the startup timeout to actor resolution.
waitForServer() uses AbortSignal.timeout(1000), but checkActor() calls lookupObject() without passing any signal. Actor retrieval can still hang after readiness succeeds; pass the same abortable request signal to lookupObject().
🤖 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 `@packages/init/src/templates/defaults/smokeTest.ts.tpl` around lines 115 -
118, Update checkActor to accept or obtain the startup abort signal used by
waitForServer, and pass that signal in the lookupObject options so actor
resolution is bounded by the same 1000ms timeout. Preserve the existing
documentLoader configuration.
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 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
fedify initcreates runnable apps, but a generated project has no standard way to check that it actually serves an actor object. Confirming it means starting the dev server by hand and looking an actor up separately, which is awkward for new users and gives contributors no quick way to validate scaffold changes.This adds a
testtask to every scaffolded project. The task runs a generated scripts/smokeTest.ts that starts the app, reads the port the server actually bound from its output, waits for it to answer, and resolves the local actor withlookupObject(). On success, it prints the resolved actor. On failure, it prints the server's stdout and stderr output and exits the process with a failure code.Assisted-by: Claude Code:claude-sonnet-5
Related issue
fedify initapps #898Changes
testFileproperty to theWebFrameworkInitializerinterface, holding the path the smoke-test script is written to.loadTest(), which reads that template and bakes in the dev command for the chosen package manager.tsxas a dev dependency to generated apps whose package manager runs on Node.js, so that they can launch the smoke test.testtask that starts the smoke test to the generated apps.patchFiles()to verify thatfedify initwrites the smoke-test script.Benefits
Checklist
Did you write some relevant docs about this change (if it's a new feature)?Did you write a regression test to reproduce the bug (if it's a bug fix)?mise teston your machine?