Implements TestJam, Testing helpers and Testing docs#106
Implements TestJam, Testing helpers and Testing docs#106
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive testing infrastructure for the jammin-sdk, including a new TestJam class for fluent test simulation APIs, a testing-helpers module with assertion utilities, detailed documentation on writing tests, and updates to work-report type definitions and module exports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
|
|
||
| 1. Your `jammin.build.yml` file is properly configured | ||
| 2. You're using `TestJam.create()` (not `TestJam.empty()`) | ||
| 3. The service ID in your test matches the service ID in your configuration |
There was a problem hiding this comment.
it looks like the user should also make sure the services are built
There was a problem hiding this comment.
as suggested in other comment I'd be in favour of having build command produce a typescript file with service details (like typesafe name->id mapping)
| const result = await jam.withWorkReport(report).accumulation(); | ||
|
|
||
| // Verify results | ||
| expect(result).toBeDefined(); |
There was a problem hiding this comment.
it would be nice if this first example showed the user one of our custom assertions
|
|
||
| // Create a work report | ||
| const report = await createWorkReportAsync({ | ||
| results: [{ serviceId: ServiceId(0), gas: Gas(1000n) }], |
There was a problem hiding this comment.
build command should generate a typescript file with details about all the genesis state (we can even have it loaded there by default). This way user will have a type-safe API with the service ids and we will not need to rely on hardcoded values.
| - `pvmBackend?: PvmBackend` - PVM backend to use (`PvmBackend.Ananas` or `PvmBackend.BuiltIn`) | ||
| - `sequential?: boolean` - Use sequential accumulation mode (default: `true`) | ||
| - `entropy?: EntropyHash` - Entropy for randomness (defaults to zero hash for deterministic tests) | ||
| - `chainSpec?: ChainSpec` - Chain specification to use |
There was a problem hiding this comment.
chain spec should be defined top-level. maybe instead of having a static TestJam class we could rather have a typescript file with an instance created by build that would contain the chainspec already?
| ```typescript | ||
| const info = jam.getServiceInfo(ServiceId(0)); | ||
| console.log(`Balance: ${info?.balance}`); | ||
| console.log(`Gas used: ${info?.gasUsed}`); |
There was a problem hiding this comment.
what's 'gas used' in service info? Are you sure it's actually there?
|
|
||
| 1. Your `jammin.build.yml` file is properly configured | ||
| 2. You're using `TestJam.create()` (not `TestJam.empty()`) | ||
| 3. The service ID in your test matches the service ID in your configuration |
There was a problem hiding this comment.
as suggested in other comment I'd be in favour of having build command produce a typescript file with service details (like typesafe name->id mapping)
|
|
||
| ### Import Errors | ||
|
|
||
| Make sure you're importing from the correct module: |
|
|
||
| ### Debug Logging | ||
|
|
||
| Enable debug logging to see what's happening during accumulation: |
There was a problem hiding this comment.
can we have that enabled by default? What kind of logs does it display? I think it would be good to have more fine-grained control over debug logs to be able to see ecalli-trace logging, but also pvm-level logging if needed.
| ```typescript | ||
| import { generateState, loadServices } from "@fluffylabs/jammin-sdk"; | ||
|
|
||
| const services = await loadServices(); |
There was a problem hiding this comment.
imho this should rather be in the auto-generated typescript file that is created by build.
| * console.log(`Processed ${result.accumulationStatistics.size} work items`); | ||
| * ``` | ||
| */ | ||
| async accumulation(): Promise<AccumulateResult> { |
There was a problem hiding this comment.
My preference would be to call it using a verb (i.e. accumulate), but I'm not strongly opinionated on this.
3 participants
Resolves: #99
Implements testing framework for JAM service development.
Updated typeberry/lib