Implements TestJam, Testing helpers and Testing docs#106
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive testing infrastructure for JAM services, including a new TestJam helper class for simpler test setup, assertion utilities, automated test configuration generation, and corresponding documentation. Updates include refactored simulator APIs, expanded work-report types, and integration with the build command. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
# Conflicts: # packages/jammin-sdk/genesis-state-generator.test.ts
![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
🤖 Fix all issues with AI agents
In `@docs/src/testing.md`:
- Around line 53-55: The docs reference two different config filenames
("jam.toml" and "jammin.build.yml") which is inconsistent; update all mentions
to use the canonical filename (choose one and apply consistently) — e.g., change
the setup example comment and any troubleshooting text that refers to
"jammin.build.yml" to the canonical name so the example with TestJam.create()
and all other occurrences (including lines around the cited blocks and lines
461-465) consistently reference the same filename.
In `@packages/jammin-sdk/index.ts`:
- Line 11: The public API change removed the top-level re-export "export * as
state from \"@typeberry/lib/state\"" (and replaced work-report exports by
re-exporting them via testing-helpers.js); confirm intent and either restore the
state re-export or document the breaking change: if you intended to keep the SDK
surface, re-add the top-level export (the missing symbol is the namespace export
"state" from "@typeberry/lib/state") so consumers can import sdk.state;
otherwise update the package release notes/CHANGELOG to call out removal of
"state" from the public API and ensure no internal imports rely on that export.
🧹 Nitpick comments (5)
packages/jammin-sdk/simulator.ts (2)
298-301: Consider merging options rather than replacing them.The
withOptions()method replaces all options. If users want to change only one option (e.g., justdebug), they lose any previously set options. Consider merging instead:♻️ Proposed fix to merge options
withOptions(options: SimulatorOptions): this { - this.options = options; + this.options = { ...this.options, ...options }; return this; }
343-358: Side effect:chainSpecis mutated onthis.options.When
this.options.chainSpecis undefined, Line 353-354 mutatesthis.optionsby settingchainSpec = tinyChainSpec. This persists across futureaccumulation()calls, which may be unexpected since options are supposed to be explicitly set viawithOptions().Consider using a local variable instead:
♻️ Proposed fix to avoid mutating options
async accumulation(): Promise<AccumulateResult> { const result = await simulateAccumulation(this.state, this.workReports, this.options); this.workReports = []; if (this.state instanceof InMemoryState) { this.state.applyUpdate(result); } else { if (!this.blake2b) { this.blake2b = await Blake2b.createHasher(); } - if (!this.options.chainSpec) { - this.options.chainSpec = tinyChainSpec; - } - this.state.backend.applyUpdate(serializeStateUpdate(this.options.chainSpec, this.blake2b, result)); + const chainSpec = this.options.chainSpec ?? tinyChainSpec; + this.state.backend.applyUpdate(serializeStateUpdate(chainSpec, this.blake2b, result)); } return result; }packages/jammin-sdk/testing-helpers.test.ts (2)
17-22: Consider usingbeforeEachto isolate test state.The
jaminstance is created once inbeforeAlland reused across all tests. Sinceaccumulation()applies state updates tojam.state, later tests may be affected by state changes from earlier tests, potentially causing test interdependencies.♻️ Proposed fix to isolate tests
describe("testing-helpers", () => { let jam: TestJam; - beforeAll(() => { + beforeEach(() => { jam = TestJam.empty(); });
96-103: Test may silently pass if no exception is thrown.The test at Lines 96-103 uses try/catch to verify the error message, but if
expectStateChangedoesn't throw, the test passes without any assertions executing.♻️ Proposed fix to ensure exception is thrown
test("should include custom error message", () => { + let caught = false; try { expectStateChange(10, 5, (before, after) => after > before, "Custom error"); } catch (e) { + caught = true; expect(e).toBeInstanceOf(StateChangeAssertionError); expect((e as Error).message).toBe("Custom error"); } + expect(caught).toBe(true); });Or use
expect().toThrow()pattern consistently:test("should include custom error message", () => { - try { - expectStateChange(10, 5, (before, after) => after > before, "Custom error"); - } catch (e) { - expect(e).toBeInstanceOf(StateChangeAssertionError); - expect((e as Error).message).toBe("Custom error"); - } + expect(() => { + expectStateChange(10, 5, (before, after) => after > before, "Custom error"); + }).toThrow("Custom error"); });packages/jammin-sdk/simulator.test.ts (1)
1-12: Isolate TestJam per test to avoid state leakage.
accumulation()mutates state andwithOptions()can persist; reusing a single instance viabeforeAllcan make tests order-dependent. PreferbeforeEach(or instantiating inside each test) for isolation.♻️ Suggested change
-import { beforeAll, describe, expect, test } from "bun:test"; +import { beforeEach, describe, expect, test } from "bun:test"; import * as config from "@typeberry/lib/config"; import { generateGuarantees, TestJam } from "./simulator.js"; import { CoreId, Gas, ServiceId, Slot } from "./types.js"; import { createWorkReportAsync } from "./work-report.js"; describe("simulateAccumulation", () => { let jam: TestJam; - beforeAll(() => { + beforeEach(() => { jam = TestJam.empty(); });
DrEverr
changed the title
skoszuta
approved these changes
Feb 5, 2026
tomusdrw
approved these changes
Feb 5, 2026
…accumulation() to accumulate(), fine-grained logging control, and documentation updates Addresses feedback from tomusdrw and skoszuta on PR 106: Phase 1: Code Generation Enhancement - Create generate-test-config.ts utility to generate TypeScript files with type-safe service ID mappings - Enhance build-command to automatically generate config/jammin.test.config.ts with service details and chainspec - Services now have a type-safe mapping instead of hardcoded values Phase 2: API Refinement - Rename accumulation() method to accumulate() for more concise verb-based API - Update all tests and documentation to use new method name - Improves code readability and aligns with reviewer preference Phase 3: Debug Logging Enhancement - Add DebugLoggingOptions interface for fine-grained control over logging categories - Support both boolean (enable all) and object (selective) debug modes - Available options: - pvmExecution: PVM execution details including instruction traces - ecalliTrace: Ecalli host call traces for service execution - hostCalls: Host calls during service execution - accumulate: Accumulation process details and state transitions - safrole: Randomness and validator selection operations - refine: Work report refinement and validation - stateTransitions: State transitions and root changes - Backward compatible with existing debug: true usage Phase 4: Documentation Updates - Add prerequisite section about building services before testing - Include custom assertions in basic test example - Document fine-grained logging control with examples - Update available options to reflect new debug capabilities - Emphasize use of generated test config for type safety All tests pass, quality checks pass, build succeeds.
…accumulation() to accumulate(), fine-grained logging control, and documentation updates Addresses feedback from tomusdrw and skoszuta on PR 106: Phase 1: Code Generation Enhancement - Create generate-test-config.ts utility to generate TypeScript files with type-safe service ID mappings - Enhance build-command to automatically generate config/jammin.test.config.ts with service details and chainspec - Services now have a type-safe mapping instead of hardcoded values Phase 2: API Refinement - Rename accumulation() method to accumulate() for more concise verb-based API - Update all tests and documentation to use new method name - Improves code readability and aligns with reviewer preference Phase 3: Debug Logging Enhancement - Add DebugLoggingOptions interface for fine-grained control over logging categories - Support both boolean (enable all) and object (selective) debug modes - Available options: - pvmExecution: PVM execution details including instruction traces - ecalliTrace: Ecalli host call traces for service execution - hostCalls: Host calls during service execution - accumulate: Accumulation process details and state transitions - safrole: Randomness and validator selection operations - refine: Work report refinement and validation - stateTransitions: State transitions and root changes - Backward compatible with existing debug: true usage Phase 4: Documentation Updates - Add prerequisite section about building services before testing - Include custom assertions in basic test example - Document fine-grained logging control with examples - Update available options to reflect new debug capabilities - Emphasize use of generated test config for type safety All tests pass, quality checks pass, build succeeds.
Create convenient wrapper functions for common BytesBlob conversions: - stringToBlob(): Convert UTF-8 strings to BytesBlob - numbersToBlob(): Convert number arrays to BytesBlob - hexToBlob(): Flexible hex parsing (with or without 0x prefix) - hexToBlobWithPrefix(): Explicit wrapper for parseBlob() - hexToBlobNoPrefix(): Explicit wrapper for parseBlobNoPrefix() These wrappers provide a cleaner API and reduce boilerplate in tests and service code. Export from jammin-sdk utils module for easy access.
Member
Author
|
I've implemented suggestions, but it's quite a change so maybe a re-review would be good. |
tomusdrw
approved these changes
Feb 16, 2026
Co-authored-by: Tomek Drwięga <tomusdrw@users.noreply.github.com>
Co-authored-by: Tomek Drwięga <tomusdrw@users.noreply.github.com>
…or generated config, update typeberry/lib to 0.5.7, remove unnececerry workreport check
Member
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
bin/cli/src/commands/build-command.ts (1)
117-126: Variableserviceson line 119 shadows the outerservicesfrom line 98.Both are named
servicesbut have different types (ServiceConfig[]vsServiceBuildOutput[]). While functionally correct due to block scoping, it hinders readability. Consider renaming, e.g.,builtServicesorloadedServices.Proposed rename
try { s.start("Generating test configuration..."); - const services = await loadServices(projectRoot); - await generateTestConfigInProjectDir(services, projectRoot); + const builtServices = await loadServices(projectRoot); + await generateTestConfigInProjectDir(builtServices, projectRoot); s.stop("✅ Test configuration generated"); p.log.message("📝 Generated: config/jammin.test.config.ts");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/cli/src/commands/build-command.ts` around lines 117 - 126, The inner variable named services (the one assigned from loadServices and passed to generateTestConfigInProjectDir) shadows an outer services variable of a different type (ServiceConfig[] vs ServiceBuildOutput[]), hurting readability; rename the inner variable to a distinct name such as loadedServices or testServices wherever it's declared and used (the assignment from loadServices and the call to generateTestConfigInProjectDir) so references are unambiguous and types no longer collide while leaving the outer services identifier untouched.packages/jammin-sdk/testing-helpers.ts (1)
117-129: Predicate returningundefinedsilently passes the assertion.When
StateChangePredicatereturnsundefined(e.g., due to a missingreturnstatement or an incomplete conditional),expectStateChangesilently succeeds because only=== falsetriggers the error. This can mask bugs in test predicates. Consider treating any non-truereturn as a failure:♻️ Suggested alternative
const result = predicate(before, after); - // If predicate returns false explicitly, throw error - if (result === false) { + // If predicate does not return true, throw error + if (result !== true) { throw new StateChangeAssertionError(errorMessage ?? "State change validation failed", before, after); }If the
undefined-passes-silently behavior is intentional, document it explicitly in the JSDoc to avoid surprise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jammin-sdk/testing-helpers.ts` around lines 117 - 129, The current expectStateChange function treats only an explicit false as failure, letting undefined (e.g., missing return in StateChangePredicate) pass silently; update the check in expectStateChange to fail on any non-true result (use result !== true) and throw StateChangeAssertionError with the provided errorMessage or default, so undefined/other falsy values trigger the assertion; if this behavior was intentional instead, add JSDoc to expectStateChange and StateChangePredicate documenting that only an explicit false fails and undefined passes.packages/jammin-sdk/simulator.test.ts (1)
7-12: Shared mutableTestJaminstance across tests risks order-dependent failures.
TestJam.empty()is created once inbeforeAll, butaccumulate()mutates the internal state (applies state updates) andwithOptions()replacesthis.optionsfor subsequent calls. If test execution order changes or new tests are added, earlier tests' side effects (state mutations, lingering options) can silently affect later ones.Consider using
beforeEachinstead so each test starts with a fresh instance:♻️ Suggested fix
- let jam: TestJam; - - beforeAll(() => { - jam = TestJam.empty(); - }); + let jam: TestJam; + + beforeEach(() => { + jam = TestJam.empty(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jammin-sdk/simulator.test.ts` around lines 7 - 12, The tests use a shared mutable TestJam instance created by TestJam.empty() in beforeAll, which causes order-dependent failures because methods like accumulate() mutate internal state and withOptions() replaces this.options; change the setup to create a fresh TestJam for each test (use beforeEach to call TestJam.empty() and assign to the jam variable) so every test starts with a new instance and no cross-test state persists.packages/jammin-sdk/simulator.ts (2)
260-295:enableLogsreconfigures the global logger on every call — consider caching.This function is called on every
simulateAccumulationinvocation. Theimport()call andLogger.configureAll()are repeated even when the same options are used. Consider caching the last-applied config to skip redundant reconfiguration:♻️ Suggested approach
+let lastLogConfig = ""; + async function enableLogs(options: DebugLoggingOptions) { try { const loggerModule = await import("@typeberry/lib/logger"); if (loggerModule.Logger && loggerModule.Level) { const enabledLoggers: string[] = ["error"]; // ... (existing logic) const logConfig = enabledLoggers.join(","); + if (logConfig === lastLogConfig) { + return; + } + lastLogConfig = logConfig; loggerModule.Logger.configureAll(logConfig, loggerModule.Level.LOG, process.cwd()); } } catch { console.warn("Warning: Could not configure typeberry logger"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jammin-sdk/simulator.ts` around lines 260 - 295, The enableLogs function reimports and reconfigures the global logger on every call; cache the last-applied configuration and the imported logger module to avoid redundant work by storing a module-scoped variable (e.g., lastLogConfig: string | null and cachedLoggerModule) and before calling loggerModule.Logger.configureAll(check) compare the newly built logConfig (from enabledLoggers) to lastLogConfig—if unchanged, return early; otherwise update lastLogConfig and call Logger.configureAll. Also reuse the cachedLoggerModule instead of repeated import() to skip repeated dynamic imports.
230-236: Debug logging enabled by default on every accumulation call — potential noise and overhead.
debugdefaults totrue(line 232), andenableLogsis invoked on everysimulateAccumulationcall (line 236), reconfiguring the global logger each time. This means:
- Every test that doesn't explicitly pass
debug: falsegets verbose logging, which can obscure test output and slow execution.enableLogsperforms a global side effect (Logger.configureAll) on each call, which could cause issues in parallel test execution.Consider defaulting
debugtofalsefor quieter test runs, or at minimum, skip theenableLogscall when the resolved options are empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jammin-sdk/simulator.ts` around lines 230 - 236, The debug flag currently defaults to true causing enableLogs to reconfigure global logging on every simulateAccumulation call; change the default assignment of debug (the variable declared alongside slot and entropy) to false and then only call enableLogs(loggingOptions) when loggingOptions is non-empty (e.g., has at least one truthy property) to avoid unnecessary global Logger.configureAll side effects; update the logic that builds loggingOptions (the ternary using typeof debug) and gate the await enableLogs call accordingly so tests run quietly by default and global logging is not toggled on every invocation.docs/src/testing.md (1)
425-439: Inconsistent use ofServiceId(0)vs.SERVICESconstant.The "Testing Error Conditions" example uses
ServiceId(0)directly (line 429), while the rest of the document consistently recommends usingSERVICES.xxx.idfrom the generated config. This contradicts best practice#2on line 588. Consider usingSERVICES.auth.idhere for consistency, or add a comment explaining why a raw ID is appropriate for this specific error-testing scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/testing.md` around lines 425 - 439, The test uses a hardcoded ServiceId(0) which is inconsistent with the document's guidance to use generated constants; update the test's results entry to use the generated SERVICES constant (e.g., replace ServiceId(0) with SERVICES.auth.id) in the createWorkReportAsync call so it matches the rest of the docs and examples referenced by createWorkReportAsync, jam.withWorkReport().accumulate, and expectAccumulationSuccess; if there is a deliberate reason to use a raw numeric ID for error-testing, add a one-line comment above the results entry explaining why ServiceId(0) is intentionally used instead of SERVICES.auth.id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/testing.md`:
- Around line 585-594: The docs have inconsistent spelling of the term
(instances of "preconfigured", "pre-configured", and "Pre-configured"); pick one
canonical form (e.g., "preconfigured") and replace all occurrences in the
document so every reference in headings, bullet points, and inline text uses
that exact spelling and casing; search for the strings "preconfigured",
"pre-configured", and "Pre-configured" and standardize them across the file
(update headings like "Use preconfigured `SERVICES`" and any other bullets or
sections using the alternate form).
In `@packages/jammin-sdk/utils/generate-test-config.ts`:
- Around line 74-77: The generated object keys in generateTestConfigCode can be
invalid for names with hyphens/dots/digits and you also shadow the imported name
by using "config"; change the map to rename the destructured variable (e.g.,
([name, svc]) instead of ([name, config])) and emit a quoted key and properly
escaped service name, e.g. use JSON.stringify(name) for the key and
JSON.stringify(svc.name) for the name value, keeping the id as
ServiceId(${svc.id}); this ensures valid TS for any service name and avoids the
config shadowing.
- Around line 32-37: The generateTestConfigFile function currently calls
loadBuildConfig() with no args so it resolves from process.cwd(); modify
generateTestConfigFile to accept a projectRoot parameter (or an already-loaded
config) and call loadBuildConfig(projectRoot) instead, and update
generateTestConfigInProjectDir to pass its projectRoot into
generateTestConfigFile (or pass the pre-loaded config) so config resolution uses
the intended project directory; reference functions: generateTestConfigFile,
generateTestConfigInProjectDir, and loadBuildConfig.
In `@packages/jammin-sdk/work-report.ts`:
- Around line 92-116: The switch over resultStatus.type leaves execResult
uninitialized for unexpected values; add a default case in the switch that
handles unknown resultStatus.type by logging the unexpected value and assigning
execResult to a safe error value (e.g., call WorkExecResult.error(...) using an
appropriate WorkExecResultKind such as a dedicated unknown/fallback kind or a
sensible fallback like badCode), so execResult is always initialized before
being passed to WorkResult.create.
---
Duplicate comments:
In `@docs/src/testing.md`:
- Around line 446-450: The document inconsistently references two config
filenames; choose the canonical name (e.g., "jammin.build.yml") and update every
occurrence in docs/src/testing.md so all references—such as the paragraph that
currently lists `jammin.build.yml`, the earlier code example around line 67, and
any mentions alongside `SERVICES` or `config/jammin.test.config.js`—use the same
filename; ensure spelling and punctuation match exactly across the file.
---
Nitpick comments:
In `@bin/cli/src/commands/build-command.ts`:
- Around line 117-126: The inner variable named services (the one assigned from
loadServices and passed to generateTestConfigInProjectDir) shadows an outer
services variable of a different type (ServiceConfig[] vs ServiceBuildOutput[]),
hurting readability; rename the inner variable to a distinct name such as
loadedServices or testServices wherever it's declared and used (the assignment
from loadServices and the call to generateTestConfigInProjectDir) so references
are unambiguous and types no longer collide while leaving the outer services
identifier untouched.
In `@docs/src/testing.md`:
- Around line 425-439: The test uses a hardcoded ServiceId(0) which is
inconsistent with the document's guidance to use generated constants; update the
test's results entry to use the generated SERVICES constant (e.g., replace
ServiceId(0) with SERVICES.auth.id) in the createWorkReportAsync call so it
matches the rest of the docs and examples referenced by createWorkReportAsync,
jam.withWorkReport().accumulate, and expectAccumulationSuccess; if there is a
deliberate reason to use a raw numeric ID for error-testing, add a one-line
comment above the results entry explaining why ServiceId(0) is intentionally
used instead of SERVICES.auth.id.
In `@packages/jammin-sdk/simulator.test.ts`:
- Around line 7-12: The tests use a shared mutable TestJam instance created by
TestJam.empty() in beforeAll, which causes order-dependent failures because
methods like accumulate() mutate internal state and withOptions() replaces
this.options; change the setup to create a fresh TestJam for each test (use
beforeEach to call TestJam.empty() and assign to the jam variable) so every test
starts with a new instance and no cross-test state persists.
In `@packages/jammin-sdk/simulator.ts`:
- Around line 260-295: The enableLogs function reimports and reconfigures the
global logger on every call; cache the last-applied configuration and the
imported logger module to avoid redundant work by storing a module-scoped
variable (e.g., lastLogConfig: string | null and cachedLoggerModule) and before
calling loggerModule.Logger.configureAll(check) compare the newly built
logConfig (from enabledLoggers) to lastLogConfig—if unchanged, return early;
otherwise update lastLogConfig and call Logger.configureAll. Also reuse the
cachedLoggerModule instead of repeated import() to skip repeated dynamic
imports.
- Around line 230-236: The debug flag currently defaults to true causing
enableLogs to reconfigure global logging on every simulateAccumulation call;
change the default assignment of debug (the variable declared alongside slot and
entropy) to false and then only call enableLogs(loggingOptions) when
loggingOptions is non-empty (e.g., has at least one truthy property) to avoid
unnecessary global Logger.configureAll side effects; update the logic that
builds loggingOptions (the ternary using typeof debug) and gate the await
enableLogs call accordingly so tests run quietly by default and global logging
is not toggled on every invocation.
In `@packages/jammin-sdk/testing-helpers.ts`:
- Around line 117-129: The current expectStateChange function treats only an
explicit false as failure, letting undefined (e.g., missing return in
StateChangePredicate) pass silently; update the check in expectStateChange to
fail on any non-true result (use result !== true) and throw
StateChangeAssertionError with the provided errorMessage or default, so
undefined/other falsy values trigger the assertion; if this behavior was
intentional instead, add JSDoc to expectStateChange and StateChangePredicate
documenting that only an explicit false fails and undefined passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #99
Implements testing framework for JAM service development.
Updated typeberry/lib