tests: move Playwright CI to stock image#3995
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request refactors the regtest infrastructure to use Docker containers and named volumes instead of host filesystem directories. The CI configuration is updated to run Playwright tests in a containerized environment with Docker access, while the backend now reads the Electrum server host from an environment variable. Test helpers add Docker volume cleanup, the Python test server adds RIPEMD-160 backend fallback logic, and the regtest startup script replaces temporary directories with Docker-managed volumes and adjusts network configuration to use explicit port publishing instead of host networking. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/run_regtest.sh`:
- Line 88: Replace the unquoted command substitution fragment "-u $(id -u
$USER)" with a quoted form to prevent word-splitting and ensure the user
variable is also quoted, e.g. use "-u \"$(id -u \"$USER\")\"" (apply the same
change to the identical occurrence earlier in the script referenced by the
similar unquoted "$(id -u $USER)" usage on line 70); update both spots so the
command substitution and $USER are properly quoted.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eee05cc7-45d8-4004-b5cf-dab5c355fed6
📒 Files selected for processing (5)
.github/workflows/ci.ymlbackend/backend.gofrontends/web/tests/helpers/regtest.tsfrontends/web/tests/util/aopp/server.pyscripts/run_regtest.sh
bznein
force-pushed
the
playwright-stock-image
branch
from
a317941 to
7dddcba
Compare
|
I left this onse on hold for a while as there were more pressing things, but I think now it's a good time to get this merged, as it can drastically improve run times for playwright |
| runs-on: ubuntu-22.04 | ||
| needs: test-lint | ||
| container: | ||
| # Keep this in sync with frontends/web/package-lock.json. |
There was a problem hiding this comment.
In case I update the lock file like here: #4144 is there anything else I should change to "keep in sync frontends/web/package-lock.json." ?
There was a problem hiding this comment.
If the PR were to up[date the version of playwright to, let's say, 1.58.0, you would need to update it below as well.
Let me know if the comment is not clear and we should update it.
There was a problem hiding this comment.
Makes sense 👍 , but in that case I think it makes sense to change to where the playwright devDependency is defined:
| # Keep this in sync with frontends/web/package-lock.json. | |
| # Keep this in sync with frontends/web/package.json. |
| run: | | ||
| cd frontends/web | ||
| npm install | ||
| npm ci |
| npx playwright install --with-deps chromium | ||
| else | ||
| npx playwright install --with-deps webkit | ||
| fi |
There was a problem hiding this comment.
nice 👍 this can take 10+ minutes, see https://github.com/BitBoxSwiss/bitbox-wallet-app/actions/runs/25811176958/job/75828736293?pr=4144
There was a problem hiding this comment.
Yeah that's the main winner. Sometime it takes very little time, but it can be super annoying when it takes so much
Run the Playwright job in the upstream Playwright container and adapt the regtest/AOPP test setup to that environment. Running on the Playwright container saves us the step of installing browsers, which can take a few minutes; sometimes, depending on network conditions, even up to 15. Install the missing CI dependencies, make the regtest services reachable from the containerized job, add an RBTC electrum host override for CI, and harden the AOPP helper for raw signature recovery and missing RIPEMD160 support.
bznein
force-pushed
the
playwright-stock-image
branch
from
7dddcba to
952116f
Compare
2 participants
Run the Playwright job in the upstream Playwright container and adapt the regtest/AOPP test setup to that environment.
Running on the Playwright container saves us the step of installing browsers, which can take a few minutes; sometimes, depending on network conditions, even up to 15.
Install the missing CI dependencies, make the regtest services reachable from the containerized job, add an RBTC electrum host override for CI, and harden the AOPP helper for raw signature recovery and missing RIPEMD160 support.
Before asking for reviews, here is a check list of the most common things you might need to consider: