Refactor test runner#744
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces the previous test-runner API with a generic RunnerBuilder and a fluent runner<T,V>() builder that accumulates parsers, variants, and chainSpecs and builds Runner instances. Introduces RunOptions and updates many run* helpers to accept RunOptions (containing path and chainSpec) plus a variant discriminator (e.g., "ananas" | "builtin"); variant selects PvmBackend and is threaded into OnChain/runner calls. Test discovery now supports extension-based patterns and per-variant test creation/labeling. Multiple w3f modules were migrated to the builder/RunOptions style, several host-call/spec modules were removed, ChainSpec gained a name field, and a few type-branding constants were converted to unique symbols. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 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: 2
🧹 Nitpick comments (2)
bin/test-runner/common.ts (2)
15-22: Avoid ‘as’ assertion; return the generic type directly.The coding guidelines disallow
asconversions. Preserve generics on the return instead of asserting toRunner<unknown, unknown>.As per coding guidelines
Apply this diff:-export function runner<T, V = string>( +export function runner<T, V = string>( name: string, fromJson: FromJson<T>, - run: (test: T, path: string, t: TestContext, variant: V | null) => Promise<void>, - variants: V[] = [], -): Runner<unknown, unknown> { - return { name, fromJson, run, variants } as Runner<unknown, unknown>; + run: (test: T, path: string, t: TestContext, variant: V | null) => Promise<void>, + variants: V[] = [], +): Runner<T, V> { + return { name, fromJson, run, variants }; }Note:
maincan still acceptRunner<unknown, unknown>[]; structural typing will allow arrays built by this function.
105-110: Readable test names per variant.The
[variant] fileformat is clear. Consider normalizing variant to string if future variants aren’t strings, but fine for "ananas"/"builtin".
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
bin/test-runner/common.ts(4 hunks)bin/test-runner/state-transition/state-transition.ts(3 hunks)bin/test-runner/w3f.ts(1 hunks)bin/test-runner/w3f/accumulate.ts(2 hunks)bin/test-runner/w3f/runners.ts(3 hunks)packages/core/numbers/index.ts(1 hunks)packages/core/pvm-debugger-adapter/debugger-adapter.ts(0 hunks)packages/core/utils/opaque.ts(1 hunks)packages/jam/config-node/node-config.ts(1 hunks)packages/jam/jam-host-calls/externalities/partial-state.ts(0 hunks)packages/jam/safrole/bandersnatch-vrf.ts(1 hunks)packages/jam/state-merkleization/serialize-state-update.ts(0 hunks)packages/jam/transition/accumulate/accumulate.ts(1 hunks)packages/jam/transition/chain-stf.ts(2 hunks)packages/jam/transition/hasher.ts(1 hunks)packages/jam/transition/preimages.ts(0 hunks)
💤 Files with no reviewable changes (4)
- packages/jam/transition/preimages.ts
- packages/jam/state-merkleization/serialize-state-update.ts
- packages/core/pvm-debugger-adapter/debugger-adapter.ts
- packages/jam/jam-host-calls/externalities/partial-state.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
packages/jam/safrole/bandersnatch-vrf.tspackages/jam/config-node/node-config.tspackages/jam/transition/accumulate/accumulate.tspackages/jam/transition/hasher.tsbin/test-runner/w3f.tsbin/test-runner/w3f/accumulate.tsbin/test-runner/state-transition/state-transition.tspackages/jam/transition/chain-stf.tspackages/core/numbers/index.tspackages/core/utils/opaque.tsbin/test-runner/common.tsbin/test-runner/w3f/runners.ts
packages/core/**/*.ts
⚙️ CodeRabbit configuration file
packages/core/**/*.ts: Core packages must not import any JAM-related packages
(i.e. packages defined underpackages/jam/**)
Files:
packages/core/numbers/index.tspackages/core/utils/opaque.ts
🧬 Code graph analysis (5)
packages/jam/transition/accumulate/accumulate.ts (1)
packages/jam/node/common.ts (1)
logger(19-19)
bin/test-runner/state-transition/state-transition.ts (1)
packages/jam/transition/chain-stf.ts (1)
OnChain(117-440)
packages/jam/transition/chain-stf.ts (1)
packages/core/utils/debug.ts (1)
measure(112-124)
bin/test-runner/common.ts (3)
packages/core/json-parser/types.ts (1)
FromJson(3-12)bin/test-runner/w3f/runners.ts (1)
runners(51-76)packages/core/json-parser/parse.ts (1)
parseFromJson(6-140)
bin/test-runner/w3f/runners.ts (3)
bin/test-runner/common.ts (1)
runner(15-22)bin/test-runner/w3f/accumulate.ts (2)
AccumulateTest(131-143)runAccumulateTest(145-176)bin/test-runner/state-transition/state-transition.ts (2)
StateTransition(35-51)runStateTransition(102-179)
⏰ 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). (5)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: benchmarks (22.x)
- GitHub Check: test (22.x)
🔇 Additional comments (18)
packages/jam/safrole/bandersnatch-vrf.ts (1)
36-38: Comment update aligns with PR cleanup objectives.The change from
TODOtoNOTEappropriately reflects that this export pattern is intentional rather than a pending task. The comment clearly explains the rationale (test mocking in ESM context), so marking it asNOTEis the right call.packages/jam/config-node/node-config.ts (1)
22-22: LGTM! Default PVM backend switched to Ananas.The change aligns with the PR objectives and is consistent with the broader updates across the codebase to support explicit PVM backend selection.
packages/jam/transition/accumulate/accumulate.ts (1)
238-238: LGTM! Minor log formatting cleanup.Removing the trailing period improves consistency.
packages/core/utils/opaque.ts (1)
24-24: LGTM! Improved type branding with unique symbol.Switching from a string literal to a
unique symbolstrengthens type-level branding without any runtime impact. This aligns with TypeScript best practices for nominal typing.packages/core/numbers/index.ts (1)
3-3: LGTM! Type branding improvement consistent with broader pattern.This change aligns with the
unique symbolbranding approach used inpackages/core/utils/opaque.ts, improving type safety at compile time without runtime impact.bin/test-runner/w3f.ts (1)
6-9: LGTM! More explicit ignore list with documentation.Replacing the generic
"traces/"pattern with specific file paths and adding an explanatory comment improves maintainability.bin/test-runner/w3f/runners.ts (2)
52-52: LGTM! Enabling multi-backend testing for accumulate.Adding the variants array
["ananas", "builtin"]enables the accumulate tests to run with both PVM backends, aligning with the PR objectives.
75-75: LGTM! Enabling multi-backend testing for traces.Adding the variants array
["ananas", "builtin"]enables the state transition tests to run with both PVM backends, aligning with the PR objectives.packages/jam/transition/chain-stf.ts (2)
305-305: LGTM! Enhanced logging with backend information.Including the PVM backend name in the log message improves debugging and traceability when running tests with different backends.
148-148: All OnChain constructor calls properly pass the pvm parameter.Verification confirms that both OnChain instantiations in the codebase pass the
pvmparameter explicitly:
packages/workers/importer/importer.ts:52bin/test-runner/state-transition/state-transition.ts:139The constructor signature shows
pvm: PvmBackend,with no default value, confirming it is required. The removal of the default has been properly propagated to all call sites.packages/jam/transition/hasher.ts (1)
1-64: No issues found—removed methods are not referenced elsewhere.The search confirms that the
workPackage()method andencodehelper removed fromTransitionHasherare not called anywhere in the codebase. The methods namedworkPackage()found in the results belong to other classes (Fetch,FetchExternalities) and are unrelated to the removal.bin/test-runner/state-transition/state-transition.ts (2)
8-8: Import of PvmBackend looks correct.Matches the new OnChain constructor and variant plumbing.
139-139: OnChain now receives pvm — good propagation.This matches packages/jam/transition/chain-stf.ts constructor.
bin/test-runner/w3f/accumulate.ts (1)
2-2: Type-only TestContext import OK.Underscored parameter avoids unused warnings.
bin/test-runner/common.ts (4)
24-29: Runner type extension LGTM.The
variant: V | nullin the run signature andvariants: V[]field are consistent with per‑variant execution.
63-70: Per‑variant test expansion wiring looks good.prepareTest returns multiple TestAndRunner entries and carry shouldSkip per entry; matches the new runner API.
141-172: Variant expansion logic is sound.
- Uses first matching runner.
- Parses once, maps across variants.
- Passes variant through to run callback.
Minor:
options = variants.length > 0 ? variants : [null]is clear and avoids branching elsewhere.
177-191: Invalid-case reporting remains helpful.Aggregating parse errors per runner then failing the test is preserved for multi‑variant context.
7c4c24b to
3dc18bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bin/test-runner/w3f/accumulate.ts (1)
144-149: Default to Ananas variant (and guard invalid inputs)To make direct invocations safe outside the builder, default variant to "ananas" and map "builtin" explicitly; optionally guard unexpected values at runtime.
-export async function runAccumulateTest( - test: AccumulateTest, - { chainSpec }: RunOptions, - variant: "ananas" | "builtin", -) { - const pvm = variant === "ananas" ? PvmBackend.Ananas : PvmBackend.BuiltIn; +export async function runAccumulateTest( + test: AccumulateTest, + { chainSpec }: RunOptions, + variant: "ananas" | "builtin" = "ananas", +) { + const pvm = variant === "builtin" ? PvmBackend.BuiltIn : PvmBackend.Ananas; + // Optional: if called from JS without TS checks + // if (variant !== "ananas" && variant !== "builtin") throw new Error(`Unsupported PVM variant: ${variant}`);
🧹 Nitpick comments (5)
bin/test-runner/common.ts (1)
204-204: Consider formatting for empty variant strings.When
runner.variantis an empty string (for tests without variants), the test case name will have an empty bracket prefix:[] filename. Consider conditional formatting.Apply this diff to improve test name formatting:
-const testCase = `${runner.variant}` !== "" ? `[${runner.variant}] ${fileName}` : fileName; +const testCase = runner.variant ? `[${runner.variant}] ${fileName}` : fileName;bin/test-runner/javajam.ts (1)
4-9: Builder usage + per-variant traces: LGTMConfiguration is correct. Optional: centralize the variant list (e.g., export from common) to avoid drifting literals across files.
bin/test-runner/state-transition/state-transition.ts (1)
112-115: Avoid hardcoded path magic (optional)The special-case path check is brittle. Consider extracting to a small allowlist/constant or moving into a helper to keep the rule centralized.
-const spec = options.path.includes("fuzz-reports/0.7.0/traces/1756548916/00000082.json") - ? jamConformance070V0Spec - : options.chainSpec; +const SPECIAL_CASES = new Set<string>([ + "fuzz-reports/0.7.0/traces/1756548916/00000082.json", +]); +const spec = [...SPECIAL_CASES].some((p) => options.path.includes(p)) + ? jamConformance070V0Spec + : options.chainSpec;bin/test-runner/w3f/assurances.ts (1)
169-175: Optional: reduce wrapper duplicationBoth Tiny/Full wrappers duplicate the same pattern. Consider a small helper to DRY them.
-export async function runAssurancesTestTiny(testContent: AssurancesTestTiny, options: RunOptions) { - const spec = options.chainSpec; - const preState = TestState.toAssurancesState(testContent.pre_state, spec); - const postState = TestState.toAssurancesState(testContent.post_state, spec); - const input = Input.toAssurancesInput(testContent.input, spec, preState.availabilityAssignment); - const expectedResult = Output.toAssurancesTransitionResult(testContent.output); - await runAssurancesTest(options, preState, postState, input, expectedResult); -} +function buildAssurancesRun<T extends AssurancesTestTiny | AssurancesTestFull>() { + return async (testContent: T, options: RunOptions) => { + const spec = options.chainSpec; + const preState = TestState.toAssurancesState(testContent.pre_state, spec); + const postState = TestState.toAssurancesState(testContent.post_state, spec); + const input = Input.toAssurancesInput(testContent.input, spec, preState.availabilityAssignment); + const expectedResult = Output.toAssurancesTransitionResult(testContent.output); + await runAssurancesTest(options, preState, postState, input, expectedResult); + }; +} +export const runAssurancesTestTiny = buildAssurancesRun<AssurancesTestTiny>(); +export const runAssurancesTestFull = buildAssurancesRun<AssurancesTestFull>();bin/test-runner/w3f/codec/index.ts (1)
14-56: Consider passingchainSpecfromRunOptionstorunCodecTest.All functions now receive
RunOptionswhich includes achainSpecfield, but only thepathis extracted and used. The underlyingrunCodecTestfunction still derives the chain spec by callinggetChainSpec(file)internally. This redundant derivation is inefficient and inconsistent with the refactoring goals.Consider updating
runCodecTestinbin/test-runner/w3f/codec/common.tsto acceptchainSpecas a parameter:-export function runCodecTest<T>(codec: Codec<T>, test: T, file: string) { - const spec = getChainSpec(file); +export function runCodecTest<T>(codec: Codec<T>, test: T, file: string, spec: ChainSpec) { const encoded = new Uint8Array(fs.readFileSync(file.replace("json", "bin"))); const myEncoded = Encoder.encodeObject(codec, test, spec); assert.deepStrictEqual(myEncoded.toString(), BytesBlob.blobFrom(encoded).toString()); const decoded = Decoder.decodeObject(codec, BytesBlob.blobFrom(encoded), spec); assert.deepStrictEqual(decoded, test); }Then update all call sites in this file to pass
chainSpec:-export async function runAssurancesExtrinsicTest(test: AssurancesExtrinsic, { path: file }: RunOptions) { - runCodecTest(assurancesExtrinsicCodec, test, file); +export async function runAssurancesExtrinsicTest(test: AssurancesExtrinsic, { path: file, chainSpec }: RunOptions) { + runCodecTest(assurancesExtrinsicCodec, test, file, chainSpec); }Apply similar changes to the remaining 10 functions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
bin/test-runner/common.ts(4 hunks)bin/test-runner/jamduna-067.ts(1 hunks)bin/test-runner/javajam.ts(1 hunks)bin/test-runner/state-transition/state-transition.ts(4 hunks)bin/test-runner/w3f.ts(1 hunks)bin/test-runner/w3f/accumulate.ts(3 hunks)bin/test-runner/w3f/assurances.ts(2 hunks)bin/test-runner/w3f/authorizations.ts(2 hunks)bin/test-runner/w3f/codec/index.ts(1 hunks)bin/test-runner/w3f/codec/work-item.ts(2 hunks)bin/test-runner/w3f/codec/work-package.ts(2 hunks)bin/test-runner/w3f/disputes.ts(2 hunks)bin/test-runner/w3f/erasure-coding.ts(2 hunks)bin/test-runner/w3f/host-calls-accumulate.ts(0 hunks)bin/test-runner/w3f/host-calls-general.ts(0 hunks)bin/test-runner/w3f/host-calls-refine.ts(0 hunks)bin/test-runner/w3f/reports.ts(3 hunks)bin/test-runner/w3f/runners.ts(1 hunks)bin/test-runner/w3f/safrole.ts(2 hunks)bin/test-runner/w3f/shuffling.ts(1 hunks)bin/test-runner/w3f/spec.ts(0 hunks)packages/core/numbers/index.ts(1 hunks)packages/core/pvm-debugger-adapter/debugger-adapter.ts(0 hunks)packages/core/utils/opaque.ts(1 hunks)packages/jam/config-node/node-config.ts(1 hunks)packages/jam/config/chain-spec.ts(4 hunks)packages/jam/jam-host-calls/externalities/partial-state.ts(0 hunks)packages/jam/safrole/bandersnatch-vrf.ts(1 hunks)packages/jam/state-merkleization/serialize-state-update.ts(0 hunks)packages/jam/state/in-memory-state.ts(2 hunks)packages/jam/transition/accumulate/accumulate.ts(1 hunks)packages/jam/transition/chain-stf.ts(2 hunks)packages/jam/transition/externalities/accumulate-externalities.test.ts(2 hunks)packages/jam/transition/externalities/accumulate-externalities.ts(1 hunks)packages/jam/transition/hasher.ts(1 hunks)packages/jam/transition/preimages.ts(0 hunks)
💤 Files with no reviewable changes (8)
- packages/jam/jam-host-calls/externalities/partial-state.ts
- packages/jam/state-merkleization/serialize-state-update.ts
- bin/test-runner/w3f/host-calls-general.ts
- packages/core/pvm-debugger-adapter/debugger-adapter.ts
- packages/jam/transition/preimages.ts
- bin/test-runner/w3f/spec.ts
- bin/test-runner/w3f/host-calls-accumulate.ts
- bin/test-runner/w3f/host-calls-refine.ts
✅ Files skipped from review due to trivial changes (1)
- packages/jam/transition/externalities/accumulate-externalities.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jam/transition/accumulate/accumulate.ts
- packages/core/utils/opaque.ts
- packages/core/numbers/index.ts
- packages/jam/safrole/bandersnatch-vrf.ts
🧰 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/w3f.tspackages/jam/config/chain-spec.tsbin/test-runner/w3f/shuffling.tspackages/jam/transition/hasher.tsbin/test-runner/w3f/codec/work-package.tsbin/test-runner/w3f/authorizations.tspackages/jam/transition/externalities/accumulate-externalities.tsbin/test-runner/w3f/disputes.tsbin/test-runner/w3f/safrole.tsbin/test-runner/w3f/erasure-coding.tsbin/test-runner/w3f/reports.tsbin/test-runner/w3f/assurances.tsbin/test-runner/w3f/codec/work-item.tsbin/test-runner/javajam.tsbin/test-runner/state-transition/state-transition.tsbin/test-runner/jamduna-067.tspackages/jam/state/in-memory-state.tsbin/test-runner/w3f/runners.tsbin/test-runner/w3f/accumulate.tspackages/jam/config-node/node-config.tsbin/test-runner/common.tsbin/test-runner/w3f/codec/index.tspackages/jam/transition/chain-stf.ts
🧠 Learnings (1)
📚 Learning: 2025-06-10T12:20:17.513Z
Learnt from: tomusdrw
PR: FluffyLabs/typeberry#419
File: packages/jam/state/state-inmemory.ts:141-149
Timestamp: 2025-06-10T12:20:17.513Z
Learning: In the `InMemoryService.copyFrom` function in `packages/jam/state/state-inmemory.ts`, duplicate checking for `(hash, length)` pairs in the lookup history is not necessary because the function operates under the assumption that the input `ServiceEntries` comes from an existing well-formed state, which already maintains the invariant of unique lookup history entries per hash and length combination.
Applied to files:
packages/jam/state/in-memory-state.ts
🧬 Code graph analysis (18)
bin/test-runner/w3f/shuffling.ts (1)
bin/test-runner/common.ts (2)
json(89-89)json(90-90)
bin/test-runner/w3f/codec/work-package.ts (2)
packages/jam/block/work-package.ts (1)
WorkPackage(37-94)bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/w3f/authorizations.ts (1)
bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/w3f/disputes.ts (1)
bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/w3f/safrole.ts (1)
bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/w3f/erasure-coding.ts (1)
bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/w3f/reports.ts (1)
bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/w3f/assurances.ts (1)
bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/w3f/codec/work-item.ts (2)
bin/test-runner/common.ts (1)
RunOptions(72-76)bin/test-runner/w3f/codec/common.ts (1)
runCodecTest(7-15)
bin/test-runner/javajam.ts (3)
bin/test-runner/w3f/runners.ts (1)
runners(53-77)bin/test-runner/common.ts (1)
runner(64-70)bin/test-runner/state-transition/state-transition.ts (2)
runStateTransition(102-178)StateTransition(35-51)
bin/test-runner/state-transition/state-transition.ts (3)
bin/test-runner/common.ts (1)
RunOptions(72-76)packages/core/hash/blake2b.ts (1)
Blake2b(8-44)bin/test-runner/state-transition/state-loader.ts (1)
loadState(39-45)
bin/test-runner/jamduna-067.ts (3)
bin/test-runner/w3f/runners.ts (1)
runners(53-77)bin/test-runner/common.ts (1)
runner(64-70)bin/test-runner/state-transition/state-transition.ts (2)
runStateTransition(102-178)StateTransition(35-51)
packages/jam/state/in-memory-state.ts (2)
packages/jam/state/service.ts (1)
ServiceAccountInfo(82-154)packages/core/collections/hash-dictionary.ts (1)
HashDictionary(24-102)
bin/test-runner/w3f/runners.ts (7)
packages/jam/config/chain-spec.ts (2)
tinyChainSpec(109-126)fullChainSpec(132-147)bin/test-runner/common.ts (1)
runner(64-70)bin/test-runner/w3f/accumulate.ts (2)
runAccumulateTest(144-172)AccumulateTest(130-142)bin/test-runner/w3f/reports.ts (2)
runReportsTest(251-284)ReportsTest(238-249)bin/test-runner/w3f/shuffling.ts (2)
runShufflingTests(25-36)shufflingTestsFromJson(23-23)bin/test-runner/w3f/trie.ts (2)
runTrieTest(39-60)trieTestSuiteFromJson(37-37)bin/test-runner/w3f/codec/index.ts (4)
runAssurancesExtrinsicTest(14-16)runBlockTest(18-20)runDisputesExtrinsicTest(22-24)runExtrinsicTest(26-28)
bin/test-runner/w3f/accumulate.ts (1)
bin/test-runner/common.ts (1)
RunOptions(72-76)
bin/test-runner/common.ts (3)
packages/core/json-parser/types.ts (1)
FromJson(3-12)packages/core/codec/decoder.ts (2)
Decode(6-9)Decoder(12-379)packages/core/json-parser/parse.ts (1)
parseFromJson(6-140)
bin/test-runner/w3f/codec/index.ts (2)
bin/test-runner/common.ts (1)
RunOptions(72-76)bin/test-runner/w3f/codec/common.ts (1)
runCodecTest(7-15)
packages/jam/transition/chain-stf.ts (1)
packages/core/utils/debug.ts (1)
measure(112-124)
⏰ 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). (4)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: test (22.x)
🔇 Additional comments (34)
packages/jam/transition/externalities/accumulate-externalities.ts (1)
154-154: LGTM! Enhanced diagnostic message improves debuggability.The improved error message now includes the actual preimage status value, making it easier to diagnose why
ejectoperations fail when the previous code is not in the expectedUnavailablestate.packages/jam/transition/chain-stf.ts (2)
5-5: LGTM! Import change necessary for runtime enum access.The change from a type-only import to a mixed import (value for
PvmBackend, type forChainSpec) is correct and required to support the runtime enum indexing on line 305 wherePvmBackend[this.accumulate.pvm]is used to label the performance metric.
305-305: Excellent observability improvement!Adding the PVM backend name to the timer label enables differentiation of performance metrics when running test vectors with both Ananas and Builtin backends. This directly supports the PR objectives and will provide valuable performance comparison data.
packages/jam/config-node/node-config.ts (1)
22-22: Change verified: Ananas backend is production-ready and properly tested.The default PVM backend switch is supported by comprehensive test coverage. The codebase already parametrizes tests across both backends (accumulate.test.ts), and both BuiltIn and Ananas interpreters are fully implemented. Tests relying on NODE_DEFAULTS will use Ananas, but existing parametric tests confirm compatibility.
packages/jam/transition/hasher.ts (1)
3-5: The review comment is based on incorrect assumptions.The analysis found that:
TransitionHasherclass inpackages/jam/transition/hasher.tsdoes not have aworkPackage()method and never did. The class contains only:hashConcat(),hashConcatPrepend(),header(), andextrinsic().The
workPackage()method exists in theIFetchExternalitiesinterface (inpackages/jam/jam-host-calls/fetch.ts), which is completely unrelated to the import changes inhasher.ts.The call at
packages/jam/jam-host-calls/fetch.ts:320(return this.fetch.workPackage()) invokes the interface method on anIFetchExternalitiesobject, not onTransitionHasher. This method remains unchanged.The import modifications at lines 3-5 in
hasher.tsare clean and correct—all imported symbols are actually used in the file.There is no breaking change here, and no verification of method removal is needed.
Likely an incorrect or invalid review comment.
packages/jam/config/chain-spec.ts (1)
42-43: LGTM!The addition of the
namefield toChainSpecis implemented consistently across the class definition, constructor initialization, and both predefined instances. This enables explicit identification of chain specs at runtime, which aligns with the PR's goal of supporting per-variant test wiring.Also applies to: 88-88, 110-110, 133-133
bin/test-runner/w3f.ts (1)
6-9: LGTM!Replacing the broad "traces/" ignore pattern with explicit file-level ignores is more precise and maintainable. The explanatory comment clearly documents why these specific files are being ignored.
packages/jam/state/in-memory-state.ts (1)
48-48: LGTM!The new
clone()method provides proper deep-copy semantics forInMemoryService. All nested collections (preimages, lookupHistory, storage) are correctly cloned, and the import change from type-only to value import forServiceAccountInfois necessary to useServiceAccountInfo.create(). This aligns with the PR's test infrastructure needs for isolating state between test runs.Also applies to: 132-142
bin/test-runner/common.ts (5)
17-70: LGTM! Well-structured builder pattern.The new
RunnerBuilderclass andrunner()factory function provide a clean, type-safe API for configuring test runners with parsers, variants, and chain specs. The builder pattern enables flexible composition and clear configuration.
72-86: LGTM! RunOptions standardizes test parameter passing.The
RunOptionstype andRunFunctionsignature provide a consistent interface across all test runners, replacing scattered string path parameters with a structured object containing test context, chain spec, and path.
88-113: LGTM! testFile namespace provides type-safe file handling.The namespace clearly defines supported file types (json/bin) with corresponding parsers and content types, enabling type-safe parser selection.
248-250: Verify type safety of noneVariant assignment.The type assertion
"" as Vmay be unsafe if variant typeVhas specific constraints. While the comment acknowledges this, consider whether a sentinel type or explicitV | undefinedwould be safer.Consider this safer alternative:
// Make variants explicitly optional in the type system const possibleVariants: (V | undefined)[] = variants.length === 0 ? [undefined] : variants;Or define a branded empty variant type if string literals are required.
294-308: Verify error handling for multi-variant scenarios.The error path returns a single
TestAndRunnerentry with runner name "Invalid", even when multiple variants might be expected. This could mask which variant(s) failed to parse.Consider whether variant-specific error entries would provide better diagnostics:
// Return one error entry per expected variant for clarity return possibleVariants.map(variant => ({ shouldSkip: false, runner: "Invalid", file: fileName, variant, test: () => { /* error logging */ } }));bin/test-runner/w3f/codec/work-package.ts (1)
10-10: LGTM!The migration to
RunOptionsis implemented correctly, using destructuring with alias{ path: file }to maintain the existing variable name. This aligns with the broader test runner API standardization in the PR.Also applies to: 80-81
bin/test-runner/w3f/codec/work-item.ts (1)
7-7: LGTM!The migration to
RunOptionsis implemented correctly with direct destructuring of thepathproperty. This aligns with the broader test runner API standardization in the PR.Also applies to: 61-62
bin/test-runner/w3f/erasure-coding.ts (1)
18-18: LGTM!The migration to
RunOptionswith directchainSpecdestructuring is cleaner than the previous path-based chain spec derivation. This aligns with the broader test runner API standardization in the PR.Also applies to: 30-30
bin/test-runner/w3f/shuffling.ts (1)
23-23: All import sites have been successfully updated.The verification confirms:
- Export in
shuffling.tsline 23 correctly uses the new nameshufflingTestsFromJson- No remaining references to the old name
shufflingTestsexist in the codebase- Import site in
runners.tsline 44 properly uses the new nameThe rename is complete and consistent across the codebase.
bin/test-runner/w3f/disputes.ts (1)
14-14: RunOptions injection looks goodSignature/import change aligns with the new runner API; chainSpec usage remains correct.
Also applies to: 91-91
bin/test-runner/w3f/authorizations.ts (1)
13-13: Consistent shift to RunOptionsClean swap from path-based spec to injected chainSpec. No behavioral regressions spotted.
Also applies to: 80-86
bin/test-runner/w3f/safrole.ts (1)
33-33: Good migration to RunOptionsSignature/import update matches the builder pattern; chainSpec flows correctly through Safrole setup.
Also applies to: 214-214
bin/test-runner/w3f/accumulate.ts (1)
114-114: Cloning services prevents shared-state bleedUsing service.clone() when building the services map avoids cross-test mutation. Good call for test isolation.
bin/test-runner/w3f/reports.ts (2)
12-12: Type-only imports LGTMType-only usage for ChainSpec/RunOptions is correct and keeps value bundle clean.
Also applies to: 46-46
251-284: RunOptions wiring: good migrationSignature switch to RunOptions and spec extraction is consistent with runners. No issues found.
bin/test-runner/state-transition/state-transition.ts (5)
17-17: Import addition LGTMRunOptions import is appropriate for the new API.
98-100: ChainSpec name initializationAdding a name to jamConformance070V0Spec is fine and helpful for diagnostics.
102-108: Variant-to-PVM mapping looks correctExplicit variant param and mapping to PvmBackend is clear and matches the builder’s withVariants setup.
123-123: Using options.path for block discoverySwitch to options.path is consistent with the new RunOptions flow.
138-139: Pass PVM into OnChainConstructor update to include pvm is correct and matches downstream usage.
bin/test-runner/w3f/assurances.ts (2)
148-156: Tiny: RunOptions migration LGTMSpec pulled from options; wrapper is correct.
158-166: Full: RunOptions migration LGTMSame as Tiny; consistent and correct.
bin/test-runner/w3f/runners.ts (3)
48-52: Helper constants LGTMpvms/tiny/full/tinyFull helpers improve readability and reuse.
54-77: Runners array migration LGTMBuilder pattern with withVariants for PVM-dependent runners ensures both Ananas and Builtin are exercised. Ordering (Ananas first) aligns with “default Ananas” objective.
82-101: codecRunners: spec-scoped fromJson wiring looks correctPer-flavor spec is passed where required; setup matches the new runner API.
bin/test-runner/jamduna-067.ts (1)
4-9: Chainspec handling is correct; no action neededThe
chainSpecsarray is properly handled through implicit default detection. Whenrunner()is called without achainSpecsparameter, theRunnerBuilderreturns an emptychainSpecsarray. During test execution, this empty array triggers the fallback to[tinyChainSpec](line 254 in common.ts), ensuring each test case receives the appropriate default chainSpec. No additional chainSpec wiring is required.
tomusdrw
changed the title
e15ed21 to
fed7acc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/test-runner/w3f-davxy-071.ts (1)
19-22: Consider using a conventional exit code.The error handling is good, but
process.exit(-1)is unconventional. Standard practice uses positive integers (e.g.,process.exit(1)) for error conditions. While-1works in Node.js (interpreted as 255), positive integers are more portable and follow common conventions.Apply this diff if you'd like to use a conventional exit code:
- process.exit(-1); + process.exit(1);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/test-runner/w3f-davxy-071.ts(1 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/w3f-davxy-071.ts
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/w3f-davxy-071.ts
⏰ 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: test (22.x)
- GitHub Check: benchmarks (22.x)
- GitHub Check: e2e (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
🔇 Additional comments (3)
bin/test-runner/w3f-davxy-071.ts (3)
5-8: LGTM! Clear improvement in test vector configuration.The new structure with
patternsand per-extensionacceptedmapping makes the test discovery more explicit and extensible, aligning well with the PR's goal to support clean loading of multiple test vector formats.
11-11: Good addition.Including
genesis.binin the ignored list is consistent with the support for both.binand.jsontest vectors.
12-15: LGTM! Consistent with the new pattern-based API.The change from full filenames to base paths (without extensions) aligns with the new pattern-based test discovery approach. The comments explaining the ignore reasons are helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bin/test-runner/w3f-davxy-071.ts (1)
15-18: Use exit code 1 instead of -1.While the error handling is good, exit code -1 is not standard. Exit codes should be in the range 0-255, where 0 indicates success and positive values indicate errors. Node.js will convert -1 to 255, but it's clearer to use 1 explicitly.
Apply this diff:
.catch((e) => { logger.error`${e}`; - process.exit(-1); + process.exit(1); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/test-runner/w3f-davxy-071.ts(1 hunks)packages/jam/transition/externalities/accumulate-externalities.test.ts(2 hunks)packages/jam/transition/externalities/accumulate-externalities.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jam/transition/externalities/accumulate-externalities.test.ts
- packages/jam/transition/externalities/accumulate-externalities.ts
🧰 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/w3f-davxy-071.ts
🧠 Learnings (1)
📚 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/w3f-davxy-071.ts
⏰ 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). (2)
- GitHub Check: e2e (22.x)
- GitHub Check: test (22.x)
🔇 Additional comments (2)
bin/test-runner/w3f-davxy-071.ts (2)
5-8: LGTM! Pattern-based configuration aligns with the refactoring.The change from a flat list to a per-extension mapping structure is consistent with the project-wide shift to pattern-based test discovery mentioned in the PR objectives.
11-11: LGTM! Ignored path correctly updated for pattern-based system.Removing the
.jsonextension is appropriate since the new pattern-based configuration handles extensions separately.
View all
Benchmarks summary: 63/63 OK ✅ |
3 participants
Uh oh!
There was an error while loading. Please reload this page.