Run STF tests using only one VM#798
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds multi-PVM support to the test-runner: introduces a SelectedPvm enum (Ananas, Builtin), a parseArgs(argv) CLI parser (using minimist) and selectedPvmToBackend mapping, propagates selected pvms through main → prepareTest → createTestDefinitions to filter variants, updates runStateTransition and various test-runner entry points to use the enum, replaces string variant literals with enum values, adds tests validating parseArgs, updates package.json to add minimist, and expands README with a "Selecting PVM Backend" section. No behavioral changes outside PVM selection propagation are introduced. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/test-runner/common.ts (1)
384-422: Refactor to eliminate type casts in variant filtering logic.The PVM filtering logic uses
as SelectedPvmcasts on lines 395 and 397, which violates the coding guideline stating "asconversions must not be used." Additionally, the comment on line 394 acknowledges the detection method is "a bit hacky."As per coding guidelines.
Option 1 (Recommended): Add a type guard function
function isPvmVariant<V>(variant: V): variant is V & SelectedPvm { return ALL_PVMS.includes(variant as SelectedPvm); } // Then use in createTestDefinitions: let possibleVariants: V[] = variants.length === 0 ? [noneVariant] : variants; if (possibleVariants.length > 0 && isPvmVariant(possibleVariants[0])) { possibleVariants = possibleVariants.filter(isPvmVariant) .filter((x) => globalOptions.pvms.includes(x)); }Option 2: Add a flag to the Runner type
Add a
usesPvmVariants: booleanfield to theRunnertype, set it explicitly when creating runners with PVM variants, and use that flag instead of the runtime type check. This would eliminate the need for runtime type detection entirely.
🧹 Nitpick comments (1)
bin/test-runner/common.test.ts (1)
1-41: Good baseline test coverage.The tests cover the main scenarios: specific PVM selection, default behavior, and error handling. The use of
deepEqualfor structured comparison is appropriate.Consider adding tests for comma-separated PVM values to ensure the implementation handles multiple selections correctly:
it("should parse comma-separated pvms", () => { const args = ["--pvm", "ananas,builtin", "file1.json"]; const result = parseArgs(args); deepEqual(result, { initialFiles: ["file1.json"], pvms: [SelectedPvm.Ananas, SelectedPvm.Builtin], }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
README.md(1 hunks)bin/test-runner/common.test.ts(1 hunks)bin/test-runner/common.ts(7 hunks)bin/test-runner/jam-conformance-070.ts(1 hunks)bin/test-runner/jam-conformance-071.ts(1 hunks)bin/test-runner/jamduna-067.ts(1 hunks)bin/test-runner/javajam.ts(1 hunks)bin/test-runner/package.json(1 hunks)bin/test-runner/state-transition/state-transition.ts(3 hunks)bin/test-runner/w3f-davxy-067.ts(1 hunks)bin/test-runner/w3f-davxy-070.ts(1 hunks)bin/test-runner/w3f-davxy-071.ts(1 hunks)bin/test-runner/w3f.ts(1 hunks)bin/test-runner/w3f/runners.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: rules from./CODESTYLE.mdshould be adhered to.
**/*.ts: Any function whose documentation mention it must not be used in production code,
can be safely used in*.test.tsfiles. Other usage should be carefuly reviewed
and the comment must explain why it's safe to use there.
**/*.ts:asconversions must not be used. Suggest usingtryAsconversion methods.
**/*.ts: Classes withstatic Codecfield must have private constructor and staticcreatemethod.
**/*.ts: Casting abigint(orU64) usingNumber(x)must have an explanation comment why
it is safe.
**/*.ts: When making changes to code with comments containing links (in classes, constants, methods, etc.)
to graypaper.fluffylabs.dev, ensure those links point to the current version for this update.
Files:
bin/test-runner/common.test.tsbin/test-runner/jam-conformance-070.tsbin/test-runner/jamduna-067.tsbin/test-runner/state-transition/state-transition.tsbin/test-runner/w3f/runners.tsbin/test-runner/w3f.tsbin/test-runner/w3f-davxy-067.tsbin/test-runner/w3f-davxy-070.tsbin/test-runner/jam-conformance-071.tsbin/test-runner/common.tsbin/test-runner/w3f-davxy-071.tsbin/test-runner/javajam.ts
🧠 Learnings (2)
📚 Learning: 2025-05-26T21:31:58.688Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 399
File: .github/workflows/vectors-jamduna.yml:78-78
Timestamp: 2025-05-26T21:31:58.688Z
Learning: In the typeberry project, the npm start script in the test-runner workspace is designed to accept test suite names as arguments (e.g., "jamduna", "w3f"). The command `npm start -w typeberry/test-runner jamduna` is the correct way to run the jamduna test suite, not `npm run jamduna`. This is an architectural design choice where the start script acts as a unified entry point for different test suites.
Applied to files:
bin/test-runner/common.test.tsbin/test-runner/jam-conformance-070.tsbin/test-runner/jamduna-067.tsREADME.mdbin/test-runner/jam-conformance-071.tsbin/test-runner/common.tsbin/test-runner/javajam.tsbin/test-runner/package.json
📚 Learning: 2025-06-18T20:35:13.536Z
Learnt from: tomusdrw
Repo: FluffyLabs/typeberry PR: 442
File: packages/core/pvm-debugger-adapter/index.ts:22-40
Timestamp: 2025-06-18T20:35:13.536Z
Learning: The `typeberry/utils` package has browser compatibility issues due to Node.js-specific code like `measure` function using `process.hrtime()` and `testUtils` importing `node:assert`, causing white screens in browser environments.
Applied to files:
bin/test-runner/package.json
🧬 Code graph analysis (11)
bin/test-runner/common.test.ts (2)
bin/test-runner/common.ts (1)
parseArgs(137-166)packages/core/utils/test.ts (1)
deepEqual(43-195)
bin/test-runner/jam-conformance-070.ts (2)
bin/test-runner/common.ts (2)
main(168-290)parseArgs(137-166)bin/test-runner/w3f/runners.ts (1)
runners(54-78)
bin/test-runner/jamduna-067.ts (1)
bin/test-runner/common.ts (3)
runner(86-92)main(168-290)parseArgs(137-166)
bin/test-runner/state-transition/state-transition.ts (2)
packages/jam/state-vectors/index.ts (1)
StateTransition(53-69)bin/test-runner/common.ts (2)
RunOptions(94-98)selectedPvmToBackend(24-33)
bin/test-runner/w3f.ts (1)
bin/test-runner/common.ts (2)
main(168-290)parseArgs(137-166)
bin/test-runner/w3f-davxy-067.ts (1)
bin/test-runner/common.ts (2)
main(168-290)parseArgs(137-166)
bin/test-runner/w3f-davxy-070.ts (2)
bin/test-runner/common.ts (2)
main(168-290)parseArgs(137-166)bin/test-runner/w3f/runners.ts (1)
runners(54-78)
bin/test-runner/jam-conformance-071.ts (1)
bin/test-runner/common.ts (2)
main(168-290)parseArgs(137-166)
bin/test-runner/common.ts (3)
packages/core/utils/debug.ts (1)
assertNever(36-38)bin/jam/args.ts (1)
parseArgs(114-190)bin/test-runner/w3f/runners.ts (1)
runners(54-78)
bin/test-runner/w3f-davxy-071.ts (1)
bin/test-runner/common.ts (2)
main(168-290)parseArgs(137-166)
bin/test-runner/javajam.ts (1)
bin/test-runner/common.ts (3)
runner(86-92)main(168-290)parseArgs(137-166)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: run (22.x)
- GitHub Check: e2e (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: test (22.x)
🔇 Additional comments (15)
README.md (1)
171-189: LGTM! Clear documentation for the new feature.The documentation clearly explains the new
--pvmoption and provides helpful examples. The note about its use cases (debugging and performance) adds valuable context.bin/test-runner/jam-conformance-071.ts (1)
1-6: LGTM! Consistent refactoring to use parseArgs.The changes properly integrate CLI argument parsing through the new
parseArgshelper, spreading its results into the main options object.bin/test-runner/w3f-davxy-067.ts (1)
1-5: LGTM! Consistent refactoring pattern.Follows the same approach as other test runner files, properly integrating the new
parseArgsfunctionality.bin/test-runner/w3f-davxy-070.ts (1)
1-5: LGTM! Clean and consistent.The refactoring follows the established pattern across all test runner entry points.
bin/test-runner/w3f/runners.ts (1)
16-49: LGTM! Improved type safety with enum values.The change from string literals to the
SelectedPvmenum provides better type safety and consistency across the codebase.bin/test-runner/jam-conformance-070.ts (1)
1-5: LGTM! Final entry point updated consistently.All test runner entry points now follow the same pattern, providing a uniform interface for PVM selection.
bin/test-runner/package.json (1)
34-34: minimist "^1.2.8" is current and secure.The latest version is 1.2.8, and all known security vulnerabilities have been patched. The CRITICAL Prototype Pollution vulnerability affecting versions >= 1.0.0, < 1.2.6 was fixed in 1.2.6, so version 1.2.8 is not affected. Other reported advisories target older version ranges that are also not applicable to 1.2.8.
bin/test-runner/javajam.ts (2)
2-2: LGTM! Type-safe enum usage.The transition from string literals to
SelectedPvmenum values improves type safety and aligns with the PR's goal of adding PVM selection support.Also applies to: 9-9
12-13: LGTM! Proper integration of CLI argument parsing.The refactored
maininvocation correctly spreads parsed arguments into the options object, enabling PVM selection via command-line arguments.bin/test-runner/w3f.ts (1)
1-1: LGTM! Consistent CLI argument parsing integration.The changes properly integrate
parseArgsto enable command-line argument support, maintaining consistency with other test runner entry points.Also applies to: 4-5
bin/test-runner/w3f-davxy-071.ts (1)
1-1: LGTM! Consistent refactoring.The integration of
parseArgsfollows the established pattern and correctly merges CLI-derived options into the main invocation.Also applies to: 4-5
bin/test-runner/jamduna-067.ts (1)
2-2: LGTM! Consistent enum usage and CLI parsing integration.The changes align with the refactoring pattern: type-safe
SelectedPvmenum replaces string literals, andparseArgsenables command-line PVM selection.Also applies to: 9-9, 12-13
bin/test-runner/state-transition/state-transition.ts (1)
16-16: LGTM! Clean refactoring with improved type safety.The transition from string union types to
SelectedPvmenum and the use ofselectedPvmToBackendmapping function improves type safety and centralizes PVM backend selection logic.Also applies to: 68-69
bin/test-runner/common.ts (2)
14-14: LGTM! Well-structured PVM infrastructure.The
SelectedPvmenum,ALL_PVMSconstant, andselectedPvmToBackendmapping function provide a clean, type-safe foundation for PVM selection. The exhaustive switch withassertNeverensures compile-time completeness checking.Also applies to: 19-33
168-236: LGTM! Well-integrated signature updates.The updated
mainfunction signature properly acceptsinitialFilesandpvmsas required parameters, enabling the PVM selection feature. The parameters are correctly propagated through the test preparation pipeline toprepareTest.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
View all
Benchmarks summary: 83/83 OK ✅ |
2 participants
Currently all test vectors are always run twice - once in bulit-in vm and second time in ananas. This is a bit annoying when debugging, hence this PR adds a CLI option to easily select only one of the VMs.