feat: enable tests on pr with target dev #192
Conversation
📝 WalkthroughWalkthroughAdds an E2E renderer sync script, updates npm scripts to build and sync renderer before E2E tests, and modifies GitHub Actions workflows to change triggers and use the new E2E build step (including Linux/Xvfb setup for E2E). No exported APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Dev as Developer
participant GH as GitHub Actions
participant NPM as npm scripts
participant Builder as Renderer Build (webpack/...).
participant FS as Filesystem (.erb/scripts)
participant E2E as Playwright E2E Runner
end
Dev->>GH: open PR / trigger workflow
GH->>NPM: run e2e build step (npm run e2e:build)
NPM->>Builder: build renderer (build:renderer)
Builder-->>FS: output built assets (release/dist)
NPM->>FS: run sync-e2e-renderer (node .erb/scripts/sync-e2e-renderer.js)
FS-->>FS: copy release/dist -> .erb/renderer and verify index.html
GH->>E2E: run Playwright tests using .erb/renderer
E2E-->>GH: report test results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
- Add script to sync release/app/dist/renderer -> .erb/renderer - Make `npm run test:e2e` run the build+sync step automatically (no manual cp) - Update GitHub Actions E2E workflow to run `npm run e2e:build` - Ignore .erb/renderer in git
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.erb/scripts/sync-e2e-renderer.js:
- Around line 23-28: The code uses fs.cpSync inside the copyDirContents function
which requires Node ≥16.7 and breaks on Node 14; replace that call with a Node
14-compatible recursive copy routine: keep the ensureDir(to) call, then
implement recursive traversal using fs.readdirSync/fs.statSync to detect files
vs directories, use fs.mkdirSync for dirs and fs.copyFileSync for files (or use
a small helper like copyDirRecursive) so the behavior remains recursive and
force-overwrites; alternatively, change the project devEngine to >=16.7 if you
want to keep fs.cpSync (but if you choose code change, update copyDirContents
and reference ensureDir and copyDirContents in tests).
🧹 Nitpick comments (2)
.erb/scripts/sync-e2e-renderer.js (1)
38-45: Clean destination before copy to avoid stale renderer assets.
cpSyncwon’t remove files that no longer exist in the source; stale assets can linger.♻️ Suggested cleanup before copy
if (!isDirectoryNonEmpty(srcDir)) { throw new Error( `Renderer build output not found or empty at: ${srcDir}. Run "npm run build:renderer" first.`, ); } + fs.rmSync(destDir, { recursive: true, force: true }); copyDirContents(srcDir, destDir);.github/workflows/e2e-tests.yml (1)
33-34: Avoid rebuilding twice in CI.
test:e2ealready runse2e:build(package.json line 44), so this step duplicates work. Consider removing the standalone build step or makingtest:e2eskip the build in CI.🧩 Suggested workflow simplification
- - name: Build application (main + renderer for E2E) - run: npm run e2e:build - env: - NODE_ENV: development - - name: Run E2E tests run: npm run test:e2e
| const copyDirContents = (from, to) => { | ||
| ensureDir(to); | ||
| fs.cpSync(from, to, { | ||
| recursive: true, | ||
| force: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Node.js fs.cpSync version requirement when introduced
💡 Result:
fs.cpSync() was introduced in Node.js v16.7.0 (initially Stability: 1 – Experimental), so you need Node >= 16.7.0 to use it. [1]
It later became non-experimental (stable) in Node.js v22.3.0. [2]
Sources: [1], [2]
🏁 Script executed:
# Find and check package.json for devEngines configuration
find . -name "package.json" -type f | head -5Repository: rosettadb/dbt-studio
Length of output: 105
🏁 Script executed:
# Check the actual sync-e2e-renderer.js file
cat .erb/scripts/sync-e2e-renderer.jsRepository: rosettadb/dbt-studio
Length of output: 1240
🏁 Script executed:
# Check the root package.json for devEngines configuration
cat ./package.json | grep -A 10 "devEngines"Repository: rosettadb/dbt-studio
Length of output: 279
🏁 Script executed:
# Also check release/app/package.json in case it's relevant
cat ./release/app/package.json | grep -A 10 "devEngines"Repository: rosettadb/dbt-studio
Length of output: 46
fs.cpSync requires Node ≥16.7; devEngines allows ≥14.x.
This causes a runtime failure on Node 14 (TypeError: fs.cpSync is not a function). Either bump the engine requirement to ≥16.7 or replace fs.cpSync with a Node 14-compatible copy routine.
🤖 Prompt for AI Agents
In @.erb/scripts/sync-e2e-renderer.js around lines 23 - 28, The code uses
fs.cpSync inside the copyDirContents function which requires Node ≥16.7 and
breaks on Node 14; replace that call with a Node 14-compatible recursive copy
routine: keep the ensureDir(to) call, then implement recursive traversal using
fs.readdirSync/fs.statSync to detect files vs directories, use fs.mkdirSync for
dirs and fs.copyFileSync for files (or use a small helper like copyDirRecursive)
so the behavior remains recursive and force-overwrites; alternatively, change
the project devEngine to >=16.7 if you want to keep fs.cpSync (but if you choose
code change, update copyDirContents and reference ensureDir and copyDirContents
in tests).
2 participants
Summary by CodeRabbit
Chores
Tests
Note