Conversation
There was a problem hiding this comment.
Pull request overview
Revamps the API-focused Playwright E2E setup by removing the legacy YAML/EntitiesBuilder-style API specs and introducing a DB-backed seeding approach plus updated CI workflows to run the new API test subset.
Changes:
- Removed most
e2e/tests/api/**specs/fixtures and consolidated API coverage down to a smaller set (notably deployments/workspace). - Added
e2e/seed.tsto seed a workspace/user/api-key directly via@ctrlplane/dband write Playwright.state/*files. - Updated Playwright config and GitHub Actions workflows to run API tests in CI using the new approach.
Reviewed changes
Copilot reviewed 56 out of 58 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Adds @ctrlplane/db to the e2e workspace and updates lock snapshots accordingly. |
e2e/tests/api/deployments.spec.ts |
Rewrites deployment API tests to use workspace-scoped endpoints and job-agent associations. |
e2e/tests/api/workspace.spec.ts |
Removes the workspace-by-slug test case. |
e2e/seed.ts |
New DB seeding script that generates a workspace/user/api-key and writes .state fixtures. |
e2e/playwright.config.ts |
Adds an api-tests project and excludes API specs from the UI project. |
e2e/package.json |
Adds a seed script and updates the deployments test command to target api-tests. |
.github/workflows/playwright.yml |
Introduces a dedicated API test job on PRs with Postgres + seed + dev servers + Playwright API tests. |
.github/workflows/apps-api.yaml |
Adds an api-tests job to the apps/api workflow that seeds + starts servers + runs Playwright API tests. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await db.insert(user).values({ | ||
| id: userId, | ||
| name: "E2E Test User", | ||
| email: `e2e-${crypto.randomBytes(4).toString("hex")}@test.local`, | ||
| emailVerified: true, | ||
| activeWorkspaceId: null, | ||
| }); | ||
|
|
||
| await db.insert(workspace).values({ | ||
| id: workspaceId, | ||
| name: workspaceName, | ||
| slug: workspaceName, | ||
| }); | ||
|
|
||
| await db | ||
| .update(user) | ||
| .set({ activeWorkspaceId: workspaceId }) | ||
| .where(eq(user.id, userId)); |
There was a problem hiding this comment.
The seed script creates a user + workspace but doesn’t create an RBAC link (e.g., entity_role) granting that user access to the workspace (nor does it set the user’s systemRole to admin). This can make seeded credentials unusable for any endpoints that enforce workspace membership (e.g., listing/getting workspaces) and diverges from the app’s normal workspace creation flow. Consider inserting the appropriate entity_role row for the user/workspace (admin role) or setting systemRole to "admin" for the seeded user.
| ); | ||
|
|
||
| console.log(`Workspace: ${workspaceName} (${workspaceId})`); | ||
| console.log(`API Key: ${apiKey}`); |
There was a problem hiding this comment.
This prints the full generated API key into CI logs. Even if intended for tests, it’s still an active credential for the running test environment and can be reused within the job while services are up. Prefer masking the secret (log only the prefix) or omit logging the API key entirely.
| console.log(`API Key: ${apiKey}`); | |
| console.log(`API Key: ${prefix}.****`); |
| "test:resources": "pnpm exec playwright test tests/api/resources.spec.ts", | ||
| "test:environments": "pnpm exec playwright test tests/api/environments.spec.ts", | ||
| "test:policies": "pnpm exec playwright test tests/api/policies/", | ||
| "test:deployments": "pnpm exec playwright test tests/api/deployments.spec.ts", | ||
| "test:deployments": "pnpm exec playwright test --project=api-tests tests/api/deployments.spec.ts", | ||
| "test:release-targets": "pnpm exec playwright test tests/api/release-targets.spec.ts", |
There was a problem hiding this comment.
Several scripts still reference API spec files/directories that were removed in this PR (e.g., tests/api/resources.spec.ts, tests/api/environments.spec.ts, tests/api/policies/, etc.). These scripts will now fail if invoked. Consider removing them or updating them to point at the remaining API specs/projects.
| projects: [ | ||
| { | ||
| name: "setup", | ||
| testMatch: /.*\.setup\.ts/, | ||
| }, | ||
| { | ||
| name: "api-tests", | ||
| testMatch: /tests\/api\/.*\.spec\.ts/, | ||
| use: { | ||
| storageState: authFile, | ||
| }, | ||
| }, | ||
| { | ||
| name: "chromium", | ||
| use: { | ||
| ...devices["Desktop Chrome"], | ||
| storageState: authFile, | ||
| }, | ||
| dependencies: ["setup"], | ||
| testIgnore: /tests\/api\/.*/, | ||
| testMatch: /.*\.spec\.ts/, | ||
| }, |
There was a problem hiding this comment.
The config defines projects named "setup", "api-tests", and "chromium", but the e2e package scripts reference a "ui-tests" project (e.g., test:web). Either rename/alias the UI project to "ui-tests" (or adjust scripts) so pnpm -F @ctrlplane/e2e test:web works.
.github/workflows/playwright.yml
Outdated
| - name: Start API server | ||
| run: pnpm -F @ctrlplane/web-api dev & | ||
| env: | ||
| PORT: "8080" | ||
|
|
||
| # - name: Install Playwright Browsers | ||
| # run: pnpm exec playwright install --with-deps | ||
| - name: Start web dev server | ||
| run: pnpm -F @ctrlplane/web dev & | ||
|
|
There was a problem hiding this comment.
This workflow starts the API and web dev servers manually, but Playwright is also configured to start a dev server via webServer.command in e2e/playwright.config.ts (and on CI reuseExistingServer is false). That combination commonly fails due to the port already being in use or the webServer command exiting non-zero. Prefer either (1) remove these manual start steps and rely on Playwright’s webServer, or (2) set reuseExistingServer: true on CI / conditionally disable webServer when servers are started externally.
.github/workflows/apps-api.yaml
Outdated
| - name: Start servers | ||
| run: | | ||
| pnpm -F @ctrlplane/web-api dev & | ||
| pnpm -F @ctrlplane/web dev & | ||
|
|
||
| - name: Wait for services | ||
| run: | | ||
| timeout 60s bash -c 'until curl -sf http://localhost:5173/api/healthz; do sleep 2; done' | ||
|
|
There was a problem hiding this comment.
This workflow starts the API and web dev servers manually, but Playwright is also configured to start a dev server via webServer.command in e2e/playwright.config.ts (and on CI reuseExistingServer is false). That combination commonly fails due to the port already being in use or the webServer command exiting non-zero. Prefer either (1) remove this manual start step and rely on Playwright’s webServer, or (2) set reuseExistingServer: true on CI / conditionally disable webServer when servers are started externally.
| - name: Start servers | |
| run: | | |
| pnpm -F @ctrlplane/web-api dev & | |
| pnpm -F @ctrlplane/web dev & | |
| - name: Wait for services | |
| run: | | |
| timeout 60s bash -c 'until curl -sf http://localhost:5173/api/healthz; do sleep 2; done' |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdded an api-tests CI job (Postgres service, migrations, seeding, start servers) and Playwright API project; introduced an e2e seeding script and test config changes; refactored deployments tests to runtime entity creation; and removed a large number of YAML-driven e2e fixtures and Playwright suites. Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant PG as PostgreSQL (service)
participant Repo as Repository
participant Build as pnpm Build
participant DB as Migrations & Seeder
participant API as `@ctrlplane/web-api`
participant Test as Playwright API tests
GHA->>PG: start PostgreSQL 18 (service + health checks)
PG-->>GHA: healthy
GHA->>Repo: checkout code
GHA->>Build: run repo setup & pnpm install/build
Build-->>GHA: build ready
GHA->>DB: write .env (inject POSTGRES_URL) and run migrations
DB-->>GHA: migrations applied
GHA->>DB: run e2e seed (pnpm -F `@ctrlplane/e2e` seed) -> writes .state
DB-->>GHA: seeded
GHA->>API: start web-api (PORT=8080)
GHA->>API: poll /api/healthz until healthy
API-->>GHA: healthy
GHA->>Test: run pnpm -F `@ctrlplane/e2e` test:api (BASE_URL set)
Test->>API: execute API requests
API-->>Test: responses
Test-->>GHA: tests complete
GHA->>GHA: upload Playwright report artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
e2e/seed.ts (1)
50-51: UsefileURLToPath(import.meta.url)for better portability and consistency with the codebase.While
import.meta.dirnameworks in Node.js 22.10.0+, other scripts in the codebase (packages/db/migrate.ts,apps/api/src/server.ts) use thefileURLToPath(import.meta.url)pattern. Aligning to this established convention improves portability and reduces inconsistency.Suggested refactor
+import { fileURLToPath } from "node:url"; + const stateDir = path.join( - import.meta.dirname, + path.dirname(fileURLToPath(import.meta.url)), ".state" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/seed.ts` around lines 50 - 51, Replace the use of import.meta.dirname when constructing stateDir with the fileURLToPath(import.meta.url) pattern used elsewhere: compute a file-system path from import.meta.url using fileURLToPath and derive its directory (so path.join(path.dirname(fileURLToPath(import.meta.url)), ".state") or equivalent), and add the necessary import for fileURLToPath from 'url'; update the stateDir declaration to use this computed directory instead of import.meta.dirname.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/apps-api.yaml:
- Around line 73-75: The workflow step named "Run migrations" currently invokes
the raw tool via "pnpm exec drizzle-kit migrate" which bypasses the repo's
bootstrap logic; change that step to call the repository migration runner
instead (the script that runs packages/db/migrate.ts or the package.json script
that wraps it) so the predefined roles upsert and any bootstrap in
packages/db/migrate.ts are executed; locate the "Run migrations" step and
replace the direct drizzle-kit invocation with the repo migration runner command
that triggers packages/db/migrate.ts.
- Around line 80-90: The workflow currently starts the app servers in the "Start
servers" step while Playwright's global webServer (in e2e/playwright.config.ts,
configured with reuseExistingServer: !process.env.CI) will also attempt to start
a server for the api-tests project, causing a port conflict; fix by removing the
manual startup lines (the pnpm -F `@ctrlplane/web-api` dev & and pnpm -F
`@ctrlplane/web` dev & in the Start servers step) so Playwright owns the server
lifecycle, or alternatively disable webServer for the api-tests project in
e2e/playwright.config.ts (or override its project config) so Playwright will
reuse the externally started servers (referencing the webServer config,
reuseExistingServer, and the Run API tests step that invokes pnpm -F
`@ctrlplane/e2e` test:api).
In @.github/workflows/playwright.yml:
- Around line 50-51: The workflow currently runs the one-off command "pnpm -F
`@ctrlplane/db` push" which bypasses the versioned SQL in packages/db/drizzle/ and
the predefined-role bootstrap in packages/db/migrate.ts; replace that step so
the CI uses the same migration path as the app by invoking the repository's
migration script (the logic in packages/db/migrate.ts) or the package script
that runs it, ensuring versioned SQL in packages/db/drizzle/ and the
predefined-role bootstrap are applied instead of a direct db push.
- Around line 59-72: Remove the manual server startup steps from the GitHub
Actions workflow (the "Start API server", "Start web dev server", and "Wait for
services" steps) so Playwright's webServer in playwright.config.ts (check the
webServer config and its reuseExistingServer flag) can manage the dev stack
lifecycle; keep the test invocation (e.g., the "Run API tests" pnpm -F
`@ctrlplane/e2e` test:api step) but ensure it relies on Playwright to start/await
the servers rather than starting them in the workflow.
In `@e2e/tests/api/deployments.spec.ts`:
- Around line 64-71: Capture the result of the api.PUT call that links the
system to the deployment (the api.PUT invocation using path params workspaceId,
systemId, deploymentId) and assert it succeeded before proceeding; e.g., store
the response from api.PUT in a variable and add an assertion that the status is
a success (2xx) or that response.ok is true so the test fails if the PUT
returned a 4xx/5xx, then continue to the subsequent GET as before.
- Around line 92-121: The test asserts that POST creates a deployment-job-agent
link (checking deployment.jobAgents, jobAgentId, jobAgentConfig) but the POST
handler currently only creates the deployment row and does not persist job-agent
links; either remove the expectation or implement POST-side linking: update the
POST handler to detect jobAgentId/jobAgentConfig in the request body and persist
the same deployment↔job-agent records the PUT upsert path uses (reuse the PUT's
persistence logic or extract it into a shared function used by both the POST
handler and the PUT upsert handler), then return the deployment with jobAgents
populated so the test passes.
---
Nitpick comments:
In `@e2e/seed.ts`:
- Around line 50-51: Replace the use of import.meta.dirname when constructing
stateDir with the fileURLToPath(import.meta.url) pattern used elsewhere: compute
a file-system path from import.meta.url using fileURLToPath and derive its
directory (so path.join(path.dirname(fileURLToPath(import.meta.url)), ".state")
or equivalent), and add the necessary import for fileURLToPath from 'url';
update the stateDir declaration to use this computed directory instead of
import.meta.dirname.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4374c53e-b0c8-4864-b0f1-161676b21f9d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (57)
.github/workflows/apps-api.yaml.github/workflows/playwright.ymle2e/api/schema.tse2e/package.jsone2e/playwright.config.tse2e/seed.tse2e/tests/api/deployment-remove-event.spec.tse2e/tests/api/deployment-remove-event.spec.yamle2e/tests/api/deployment-variable.spec.tse2e/tests/api/deployment-variable.spec.yamle2e/tests/api/deployment-version.spec.tse2e/tests/api/deployment-version.spec.yamle2e/tests/api/deployments.spec.tse2e/tests/api/deployments.spec.yamle2e/tests/api/environments.spec.tse2e/tests/api/environments.spec.yamle2e/tests/api/job-agents.spec.tse2e/tests/api/job-agents.spec.yamle2e/tests/api/jobs/new-entity-triggers.spec.tse2e/tests/api/jobs/new-entity-triggers.spec.yamle2e/tests/api/policies/approval-policy.spec.tse2e/tests/api/policies/approval-policy.spec.yamle2e/tests/api/policies/environment-version-rollout-policy.spec.tse2e/tests/api/policies/environment-version-rollout-policy.spec.yamle2e/tests/api/policies/matched-release-targets.spec.tse2e/tests/api/policies/matched-release-targets.spec.yamle2e/tests/api/policies/retry-policy.spec.tse2e/tests/api/policies/retry-policy.spec.yamle2e/tests/api/policies/version-cooldown-policy.spec.yamle2e/tests/api/policies/version-selector-policy.spec.tse2e/tests/api/policies/version-selector-policy.spec.yamle2e/tests/api/release-target-lock.spec.tse2e/tests/api/release-target-lock.spec.yamle2e/tests/api/release-targets.spec.tse2e/tests/api/release-targets.spec.yamle2e/tests/api/releases/entity-creation.spec.tse2e/tests/api/releases/redeploy.spec.tse2e/tests/api/releases/redeploy.spec.yamle2e/tests/api/releases/release.spec.yamle2e/tests/api/releases/variable-release.spec.tse2e/tests/api/releases/version-release.spec.tse2e/tests/api/resource-grouping.spec.tse2e/tests/api/resource-grouping.spec.yamle2e/tests/api/resource-providers.spec.tse2e/tests/api/resource-relationships.spec.tse2e/tests/api/resource-relationships.spec.yamle2e/tests/api/resource-variables.spec.tse2e/tests/api/resource-variables.spec.yamle2e/tests/api/resources.spec.tse2e/tests/api/resources.spec.yamle2e/tests/api/version-dependency.spec.tse2e/tests/api/version-dependency.spec.yamle2e/tests/api/version-pinning.spec.tse2e/tests/api/version-pinning.spec.yamle2e/tests/api/workspace.spec.tse2e/tests/api/yaml-import.spec.tse2e/tests/api/yaml-import.spec.yaml
💤 Files with no reviewable changes (50)
- e2e/tests/api/policies/approval-policy.spec.yaml
- e2e/tests/api/deployment-remove-event.spec.yaml
- e2e/tests/api/job-agents.spec.yaml
- e2e/tests/api/releases/release.spec.yaml
- e2e/tests/api/version-pinning.spec.yaml
- e2e/tests/api/policies/matched-release-targets.spec.yaml
- e2e/tests/api/release-target-lock.spec.yaml
- e2e/tests/api/workspace.spec.ts
- e2e/tests/api/deployment-version.spec.yaml
- e2e/tests/api/release-targets.spec.yaml
- e2e/tests/api/policies/retry-policy.spec.yaml
- e2e/tests/api/deployment-variable.spec.yaml
- e2e/tests/api/policies/version-selector-policy.spec.yaml
- e2e/tests/api/version-dependency.spec.yaml
- e2e/tests/api/resource-relationships.spec.yaml
- e2e/tests/api/policies/version-cooldown-policy.spec.yaml
- e2e/tests/api/policies/environment-version-rollout-policy.spec.yaml
- e2e/tests/api/deployments.spec.yaml
- e2e/tests/api/resource-grouping.spec.yaml
- e2e/tests/api/releases/redeploy.spec.yaml
- e2e/tests/api/release-target-lock.spec.ts
- e2e/tests/api/yaml-import.spec.yaml
- e2e/tests/api/deployment-variable.spec.ts
- e2e/tests/api/deployment-remove-event.spec.ts
- e2e/tests/api/release-targets.spec.ts
- e2e/tests/api/releases/redeploy.spec.ts
- e2e/tests/api/job-agents.spec.ts
- e2e/tests/api/resource-grouping.spec.ts
- e2e/tests/api/releases/entity-creation.spec.ts
- e2e/tests/api/jobs/new-entity-triggers.spec.yaml
- e2e/tests/api/environments.spec.yaml
- e2e/tests/api/version-pinning.spec.ts
- e2e/tests/api/deployment-version.spec.ts
- e2e/tests/api/environments.spec.ts
- e2e/tests/api/releases/version-release.spec.ts
- e2e/tests/api/yaml-import.spec.ts
- e2e/tests/api/policies/version-selector-policy.spec.ts
- e2e/tests/api/policies/retry-policy.spec.ts
- e2e/tests/api/resource-variables.spec.yaml
- e2e/tests/api/resource-variables.spec.ts
- e2e/tests/api/resources.spec.yaml
- e2e/tests/api/resource-providers.spec.ts
- e2e/tests/api/resource-relationships.spec.ts
- e2e/tests/api/version-dependency.spec.ts
- e2e/tests/api/resources.spec.ts
- e2e/tests/api/policies/approval-policy.spec.ts
- e2e/tests/api/policies/matched-release-targets.spec.ts
- e2e/tests/api/policies/environment-version-rollout-policy.spec.ts
- e2e/tests/api/releases/variable-release.spec.ts
- e2e/tests/api/jobs/new-entity-triggers.spec.ts
| - name: Run migrations | ||
| working-directory: packages/db | ||
| run: pnpm exec drizzle-kit migrate |
There was a problem hiding this comment.
Use the repo migration runner here, not raw drizzle-kit migrate.
This step applies the SQL files, but it skips the extra bootstrap in packages/db/migrate.ts that upserts the predefined roles afterwards. That leaves the API E2E job exercising a DB initialization path different from the one the app uses elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/apps-api.yaml around lines 73 - 75, The workflow step
named "Run migrations" currently invokes the raw tool via "pnpm exec drizzle-kit
migrate" which bypasses the repo's bootstrap logic; change that step to call the
repository migration runner instead (the script that runs packages/db/migrate.ts
or the package.json script that wraps it) so the predefined roles upsert and any
bootstrap in packages/db/migrate.ts are executed; locate the "Run migrations"
step and replace the direct drizzle-kit invocation with the repo migration
runner command that triggers packages/db/migrate.ts.
.github/workflows/apps-api.yaml
Outdated
| - name: Start servers | ||
| run: | | ||
| pnpm -F @ctrlplane/web-api dev & | ||
| pnpm -F @ctrlplane/web dev & | ||
|
|
||
| - name: Wait for services | ||
| run: | | ||
| timeout 60s bash -c 'until curl -sf http://localhost:5173/api/healthz; do sleep 2; done' | ||
|
|
||
| - name: Run API tests | ||
| run: pnpm -F @ctrlplane/e2e test:api |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the workflow file exists and check the relevant section
if [ -f ".github/workflows/apps-api.yaml" ]; then
echo "=== Workflow file lines 75-95 ==="
sed -n '75,95p' .github/workflows/apps-api.yaml
else
echo "Workflow file not found"
fiRepository: ctrlplanedev/ctrlplane
Length of output: 682
🏁 Script executed:
# Check if e2e/playwright.config.ts exists and examine its webServer configuration
if [ -f "e2e/playwright.config.ts" ]; then
echo "=== playwright.config.ts content ==="
cat -n "e2e/playwright.config.ts"
else
echo "Playwright config not found at e2e/playwright.config.ts"
fiRepository: ctrlplanedev/ctrlplane
Length of output: 2337
🏁 Script executed:
# Search for where test:api is defined (likely in package.json)
echo "=== Searching for test:api definition ==="
if [ -f "package.json" ]; then
cat package.json | grep -A 2 -B 2 "test:api" || echo "test:api not found in root package.json"
fi
# Also check if there's an e2e/package.json
if [ -f "e2e/package.json" ]; then
echo "=== e2e/package.json ==="
cat e2e/package.json | grep -A 2 -B 2 "test:api" || echo "test:api not found in e2e/package.json"
fiRepository: ctrlplanedev/ctrlplane
Length of output: 446
Don't pre-start the app if Playwright is still managing webServer.
e2e/playwright.config.ts configures a global webServer with reuseExistingServer: !process.env.CI (line 62), meaning in CI, reuse is disabled. The api-tests project inherits this global config without override. When pnpm -F @ctrlplane/e2e test:api runs, Playwright will attempt to start its own server command via pnpm dev despite servers already running from lines 84-85, causing port 5173 conflict. Either disable webServer for the api-tests project, or remove the manual server startup and let Playwright own it entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/apps-api.yaml around lines 80 - 90, The workflow currently
starts the app servers in the "Start servers" step while Playwright's global
webServer (in e2e/playwright.config.ts, configured with reuseExistingServer:
!process.env.CI) will also attempt to start a server for the api-tests project,
causing a port conflict; fix by removing the manual startup lines (the pnpm -F
`@ctrlplane/web-api` dev & and pnpm -F `@ctrlplane/web` dev & in the Start servers
step) so Playwright owns the server lifecycle, or alternatively disable
webServer for the api-tests project in e2e/playwright.config.ts (or override its
project config) so Playwright will reuse the externally started servers
(referencing the webServer config, reuseExistingServer, and the Run API tests
step that invokes pnpm -F `@ctrlplane/e2e` test:api).
.github/workflows/playwright.yml
Outdated
| - name: Push database schema | ||
| run: pnpm -F @ctrlplane/db push |
There was a problem hiding this comment.
db push makes this workflow diverge from the real migration path.
pnpm -F @ctrlplane/db push bypasses both the versioned SQL in packages/db/drizzle/ and the predefined-role bootstrap in packages/db/migrate.ts. That means this job is validating against a database initialization path that does not match the rest of the app or the other API workflow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/playwright.yml around lines 50 - 51, The workflow
currently runs the one-off command "pnpm -F `@ctrlplane/db` push" which bypasses
the versioned SQL in packages/db/drizzle/ and the predefined-role bootstrap in
packages/db/migrate.ts; replace that step so the CI uses the same migration path
as the app by invoking the repository's migration script (the logic in
packages/db/migrate.ts) or the package script that runs it, ensuring versioned
SQL in packages/db/drizzle/ and the predefined-role bootstrap are applied
instead of a direct db push.
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}", | ||
| { | ||
| params: { | ||
| path: { | ||
| deploymentId, | ||
| }, | ||
| path: { workspaceId: workspace.id, systemId, deploymentId }, | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Assert that the system-link request actually succeeded.
If Lines 64-71 return 4xx/5xx, this test still passes because the later GET only proves that the deployment exists. Capture the PUT response and assert success so the association route is actually covered.
Suggested fix
- await api.PUT(
+ const linkRes = await api.PUT(
"/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}",
{
params: {
path: { workspaceId: workspace.id, systemId, deploymentId },
},
},
);
+ expect(linkRes.response.ok()).toBeTruthy();📝 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.
| await api.PUT( | |
| "/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}", | |
| { | |
| params: { | |
| path: { | |
| deploymentId, | |
| }, | |
| path: { workspaceId: workspace.id, systemId, deploymentId }, | |
| }, | |
| }, | |
| ); | |
| const linkRes = await api.PUT( | |
| "/v1/workspaces/{workspaceId}/systems/{systemId}/deployments/{deploymentId}", | |
| { | |
| params: { | |
| path: { workspaceId: workspace.id, systemId, deploymentId }, | |
| }, | |
| }, | |
| ); | |
| expect(linkRes.response.ok()).toBeTruthy(); |
🧰 Tools
🪛 GitHub Actions: Apps / API
[error] Command failed with exit code 1: tsx seed.ts (pnpm -F @ctrlplane/e2e seed).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/api/deployments.spec.ts` around lines 64 - 71, Capture the result
of the api.PUT call that links the system to the deployment (the api.PUT
invocation using path params workspaceId, systemId, deploymentId) and assert it
succeeded before proceeding; e.g., store the response from api.PUT in a variable
and add an assertion that the status is a success (2xx) or that response.ok is
true so the test fails if the PUT returned a 4xx/5xx, then continue to the
subsequent GET as before.
| const createRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/deployments", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { | ||
| name, | ||
| slug: name, | ||
| jobAgentId, | ||
| jobAgentConfig: { repo: "my-repo" }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| await page.waitForTimeout(10_000); | ||
| ); | ||
|
|
||
| const deploymentId = deployment.data?.id; | ||
| expect(deploymentId).toBeDefined(); | ||
| if (!deploymentId) throw new Error("Deployment ID is undefined"); | ||
| expect(createRes.response.status).toBe(202); | ||
| const deploymentId = createRes.data!.id; | ||
|
|
||
| const resources = await api.GET( | ||
| "/v1/deployments/{deploymentId}/resources", | ||
| const getRes = await api.GET( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { | ||
| params: { | ||
| path: { | ||
| deploymentId, | ||
| }, | ||
| path: { workspaceId: workspace.id, deploymentId }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| expect(resources.response.status).toBe(200); | ||
| expect(resources.data?.resources?.length).toBe(1); | ||
| const receivedResource = resources.data?.resources?.[0]; | ||
| expect(receivedResource).toBeDefined(); | ||
| if (!receivedResource) throw new Error("Resource is undefined"); | ||
| expect(receivedResource.identifier).toBe( | ||
| builder.refs.resources.find((r) => r.metadata?.env === "qa")?.identifier, | ||
| ); | ||
|
|
||
| const releaseTargets = await api.GET( | ||
| "/v1/resources/{resourceId}/release-targets", | ||
| { params: { path: { resourceId: receivedResource.id } } }, | ||
| ); | ||
|
|
||
| expect(releaseTargets.response.status).toBe(200); | ||
| expect(releaseTargets.data?.length).toBe(1); | ||
| const releaseTarget = releaseTargets.data?.[0]; | ||
| expect(releaseTarget).toBeDefined(); | ||
| if (!releaseTarget) throw new Error("Release target is undefined"); | ||
| expect(releaseTarget.deployment.id).toBe(deploymentId); | ||
| const matchedEnvironment = builder.refs.environments.find( | ||
| (e) => e.id === releaseTarget.environment.id, | ||
| ); | ||
| expect(matchedEnvironment).toBeDefined(); | ||
| expect(getRes.response.status).toBe(200); | ||
| const { deployment } = getRes.data!; | ||
| expect(deployment.jobAgents).toHaveLength(1); | ||
| expect(deployment.jobAgents![0]!.ref).toBe(jobAgentId); | ||
| expect(deployment.jobAgents![0]!.config).toEqual({ repo: "my-repo" }); |
There was a problem hiding this comment.
This test expects POST-time job-agent linking that the handler does not implement.
The POST handler in apps/api/src/routes/v1/workspaces/deployments.ts:222-246 only creates the deployment row; it never writes a deployment-job-agent link. That means deployment.jobAgents on Line 119 will stay empty, so this spec is asserting behavior the API does not currently support. Either add POST-side linking in the API, or move this expectation to the PUT upsert path that already exercises job-agent persistence.
🧰 Tools
🪛 GitHub Actions: Apps / API
[error] Command failed with exit code 1: tsx seed.ts (pnpm -F @ctrlplane/e2e seed).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/api/deployments.spec.ts` around lines 92 - 121, The test asserts
that POST creates a deployment-job-agent link (checking deployment.jobAgents,
jobAgentId, jobAgentConfig) but the POST handler currently only creates the
deployment row and does not persist job-agent links; either remove the
expectation or implement POST-side linking: update the POST handler to detect
jobAgentId/jobAgentConfig in the request body and persist the same
deployment↔job-agent records the PUT upsert path uses (reuse the PUT's
persistence logic or extract it into a shared function used by both the POST
handler and the PUT upsert handler), then return the deployment with jobAgents
populated so the test passes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/apps-api.yaml (1)
73-75:⚠️ Potential issue | 🟠 MajorUse the repo migration bootstrap here, not raw
drizzle-kit.Line 75 applies the migrations directly, but it skips the repo’s migration bootstrap path. That can leave the API E2E job seeded against a database state that differs from the app’s normal initialization flow.
This verifies which package script wraps migrations and whether
packages/db/migrate.tsdoes extra bootstrap work; expected: there is a repo migration entrypoint that should replace the directdrizzle-kitcall.#!/bin/bash set -euo pipefail echo "== migrate-related package scripts ==" python - <<'PY' import json, pathlib for path in pathlib.Path('.').rglob('package.json'): try: data = json.loads(path.read_text()) except Exception: continue for name, cmd in (data.get('scripts') or {}).items(): if 'migrate' in name: print(f"{path}: {name}={cmd}") PY echo echo "== packages/db migration bootstrap ==" fd -a 'migrate.ts' packages/db -x sh -c ' echo "--- $1 ---" sed -n "1,220p" "$1" ' sh {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/apps-api.yaml around lines 73 - 75, The workflow currently runs migrations with the raw command "pnpm exec drizzle-kit migrate"; replace that with the repository's migration bootstrap entrypoint instead (the package script that wraps migrations and the migrate.ts bootstrap), so the CI uses the same initialization logic as the app; update the "Run migrations" step to invoke the package script (e.g., the repo's "migrate" npm script) or the migration bootstrap script used by packages/db (migrate.ts) rather than calling drizzle-kit directly, ensuring any extra bootstrapping/seeding logic in the repo migration entrypoint runs in CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/playwright.config.ts`:
- Around line 39-45: The "api-tests" project configuration is missing a
dependency on the setup project so its storageState (authFile /
".state/user.json") may not exist; update the Playwright config by adding
dependencies: ["setup"] to the "api-tests" project entry (the project object
where name === "api-tests" and testMatch === /tests\/api\/.*\.spec\.ts/) so the
setup task runs first, or alternatively move the auth bootstrap into an
API-specific fixture used by "api-tests" to ensure the storageState file is
created before tests run.
---
Duplicate comments:
In @.github/workflows/apps-api.yaml:
- Around line 73-75: The workflow currently runs migrations with the raw command
"pnpm exec drizzle-kit migrate"; replace that with the repository's migration
bootstrap entrypoint instead (the package script that wraps migrations and the
migrate.ts bootstrap), so the CI uses the same initialization logic as the app;
update the "Run migrations" step to invoke the package script (e.g., the repo's
"migrate" npm script) or the migration bootstrap script used by packages/db
(migrate.ts) rather than calling drizzle-kit directly, ensuring any extra
bootstrapping/seeding logic in the repo migration entrypoint runs in CI.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d3cda14-d8db-49d3-a096-3f31efd670c3
📒 Files selected for processing (2)
.github/workflows/apps-api.yamle2e/playwright.config.ts
| { | ||
| name: "api-tests", | ||
| testMatch: /tests\/api\/.*\.spec\.ts/, | ||
| use: { | ||
| storageState: authFile, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test:api scripts =="
python - <<'PY'
import json, pathlib
for path in pathlib.Path('.').rglob('package.json'):
try:
data = json.loads(path.read_text())
except Exception:
continue
scripts = data.get('scripts') or {}
if 'test:api' in scripts:
print(f"{path}: test:api={scripts['test:api']}")
PY
echo
echo "== auth file readers/writers =="
rg -n -C2 --glob '*.ts' --glob '*.js' 'user\.json|storageState|authFile' e2e
echo
echo "== setup files =="
fd -a '\.setup\.ts$' e2e/tests -x sh -c '
echo "--- $1 ---"
sed -n "1,220p" "$1"
' sh {}Repository: ctrlplanedev/ctrlplane
Length of output: 6407
🏁 Script executed:
cat -n e2e/playwright.config.tsRepository: ctrlplanedev/ctrlplane
Length of output: 2372
🏁 Script executed:
# Check if seed.ts is referenced in package.json or CI configs
rg -n 'seed' e2e/package.json --context 3
echo "---"
git -C e2e ls-files | grep -E '(workflow|\.github)' | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 527
Add dependency to ensure API tests have auth state.
Line 43 reuses .state/user.json, but api-tests does not declare dependencies: ["setup"] (line 52). Running pnpm test:api on a clean runner will fail because the storage state file will not exist. Either add dependencies: ["setup"] to api-tests or move auth bootstrap into an API-specific fixture.
Minimal fix
{
name: "api-tests",
testMatch: /tests\/api\/.*\.spec\.ts/,
+ dependencies: ["setup"],
use: {
storageState: authFile,
},
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/playwright.config.ts` around lines 39 - 45, The "api-tests" project
configuration is missing a dependency on the setup project so its storageState
(authFile / ".state/user.json") may not exist; update the Playwright config by
adding dependencies: ["setup"] to the "api-tests" project entry (the project
object where name === "api-tests" and testMatch === /tests\/api\/.*\.spec\.ts/)
so the setup task runs first, or alternatively move the auth bootstrap into an
API-specific fixture used by "api-tests" to ensure the storageState file is
created before tests run.
Summary by CodeRabbit