feat: Add container pooling for 60-80% faster test execution#549
Conversation
* modernize * refactor: migrate to biome and standardize code style across project * chore: update biome lint command and add fix script alias * style: standardize quotes to single quotes and fix formatting * refactor: replace forEach loops with for...of and remove unnecessary async keywords * refactor: replace forEach loops with for...of and remove unnecessary async declarations * ci: update workflow commands and disable coverage in ci-ts pipeline * temporarily disable docker
* activate testing in ci * ci: remove frozen lockfile in CI workflow
WalkthroughThis update introduces a container pooling system for emulator management in the Zemu framework, adds new configuration files for Biome, Vitest, and project tooling, and migrates CI/CD workflows to use reusable GitHub Actions. It replaces Yarn and Jest with pnpm and Vitest, updates dependencies, and applies consistent code style changes throughout the codebase. Changes
Sequence Diagram(s)Emulator Startup with Container PoolingsequenceDiagram
participant User
participant Zemu
participant ContainerPool
participant EmuContainer
User->>Zemu: start(options)
alt Pooling enabled & not disabled in options
Zemu->>ContainerPool: initializePool(config) (if needed)
Zemu->>ContainerPool: acquire(model, elfPath, libElfs)
alt Container available
ContainerPool-->>Zemu: pooledContainer
Zemu->>EmuContainer: connect()
Zemu->>Zemu: finalizeStart()
else No container available or error
Zemu->>Zemu: startWithNewContainer()
end
else Pooling disabled
Zemu->>Zemu: startWithNewContainer()
end
Zemu-->>User: (startup complete)
Emulator Close with Container PoolingsequenceDiagram
participant User
participant Zemu
participant ContainerPool
participant EmuContainer
User->>Zemu: close()
alt Using pooled container
Zemu->>ContainerPool: release(container)
alt Release fails
Zemu->>EmuContainer: stop()
end
else Not using pool
Zemu->>EmuContainer: stop()
end
Zemu-->>User: (close complete)
Container Pool InitializationsequenceDiagram
participant Zemu
participant ContainerPool
participant EmuContainer
Zemu->>ContainerPool: initialize(config)
loop For each model in config
loop For N containers
ContainerPool->>EmuContainer: create(elf, ports)
EmuContainer-->>ContainerPool: ready
end
end
ContainerPool-->>Zemu: (pool ready)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (9)
src/constants.ts (1)
39-51: Consider explicit default for new pool toggle
IStartOptionsnow contains optionaldisablePool?: boolean, butDEFAULT_START_OPTIONSomits it.
AddingdisablePool: falseclarifies the intent and avoids downstreamundefinedchecks.approveAction: ButtonKind.ApproveHoldButton, approveKeyword: '', rejectKeyword: '', + disablePool: false, }src/actions.ts (1)
20-26: Dummy button is hard-coded tonanos; consider making it model-agnosticHard-coding the fallback button to
'nanos'may yield incorrect coordinates/timing when the click schedule ultimately drives a different device model. Not a blocker, but worth keeping in mind if you later introduce model-specific behaviour or visual tests that rely on accurate coordinates.src/grpc/index.ts (1)
31-35:const self = thiscan be replaced with arrow callbackUsing an arrow function keeps lexical
thisintact and avoids theselfalias boilerplate.- // eslint-disable-next-line @typescript-eslint/no-this-alias - const self = this - // @ts-expect-error types are missing - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - this.server.addService(rpcDefinition.ledger_go.ZemuCommand.service, { - Exchange(call: any, callback: any, ctx = self) { + this.server.addService(rpcDefinition.ledger_go.ZemuCommand.service, { + Exchange: (call: any, callback: any) => { + const ctx = this ... }, })src/buttons.ts (1)
52-54: Remove stray console output from library code
console.login a shared helper risks polluting stdout for downstream consumers (e.g., test output, CI logs). Prefer throwing, returningundefined, or using a proper logger with verbosity control.- console.log(`Unsupported ButtonKind: ${model}, ${buttonKind}`) + // Consider throwing or using a debug logger instead of console.logCLAUDE.md (2)
14-17:pnpm copy-filesscript is undocumented inpackage.jsonThe docs advertise an extra build step, but there is no matching script in
package.json.
Either add the script or drop the reference to avoid confusion.
94-99: Consider warning about Docker Desktop resource limitsParallel, pre-warmed containers can quickly exhaust the default 2 CPU / 2 GB limits of Docker Desktop on macOS/Windows.
A short note here (or a link to the “Important Notes” section) would help newcomers avoid opaque “container out-of-memory” failures.vitest.config.ts (1)
11-15: Path alias should be resolved to an absolute pathVitest (and Vite) recommend using
resolve(__dirname, 'src')(orfileURLToPath) to avoid win/posix edge-cases and ensure TS path mapping stays in sync.-import { defineConfig } from 'vitest/config' +import { defineConfig } from 'vitest/config' +import { resolve } from 'node:path' ... resolve: { alias: { - '@': './src', + '@': resolve(__dirname, 'src'), }, },tests/basic.s.test.ts (1)
34-38: Vitest’stest.concurrentAPI is still experimentalVitest v1 marks
test.concurrentas experimental; failures are rare but possible.
If stability issues arise, consider plaintest()with the--run-in-bandflag orallowOnlyfilters.src/containerPool.ts (1)
135-136: Consider validating the dummy ELF path.The hardcoded dummy ELF path might not exist in all environments. Consider adding validation or making it configurable.
Consider adding a check for the dummy ELF path existence or make it a configurable constant:
- // Use a dummy ELF path for pool containers - will be replaced when acquired - // For now, we need a valid ELF file path to create the container - const dummyElfPath = 'bin/demoAppS.elf' + // Use a dummy ELF path for pool containers - will be replaced when acquired + const dummyElfPath = process.env.ZEMU_POOL_DUMMY_ELF || 'bin/demoAppS.elf' + if (!fs.existsSync(dummyElfPath)) { + throw new Error(`Dummy ELF path does not exist: ${dummyElfPath}`) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (36)
.eslintrc.json(0 hunks).github/dependabot.yml(1 hunks).github/workflows/ci-ts.yaml(1 hunks).github/workflows/main.yml(0 hunks).github/workflows/publish.yml(1 hunks).gitignore(1 hunks).mise.toml(1 hunks).nvmrc(1 hunks).prettierignore(0 hunks).prettierrc(0 hunks).vscode/extensions.json(1 hunks).vscode/settings.json(1 hunks).yarnrc.yml(0 hunks)CLAUDE.md(1 hunks)biome.json(1 hunks)jest.config.js(0 hunks)package.json(2 hunks)src/Zemu.ts(9 hunks)src/actions.ts(1 hunks)src/buttons.ts(1 hunks)src/buttons_flex.ts(3 hunks)src/buttons_stax.ts(3 hunks)src/constants.ts(1 hunks)src/containerPool.ts(1 hunks)src/emulator.ts(1 hunks)src/grpc/index.ts(2 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)src/zondax.ts(3 hunks)tests/basic.s.test.ts(2 hunks)tests/basic.x.test.ts(2 hunks)tests/globalsetup.ts(1 hunks)tests/minapp/index.ts(2 hunks)tests/pullImageKillOld.ts(1 hunks)tsconfig.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- .prettierignore
- .yarnrc.yml
- jest.config.js
- .eslintrc.json
- .github/workflows/main.yml
- .prettierrc
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/buttons_flex.ts (2)
src/buttons_stax.ts (17)
quitAppButton(27-32)swipeContinueButton(34-39)navRightButton(41-46)navLeftButton(48-53)settingsNavRightButton(56-56)settingsNavnavLeftButton(58-63)settingsQuitButton(65-70)rejectButton(72-77)toggleOption1(80-85)approveTapButton(87-92)prevPageButton(94-99)approveHoldButton(101-106)toggleOption2(109-114)confirmYesButton(123-128)confirmNoButton(130-135)showQRButton(137-142)closeQRButton(144-149)src/types.ts (1)
IButton(73-78)
src/constants.ts (2)
src/index.ts (3)
DEFAULT_START_OPTIONS(20-20)IStartOptions(21-21)ButtonKind(21-21)src/types.ts (2)
IStartOptions(44-65)IDeviceWindow(16-21)
src/buttons_stax.ts (2)
src/buttons_flex.ts (17)
quitAppButton(27-32)swipeContinueButton(34-39)navRightButton(41-46)navLeftButton(48-53)settingsNavRightButton(56-56)settingsNavnavLeftButton(58-63)settingsQuitButton(65-70)rejectButton(72-77)toggleOption1(80-85)approveTapButton(87-92)prevPageButton(94-99)approveHoldButton(101-106)toggleOption2(109-114)confirmYesButton(116-121)confirmNoButton(123-128)showQRButton(130-135)closeQRButton(137-142)src/types.ts (1)
IButton(73-78)
src/buttons.ts (1)
src/types.ts (2)
IButton(73-78)TModel(42-42)
tests/globalsetup.ts (1)
src/Zemu.ts (1)
Zemu(68-1049)
🪛 YAMLlint (1.37.1)
.github/dependabot.yml
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 25-25: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/publish.yml
[error] 24-24: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/ci-ts.yaml
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 34-34: trailing spaces
(trailing-spaces)
[error] 39-39: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 52-52: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Gitleaks (8.27.2)
src/emulator.ts
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Actions: CI-ts
src/containerPool.ts
[error] 48-48: Biome lint/correctness/noUnusedPrivateClassMembers: This private class member 'maxWaitTime' is defined but never used. Unsafe fix suggested: Remove unused declaration.
[error] 166-166: Biome lint/correctness/noUnusedFunctionParameters: The parameter 'transportPort' is unused. Unsafe fix suggested: prepend parameter with an underscore.
[error] 176-176: Biome lint/correctness/noUnusedVariables: The variable 'error' is unused. Unsafe fix suggested: prepend variable with an underscore.
[error] 324-324: Biome lint/correctness/noUnusedVariables: The variable 'model' is unused. Unsafe fix suggested: prepend variable with an underscore.
[error] 17-17: Biome assist/source/organizeImports: The imports and exports are not sorted. Safe fix suggested: Organize Imports.
src/Zemu.ts
[error] 17-17: Biome assist/source/organizeImports: The imports and exports are not sorted. Safe fix suggested: Organize Imports.
[error] 92-92: Biome lint/style/noInferrableTypes: This type annotation is trivially inferred from its initialization. Safe fix suggested: Remove the type annotation.
[error] 94-94: Biome lint/style/noInferrableTypes: This type annotation is trivially inferred from its initialization. Safe fix suggested: Remove the type annotation.
[error] 148-410: Biome format: Formatter would have printed changes to multiple lines including import organization, trailing commas, arrow function parentheses, and whitespace fixes.
🔇 Additional comments (37)
.gitignore (1)
176-177: Addition looks good & aligns with new snapshot workflow
Ignoringtests/snapshots-tmp/prevents committing ephemeral snapshot artefacts generated by Vitest. No concerns.src/index.ts (1)
16-22: Pure style change – OK
Import/export quote-style switch is in line with the new Biome config; nothing functional altered.src/buttons_stax.ts (1)
151-160: Map entry will break after rename
Once the symbol is fixed (see previous comment) adjust the map accordingly:- [ButtonKind.SettingsNavLeftButton, stax.settingsNavnavLeftButton], + [ButtonKind.SettingsNavLeftButton, stax.settingsNavLeftButton],src/buttons_flex.ts (1)
144-153: Update map after symbol rename- [ButtonKind.SettingsNavLeftButton, flex.settingsNavnavLeftButton], + [ButtonKind.SettingsNavLeftButton, flex.settingsNavLeftButton],src/actions.ts (1)
16-17: Purely stylistic diff – no functional impactImports were switched to single quotes and kept type-safe. No action needed.
src/zondax.ts (1)
34-36: Defaults and fall-backs look correctThe newly formatted default click arrays and touch sequences remain intact and unambiguous. No behaviour change detected.
tests/minapp/index.ts (1)
25-33:Object.hasOwnrequires Node 20; verify runtime compatibility
Object.hasOwn()landed in Node 20. If your minimum supported runtime is < 20, fall back toObject.prototype.hasOwnProperty.call(obj, key).CLAUDE.md (1)
104-107: Node 22 is not LTS – highlight the implicationRequiring a non-LTS runtime may surprise CI users locked to the LTS channel.
Add a sentence explaining why 22+ is needed (e.g.StructuredClone/ top-level await) and how to useZEMU_NODE_VERSION_OVERRIDEif they must stay on 20..nvmrc (1)
1-1: Pinning to22may break contributors on LTSNode 22 is in current status until Oct 2025. Consider:
-22 +20 +# or use: +# lts/*and rely on
engines/CI to enforce 22 only where strictly needed..vscode/extensions.json (1)
1-3: 👍 Editor tooling recommendation looks good.vscode/settings.json (2)
4-7: Biome “quick-fix” keys likely incorrectVS Code expects either
source.fixAll.biomeorsource.organizeImports.biome, notquickfix.biome.
Please double-check the Biome extension docs – these keys may be silently ignored.- "quickfix.biome": "explicit", + "source.fixAll.biome": "explicit",
1-15: Config looks good overallConsistent formatter + on-save actions will help avoid style drift.
No further issues spotted.vitest.config.ts (1)
4-10: Sane default test settings60 s timeout, Node env and globals make sense for this repo. 👍
.mise.toml (2)
21-24: Ensurepnpm checkscript exists
tasks.checkcallspnpm check, but no nativepnpm checkcommand exists unless you defined a script namedcheckinpackage.json.
Please verify; otherwise the task will fail.
1-31: Nice use of mise tasksClear, self-documenting tasks aligned with CI pipeline.
tests/basic.s.test.ts (1)
1-168: Styling-only diff acknowledgedSwitch to single quotes & reordered imports only—logic untouched. Looks good.
.github/dependabot.yml (1)
1-25: Dependabot config otherwise looks solidDaily updates, grouped minor/patch bumps & CI actions separation are sensible.
tests/basic.x.test.ts (4)
3-3: LGTM - Modern Node.js import syntaxThe change to use
'node:path'import is a good practice for explicitly indicating Node.js built-in modules.
19-19: LGTM - Import statement formattingStylistic change to single quotes aligns with the project's code style standardization.
21-30: LGTM - Consistent variable declarations and formattingThe changes maintain functionality while improving code style consistency with single quotes and proper formatting.
32-89: LGTM - Test logic preserved with improved formattingAll test cases maintain their original logic while adopting the new code style. The migration to Vitest with
test.concurrentsyntax is appropriate and the test assertions remain intact.tests/globalsetup.ts (2)
1-1: LGTM - Import statement formattingStylistic change to single quotes aligns with project standards.
3-8: LGTM - Correctly removed async from synchronous functionThe removal of
asyncfromcatchExitis correct sinceZemu.stopAllEmuContainers()is a synchronous method according to the implementation insrc/Zemu.ts.package.json (4)
3-3: LGTM - Author organization updateThe change from "Zondax GmbH" to "Zondax AG" appears to be a legitimate organizational update.
21-24: LGTM - Modern engine requirementsThe engine requirements for Node.js >=22.0.0 and pnpm >=8.0.0 are appropriate for a modern project and align with the infrastructure modernization.
25-37: LGTM - Comprehensive tooling modernizationThe script updates reflect a well-planned migration:
- Yarn → pnpm for package management
- Jest → Vitest for testing
- ESLint/Prettier → Biome for linting/formatting
This modernization should improve performance and developer experience.
41-68: LGTM - Updated dependencies align with modernizationThe dependency updates are consistent with the infrastructure modernization and include security updates for packages like axios.
src/emulator.ts (4)
17-19: LGTM - Modern Node.js import syntaxThe change to explicit Node.js built-in module imports (
node:path,node:stream) is a best practice.
62-67: LGTM - Improved iteration patternThe change from
forEachtofor...ofloop is more readable and potentially more performant.
144-148: LGTM - Consistent iteration patternAnother good improvement from
forEachtofor...ofloop for better readability.
229-230: LGTM - Explicit undefined assignmentSetting
this.currentContainer = undefinedis clearer and more explicit than usingdelete, which is a good practice.src/types.ts (4)
16-27: LGTM - Consistent interface formattingThe removal of semicolons and consistent formatting improves readability while maintaining the same interface definitions.
42-42: LGTM - Consistent string literal formattingStandardizing to single quotes aligns with the project's code style.
63-63: LGTM - New container pooling optionThe addition of
disablePool?: booleansupports the new container pooling feature mentioned in the PR objectives.
85-132: LGTM - Explicit enum values improve maintainabilityAdding explicit numeric values to enums prevents issues with accidental reordering and makes the values clear and predictable. This is especially important for enums that might be serialized or used in external interfaces.
biome.json (1)
1-104: LGTM! Comprehensive Biome configuration.The configuration properly sets up linting, formatting, and code assistance rules. The migration from ESLint/Prettier to Biome is well-executed with appropriate rule mappings and settings.
src/Zemu.ts (1)
143-166: Well-implemented pool initialization with good error handling.The pool initialization method properly handles configuration, provides sensible defaults, and includes appropriate error recovery by disabling the pool on failure.
| "target": "es2022", | ||
| "module": "commonjs", | ||
| "moduleResolution": "node", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
target: "es2022" with module: "commonjs" blocks top-level await
ES2022 features such as top-level await only work under an ESModule loader.
If the codebase intends to rely on them (e.g. pool warm-up at module scope), switch to:
- "module": "commonjs",
+ "module": "es2022",or to "module": "nodenext" for seamless CJS/ESM interop.
📝 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.
| "target": "es2022", | |
| "module": "commonjs", | |
| "moduleResolution": "node", | |
| "target": "es2022", | |
| - "module": "commonjs", | |
| + "module": "es2022", | |
| "moduleResolution": "node", |
🤖 Prompt for AI Agents
In tsconfig.json around lines 3 to 5, the current setting "target": "es2022"
combined with "module": "commonjs" prevents using top-level await. To fix this,
change the "module" option to "es2022" or "nodenext" to enable ESModule loading
and support top-level await. Update the "module" field accordingly to allow
ES2022 features to work properly.
- Fix YAML linting issues in GitHub workflow files - Remove trailing spaces in ci-ts.yaml - Add missing newlines at end of YAML files - Fix typo: settingsNavnavLeftButton → settingsNavLeftButton - Organize imports in containerPool.ts (external packages first) - Remove unused model variable in cleanup() method - Apply automated code formatting improvements
- Fix missing parentheses in tests/minapp/index.ts toString() call - Add error handling to gRPC exchange promise chain - Remove async modifier from globalsetup.ts (no await needed) - Add race condition protection for pool initialization - Document DEV_CERT_PRIVATE_KEY as development-only test key
- Fix trailing spaces in .github/dependabot.yml - Add missing await to async operations in tests/pullImageKillOld.ts - Wrap async calls in IIFE to prevent CI hanging on cleanup This should resolve the hanging CI build and address the remaining CodeRabbit linting issues.
1 participant
Summary
This PR introduces a transparent container pooling system for Zemu that dramatically improves test execution performance by reusing Docker containers instead of creating new ones for each test.
Key Benefits
What's Changed
New Features
ContainerPoolclass for managing pre-warmed containersIntegration
Zemu.start()methodUsage
The pool is enabled by default. No code changes needed:
Opt-Out Options
If you need to disable the pool for any reason:
ZEMU_DISABLE_POOL=trueawait sim.start({ ...options, disablePool: true })Zemu.disablePool()Technical Details
Container Lifecycle
Port Management
Performance Impact
Testing
Breaking Changes
None. This is a performance enhancement that maintains full backwards compatibility.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation