Repository navigation
fix: use 'arkd' container name to match existing branches - #513
Conversation
The arkade-regtest override was using container_name 'ark', but all existing test branches (and the original nigiri setup) use 'arkd'. This caused "No such container: arkd" errors on every open PR after the regtest submodule was merged. - Update regtest submodule (container_name: ark → arkd) - Revert test files to use 'docker exec -t arkd' (matching all branches) - Update CI workflow log capture to use 'arkd'
WalkthroughSwitch Docker container references from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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 |
Deploying wallet-bitcoin with
|
| Latest commit: |
767a981
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4e6e7765.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-arkd-container-name.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
767a981
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://07231f5d.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-arkd-container-name.arkade-wallet.pages.dev |
The nigiri CLI 'ark' subcommand hardcodes container name 'ark', but our override uses 'arkd'. Updated start-env.sh to use direct docker exec for the custom ARKD_IMAGE path.
arkd v0.9.1 has no 'init' subcommand — wallet setup is handled through the admin REST API. Removes 30 failed retries that caused CI environment setup to fail.
The arkade-regtest environment runs more containers than the old test.docker-compose.yml, making Google Chrome tests take ~2x longer. The default 30s Playwright assertion timeout causes borderline tests to fail on CI. Mobile Chrome passes because it runs first while the environment is fresh.
The arkade-regtest environment runs more containers than the old test.docker-compose.yml, making tests take ~2x longer. The default 30s test timeout caused frequent timeouts on slower CI runners. Set global timeout to 60s in playwright.config.ts. Tests with explicit higher timeouts (120s for swap/restore) are unaffected.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/e2e/send.test.ts (1)
333-382: Ensure fee reset always runs withtry/finally.If the test fails before Line 382,
fees clearis skipped and may pollute later tests. Wrap the body after fee setup intry/finally.Proposed refactor
test('should send MAX to onchain address with collaborative exit', async ({ page }) => { // set fees execSync('docker exec -t arkd arkd fees intent --onchain-output "200.0"') + try { - // create wallet - await createWallet(page) + // create wallet + await createWallet(page) - // get offchain address - const arkAddress = await receiveOffchain(page) - expect(arkAddress).toBeDefined() - expect(arkAddress).toBeTruthy() + // get offchain address + const arkAddress = await receiveOffchain(page) + expect(arkAddress).toBeDefined() + expect(arkAddress).toBeTruthy() - // faucet - await faucetOffchain(arkAddress, 1000) - await waitForPaymentReceived(page) + // faucet + await faucetOffchain(arkAddress, 1000) + await waitForPaymentReceived(page) - // ... rest of test steps ... + // ... rest of test steps ... - - // clear fees - execSync('docker exec -t arkd arkd fees clear') + } finally { + // clear fees + execSync('docker exec -t arkd arkd fees clear') + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/e2e/send.test.ts` around lines 333 - 382, The test sets fees with execSync('docker exec -t arkd arkd fees intent ...') but calls execSync('docker exec -t arkd arkd fees clear') only at the end, so if the test fails earlier the fees aren't cleared; wrap the test body that runs after the fees intent (everything from createWallet/receiveOffchain/... through assertions and the final clicks) in a try/finally and move the execSync('docker exec -t arkd arkd fees clear') into the finally block so fees are always reset; reference execSync, createWallet, receiveOffchain, faucetOffchain, waitForPaymentReceived, and the existing final execSync call to locate where to insert the try/finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/e2e/send.test.ts`:
- Around line 333-382: The test sets fees with execSync('docker exec -t arkd
arkd fees intent ...') but calls execSync('docker exec -t arkd arkd fees clear')
only at the end, so if the test fails earlier the fees aren't cleared; wrap the
test body that runs after the fees intent (everything from
createWallet/receiveOffchain/... through assertions and the final clicks) in a
try/finally and move the execSync('docker exec -t arkd arkd fees clear') into
the finally block so fees are always reset; reference execSync, createWallet,
receiveOffchain, faucetOffchain, waitForPaymentReceived, and the existing final
execSync call to locate where to insert the try/finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 982a8a32-84d2-4cbc-a11b-1fa5206d34c6
📒 Files selected for processing (2)
playwright.config.tssrc/test/e2e/send.test.ts
✅ Files skipped from review due to trivial changes (1)
- playwright.config.ts
Summary
Fixes all open PRs broken by #492 (arkade-regtest submodule merge).
Root causes
container_name: ark, but all existing branches usedocker exec -t arkd(the original nigiri name)arkdv0.9.1 binary has noinitsubcommand — wallet setup is already handled via the admin REST APItest.docker-compose.ymlChanges
arkd, remove broken init step, use direct docker execdocker exec -t arkd(matching all existing branches)arkdTest plan
Summary by CodeRabbit
Chores
Tests