refactor: make wasm-sdk contract version test config dynamic #2830
Conversation
Refactor platform version compatibility test configuration to be data-driven and auto-generate test suites. Makes adding new contract format versions easier by centralizing configuration.
…dynamic Replace hardcoded version arrays with auto-generated compatibility data from CONTRACT_FORMAT_VERSIONS config. Tests now dynamically iterate over all defined formats, eliminating need for manual updates when adding new contract versions.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReplaces hard-coded platform-version constants in the data-contract unit tests with dynamic PLATFORM_VERSIONS and CONTRACT_FORMAT_VERSIONS, auto-generates a per-format FORMATS compatibility matrix, and updates tests to iterate over formats and use PLATFORM_VERSIONS.MAX for latest-version checks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/wasm-sdk/tests/unit/data-contract.spec.mjs (3)
35-40: Consider removing the redundantplatformVersionproperty.The
platformVersionproperty is a direct copy ofconfig.introduced, creating unnecessary duplication. Tests could referenceFORMATS.V0.introduceddirectly instead ofFORMATS.V0.platformVersion.If the intent is to provide a more semantic property name for test readability, consider adding a comment explaining this design choice.
Apply this diff to remove the redundancy:
acc[formatKey] = { ...config, compatibleVersions, incompatibleVersions, - platformVersion: config.introduced, };Then update all test references from
FORMATS.V0.platformVersiontoFORMATS.V0.introduced(and similarly for V1).
28-33: Optional: Simplify incompatible versions generation.The
allVersionsarray is created solely to filter it forincompatibleVersions. While this is clear and works fine for test configuration, you could simplify it:- const allVersions = Array.from( - { length: PLATFORM_VERSIONS.MAX - PLATFORM_VERSIONS.MIN + 1 }, - (_, i) => i + PLATFORM_VERSIONS.MIN - ); - - const incompatibleVersions = allVersions.filter(v => v < config.introduced); + const incompatibleVersions = Array.from( + { length: Math.max(0, config.introduced - PLATFORM_VERSIONS.MIN) }, + (_, i) => i + PLATFORM_VERSIONS.MIN + );
6-19: Optional: Consider adding configuration validation.The configuration system doesn't validate constraints such as:
PLATFORM_VERSIONS.MIN < PLATFORM_VERSIONS.MAX- Format
introducedversions fall within the MIN-MAX range- Format
introducedversions are uniqueWhile misconfiguration would likely cause obvious test failures, early validation could provide clearer error messages. However, for test configuration, the current approach prioritizing simplicity is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/wasm-sdk/tests/unit/data-contract.spec.mjs(11 hunks)
🔇 Additional comments (2)
packages/wasm-sdk/tests/unit/data-contract.spec.mjs (2)
277-287: Excellent refactor to dynamic test iteration!Replacing hard-coded test blocks with dynamic iteration over
FORMATSis a solid improvement. This eliminates duplication and automatically extends test coverage when new format versions are added to the configuration.
83-83: Consistent and correct usage of FORMATS throughout tests.All test cases correctly use
FORMATS.V0.platformVersionandFORMATS.V1.platformVersioninstead of hard-coded constants. This ensures tests automatically adapt when platform version configurations change.Also applies to: 109-109, 140-140, 158-158, 171-171, 179-179, 183-183, 187-187, 197-197, 205-205, 213-213, 228-228, 235-235, 239-239, 249-249, 253-253, 265-266
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/wasm-sdk/tests/unit/data-contract.spec.mjs (2)
5-54: LGTM! Excellent refactoring to dynamic configuration.The configuration setup is well-structured and the validation logic is solid. The auto-generated compatibility arrays correctly compute which platform versions support each contract format.
However, one minor observation:
At line 48, the
platformVersionfield duplicatesconfig.introduced. While this might be intentional for clarity or to match a specific naming convention in tests, consider either:
- Removing the redundant field and using
introduceddirectly in tests, or- Adding a comment explaining why both fields exist
Apply this diff if you'd like to remove the redundancy:
acc[formatKey] = { ...config, compatibleVersions, incompatibleVersions, - platformVersion: config.introduced, };Then update test references from
FORMATS.V0.platformVersiontoFORMATS.V0.introduced.
314-329: Refactor hard-coded version numbers to use dynamic configuration.The edge case test "should handle version boundary correctly at V9 transition" uses hard-coded version numbers (9, 8) at lines 316, 321, and 327. For consistency with the dynamic configuration approach throughout the rest of the file, consider using
FORMATS.V1.platformVersionandFORMATS.V1.platformVersion - 1.Apply this diff to align with the dynamic configuration:
it('should handle version boundary correctly at V9 transition', () => { // V0 contract should work in V9 (backward compatibility) - const contract = sdk.DataContract.fromJSON(contractFixtureV0, 9); + const contract = sdk.DataContract.fromJSON(contractFixtureV0, FORMATS.V1.platformVersion); expect(contract.id()).to.equal(contractFixtureV0.id); contract.free(); // V1 contract should work in V9 (first supported version) - const contractV1 = sdk.DataContract.fromJSON(contractFixtureV1, 9); + const contractV1 = sdk.DataContract.fromJSON(contractFixtureV1, FORMATS.V1.platformVersion); expect(contractV1.id()).to.equal(contractFixtureV1.id); contractV1.free(); // V1 contract should fail in V8 (last unsupported version) expect(() => { - sdk.DataContract.fromJSON(contractFixtureV1, 8); + sdk.DataContract.fromJSON(contractFixtureV1, FORMATS.V1.platformVersion - 1); }).to.throw(/dpp unknown version/); });Alternatively, if you want to keep the test description more generic, you could rename the test to reference the format dynamically or update the description to use template literals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/wasm-sdk/tests/unit/data-contract.spec.mjs(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/tests/unit/data-contract.spec.mjs
packages/**/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place unit and integration tests alongside each package in packages//tests
Files:
packages/wasm-sdk/tests/unit/data-contract.spec.mjs
🔇 Additional comments (2)
packages/wasm-sdk/tests/unit/data-contract.spec.mjs (2)
92-283: LGTM! Tests correctly use dynamic configuration.All test cases have been properly updated to use
FORMATS.V0.platformVersionandFORMATS.V1.platformVersioninstead of hard-coded constants. The refactoring maintains test coverage while improving maintainability.
285-296: LGTM! Dynamic test generation is well-implemented.The dynamic iteration over all defined formats is excellent. This achieves the PR objective of making the test suite fully data-driven and self-expanding. Adding a new contract format now only requires updating the
CONTRACT_FORMAT_VERSIONSconfiguration.
The "should handle version boundary correctly at V9 transition" test duplicated coverage already provided by the dynamic compatibility matrix. The matrix already tests V0/V1 contracts at version 9 and V1 failure at version 8 through the auto-generated compatible/incompatible version loops.
|
Conflicting with #2800 |
I'll mark this as draft for now to possibly revisit later. |
Issue being fixed or feature implemented
The WASM SDK data contract test suite used hard-coded version arrays that required manual updates across multiple locations when adding new contract format versions or platform versions. This made the codebase harder to maintain and error-prone.
Fixes #2803 comment.
What was done?
Refactored the data contract test configuration to be fully data-driven and self-expanding:
CONTRACT_FORMAT_VERSIONSconfig object mapping format versions to their introduction platform version and fixturesFORMATSobject that auto-generates compatibility arrays, incompatibility arrays, and platform version constants for all defined formatsFORMATSreferencesAdding a new contract format (e.g., V2) now requires:
PLATFORM_VERSIONS.MAXto the new versionV2: { introduced: X, fixture: contractFixtureV2 }toCONTRACT_FORMAT_VERSIONSAll compatibility arrays, test suites, and constants are automatically generated.
How Has This Been Tested?
Breaking Changes
None. This is a pure refactoring with no API or behavior changes.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit