test(e2e): modernize plugin e2e harness and dist publish flow#197
test(e2e): modernize plugin e2e harness and dist publish flow#197
Conversation
📝 WalkthroughWalkthroughRefactors e2e test infrastructure to use isolated, dynamically-created test workspaces instead of fixed fixtures. Moves test suites to new locations, updates async command utilities to require explicit working directories, introduces new test project creation helpers, and adds environment configuration and release publishing setup for the nx-forge package. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/nx-forge-e2e/src/basic-setup.spec.ts (1)
56-167:⚠️ Potential issue | 🟠 MajorMove the Forge uninstall/delete work into a
finallyblock.Once this test registers the app, every later assertion/command is on the happy path. A failure anywhere after that leaves the app installed/registered in the shared Forge environment because
afterAllonly cleans up the local workspace. Please trackappIdand do best-effort uninstall +deleteApp(appId)in afinallyso cleanup still happens on partial failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nx-forge-e2e/src/basic-setup.spec.ts` around lines 56 - 167, Wrap the post-registration flow (everything after you obtain appId) in a try/finally: after you parse appId from registeredOutputManifestContent, enter try { /* existing deploy/install/assertions */ } finally { if (appId) { attempt a best-effort uninstall by calling runForgeCommandAsync with the same flags and cwd used for install (the previous uninstallResult call), and then call deleteApp(appId)(apiClient) to remove the registered app; swallow/log (don't throw) any errors from these cleanup steps so failures during test assertions don't leave the app registered. Move the existing uninstall + deleteApp logic into that finally block and ensure appId is defined in the outer scope so the finally can reference it. }
🧹 Nitpick comments (2)
.codex/environments/environment.toml (1)
5-6: Consider usingnpm cifor reproducible installs.For test/CI environments,
npm ciis generally preferred overnpm ibecause it:
- Provides reproducible builds by strictly following
package-lock.json- Installs faster by skipping certain validation steps
- Fails if
package.jsonand lock file are out of sync, catching potential issues early♻️ Proposed change
[setup] -script = "npm i" +script = "npm ci"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.codex/environments/environment.toml around lines 5 - 6, Update the setup script under the [setup] section to use a reproducible CI install command: replace the current script value "npm i" with "npm ci" so the environment uses the lockfile for deterministic, faster installs and fails when package.json and package-lock.json are out of sync; ensure the change is applied to the script key in the [setup] block.e2e/nx-forge-e2e/src/utils/async-commands.ts (1)
29-35: Remove the repo-root fallback ifcwdis meant to be mandatory.The type says
cwdis required, but these default option objects still let future call sites omit it and silently run againstprocess.cwd()again. If workspace isolation is the goal here, makeoptsrequired and force every caller to passcwd.Also applies to: 63-69, 90-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nx-forge-e2e/src/utils/async-commands.ts` around lines 29 - 35, The function runCommandAsync currently provides a default opts object which falls back to process.cwd(), contradicting the declared requirement that cwd is mandatory; remove the default opts value and make the opts parameter required (so callers must supply opts.cwd), update any similar helper functions in this file (the other functions around lines 63-69 and 90-96) to require opts.cwd as well, and fix all call sites to pass an explicit cwd; ensure type signatures reflect opts as required (no default) and remove any internal fallback to process.cwd().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.codex/environments/environment.toml:
- Line 1: Remove the misleading autogenerated comment at the top of
.codex/environments/environment.toml (delete the line "# THIS IS AUTOGENERATED.
DO NOT EDIT MANUALLY"), then either add this environment to .codex/config.toml
so it will be activated (reference the environment identifier used inside
environment.toml) or add a short comment in environment.toml explaining why it's
intentionally optional; finally, in the environment's setup script replace any
occurrence of "npm i" with "npm ci" to ensure reproducible installs.
In `@e2e/nx-forge-e2e/src/utils/test-project.ts`:
- Around line 30-33: The runCommand invocation that currently calls
`create-nx-workspace@latest` should be changed to pin the Nx release used for
test workspaces to 22.6.1; locate the `runCommand` call that executes `npx -y
create-nx-workspace@latest ${projectName} ...` in test-project.ts and replace
`@latest` with `@22.6.1` so the created workspace's `create-nx-workspace`/`nx`
versions match the repo's pinned Nx dependencies and avoid version skew in e2e
tests.
---
Outside diff comments:
In `@e2e/nx-forge-e2e/src/basic-setup.spec.ts`:
- Around line 56-167: Wrap the post-registration flow (everything after you
obtain appId) in a try/finally: after you parse appId from
registeredOutputManifestContent, enter try { /* existing
deploy/install/assertions */ } finally { if (appId) { attempt a best-effort
uninstall by calling runForgeCommandAsync with the same flags and cwd used for
install (the previous uninstallResult call), and then call
deleteApp(appId)(apiClient) to remove the registered app; swallow/log (don't
throw) any errors from these cleanup steps so failures during test assertions
don't leave the app registered. Move the existing uninstall + deleteApp logic
into that finally block and ensure appId is defined in the outer scope so the
finally can reference it. }
---
Nitpick comments:
In @.codex/environments/environment.toml:
- Around line 5-6: Update the setup script under the [setup] section to use a
reproducible CI install command: replace the current script value "npm i" with
"npm ci" so the environment uses the lockfile for deterministic, faster installs
and fails when package.json and package-lock.json are out of sync; ensure the
change is applied to the script key in the [setup] block.
In `@e2e/nx-forge-e2e/src/utils/async-commands.ts`:
- Around line 29-35: The function runCommandAsync currently provides a default
opts object which falls back to process.cwd(), contradicting the declared
requirement that cwd is mandatory; remove the default opts value and make the
opts parameter required (so callers must supply opts.cwd), update any similar
helper functions in this file (the other functions around lines 63-69 and 90-96)
to require opts.cwd as well, and fix all call sites to pass an explicit cwd;
ensure type signatures reflect opts as required (no default) and remove any
internal fallback to process.cwd().
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d124c9e5-3abb-4260-804e-69e239ec8c8f
📒 Files selected for processing (12)
.codex/environments/environment.tomle2e/nx-forge-e2e/project.jsone2e/nx-forge-e2e/src/application.generator.spec.tse2e/nx-forge-e2e/src/basic-setup.spec.tse2e/nx-forge-e2e/src/utils/async-commands.tse2e/nx-forge-e2e/src/utils/atlassian-graphql-client.tse2e/nx-forge-e2e/src/utils/config.tse2e/nx-forge-e2e/src/utils/generate-forge-app.tse2e/nx-forge-e2e/src/utils/test-project.tse2e/nx-forge-e2e/tests/application.generator.spec.tse2e/nx-forge-e2e/tests/utils/e2e-workspace.tspackages/nx-forge/project.json
💤 Files with no reviewable changes (2)
- e2e/nx-forge-e2e/tests/utils/e2e-workspace.ts
- e2e/nx-forge-e2e/tests/application.generator.spec.ts
| @@ -0,0 +1,6 @@ | |||
| # THIS IS AUTOGENERATED. DO NOT EDIT MANUALLY | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for scripts or documentation that generate environment.toml
# Search for references to generating or creating this environment file
rg -i "environment\.toml|generate.*environment" -g '!.codex/environments/*' -C3
# Look for any .codex documentation
fd -e md -e txt . .codex/Repository: toolsplus/nx-forge
Length of output: 44
🏁 Script executed:
# Explore .codex directory structure
find .codex -type f -o -type d | head -20
# Search for any setup, build, or generation scripts
fd -e sh -e js -e py -e ts -e bash | xargs rg -l "environment|generate" | head -20
# Check .codex/config.toml if it exists
if [ -f ".codex/config.toml" ]; then
echo "=== .codex/config.toml ===";
cat .codex/config.toml;
fi
# Look for any scripts in the repo root or .codex
ls -la .codex/*.* 2>/dev/null || true
ls -la *.js *.json *.sh 2>/dev/null || trueRepository: toolsplus/nx-forge
Length of output: 1837
🏁 Script executed:
# View the environment.toml file content
echo "=== .codex/environments/environment.toml ==="
cat .codex/environments/environment.toml
# Search for any references to this environment or "nx-forge"
echo -e "\n=== References to nx-forge environment ==="
rg -i "nx-forge|environments" --type toml
# Search for environment setup/configuration in docs or comments
echo -e "\n=== Environment-related documentation ==="
fd -e md -e txt | xargs rg -l -i "environment|codex" 2>/dev/null | head -10Repository: toolsplus/nx-forge
Length of output: 533
🏁 Script executed:
# Check the documentation files mentioned
echo "=== docs/guides/getting-started.md (first 100 lines) ==="
head -100 docs/guides/getting-started.md
echo -e "\n=== docs/guides/transforming-the-manifest.md (first 100 lines) ==="
head -100 docs/guides/transforming-the-manifest.md
# Search for any Codex environment patterns or examples
echo -e "\n=== References to environment setup ==="
rg -i "environment|setup.*script" docs/ | head -20Repository: toolsplus/nx-forge
Length of output: 10690
Remove the autogenerated comment—this file is manually maintained.
The comment on line 1 claims the file is autogenerated, but no generation script or tool was found in the codebase. Since this environment is being manually created and maintained in the PR, the misleading comment should be removed to avoid confusing future maintainers.
Additionally, this environment is defined but not referenced in .codex/config.toml, so it won't be activated. Either activate it in the main config or document why this environment definition is needed if it's meant to be optional.
Consider using npm ci instead of npm i in the setup script for reproducible, deterministic builds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.codex/environments/environment.toml at line 1, Remove the misleading
autogenerated comment at the top of .codex/environments/environment.toml (delete
the line "# THIS IS AUTOGENERATED. DO NOT EDIT MANUALLY"), then either add this
environment to .codex/config.toml so it will be activated (reference the
environment identifier used inside environment.toml) or add a short comment in
environment.toml explaining why it's intentionally optional; finally, in the
environment's setup script replace any occurrence of "npm i" with "npm ci" to
ensure reproducible installs.
| runCommand( | ||
| `npx -y create-nx-workspace@latest ${projectName} --preset=apps --nxCloud=skip --packageManager=npm --no-interactive`, | ||
| TEST_PROJECTS_ROOT | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(package\.json|nx\.json)" | head -20Repository: toolsplus/nx-forge
Length of output: 155
🏁 Script executed:
cat e2e/nx-forge-e2e/src/utils/test-project.ts | head -50Repository: toolsplus/nx-forge
Length of output: 1470
🏁 Script executed:
find . -name "package.json" -type f | head -5 | xargs grep -l "nx" 2>/dev/nullRepository: toolsplus/nx-forge
Length of output: 109
🏁 Script executed:
cat package.json | grep -A 2 -B 2 '"nx"'Repository: toolsplus/nx-forge
Length of output: 311
🏁 Script executed:
cat packages/nx-forge/package.json | grep -A 2 -B 2 '"nx"'Repository: toolsplus/nx-forge
Length of output: 44
🏁 Script executed:
rg "create-nx-workspace" e2e/ --type ts --type jsRepository: toolsplus/nx-forge
Length of output: 227
🏁 Script executed:
rg "create-nx-workspace" --type ts --type js -A 2 -B 2Repository: toolsplus/nx-forge
Length of output: 442
🏁 Script executed:
cat packages/nx-forge/package.json | head -30Repository: toolsplus/nx-forge
Length of output: 813
🏁 Script executed:
find e2e -name "*.ts" -o -name "*.js" | xargs grep -l "workspace\|version" | head -10Repository: toolsplus/nx-forge
Length of output: 104
🏁 Script executed:
rg "nx\s+version\|create-nx-workspace" --type md --type ts --type js -i | head -20Repository: toolsplus/nx-forge
Length of output: 44
Pin create-nx-workspace to match the repo's Nx version (22.6.1) instead of using @latest.
Line 31 creates the test workspace with an unpinned create-nx-workspace@latest, while the plugin is built against the repo's fixed Nx dependencies (all pinned to 22.6.1). This version skew causes e2e test failures from upstream Nx releases rather than actual regressions. Nx's create-nx-workspace, nx, and @nx/* packages are released in lockstep and must use matching versions. Change to create-nx-workspace@22.6.1 to keep the test harness aligned with the plugin's build environment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/nx-forge-e2e/src/utils/test-project.ts` around lines 30 - 33, The
runCommand invocation that currently calls `create-nx-workspace@latest` should
be changed to pin the Nx release used for test workspaces to 22.6.1; locate the
`runCommand` call that executes `npx -y create-nx-workspace@latest
${projectName} ...` in test-project.ts and replace `@latest` with `@22.6.1` so
the created workspace's `create-nx-workspace`/`nx` versions match the repo's
pinned Nx dependencies and avoid version skew in e2e tests.
1 participant
Summary by CodeRabbit
Tests
Chores