Refactor unit tests#286
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors unit tests by reorganizing test code and OpenRPC specifications into more focused, single-responsibility modules. Methods and properties are renamed with more descriptive "test" prefixes, and tests are separated by functional concern.
Changes:
- Renamed test methods to use clearer, more descriptive names (e.g.,
method→testBasicMethod,methodWithMultipleParams→testMultipleRequiredParams) - Split property and event tests from the Advanced module into dedicated PropertyExtension and EventExtension modules
- Updated copyright notices to 2026 across multiple test files
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test_sdk/suite/simple.test.js | Updated method names to match refactored OpenRPC spec (added "test" prefix) |
| test_sdk/suite/provider.with-multiple-methods.test.js | Updated copyright from Sky UK 2025 to Comcast 2026 |
| test_sdk/suite/provider-errors.test.js | Updated copyright and renamed requestSimpleMethod to requestBasicMethod |
| test_sdk/suite/property-with-context.test.js | Moved from Advanced to PropertyExtension module; removed event tests (moved to new file) |
| test_sdk/suite/properties.test.js | Migrated from Simple to PropertyExtension module; renamed plainProperty to basicProperty |
| test_sdk/suite/event-with-context.test.js | New test file containing event tests extracted from property-with-context.test.js |
| test_sdk/openrpc/simple.json | Renamed methods with test prefix; moved properties to property-extension.json; added schema tests |
| test_sdk/openrpc/provider.json | Renamed requestSimpleMethod to requestBasicMethod |
| test_sdk/openrpc/property-extension.json | New OpenRPC spec containing all property-related methods from simple.json and event-extension.json |
| test_sdk/openrpc/event-extension.json | Renamed from Advanced to EventExtension; removed property and action methods |
| test_sdk/sdk/configOverride/languages/markdown/language.config.json | New config enabling listen and once declarations |
| test_sdk/sdk/configOverride/languages/javascript/language.config.json | New config enabling listen and once declarations |
Comments suppressed due to low confidence (1)
test_sdk/suite/property-with-context.test.js:2
- The copyright year 2026 is in the future. The current date is January 20, 2026, but code is typically copyrighted for the year it was created. Unless this code is being prepared for a future release, consider using 2025 or the actual year the code was written.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test_sdk/suite/properties.test.js
Outdated
| PropertyExtension.clear(); | ||
| }) | ||
| MockTransport.event("Simple","onPlainPropertyChanged", "value 123"); | ||
| MockTransport.event("PropertyExtension", "onbasicPropertyChanged", "value 123"); |
There was a problem hiding this comment.
Inconsistent event name casing. The event name is "onbasicPropertyChanged" (lowercase 'b') but should be "onBasicPropertyChanged" (uppercase 'B') to match the naming pattern used elsewhere in the code and to match the expected event name from the basicProperty property.
test_sdk/suite/properties.test.js
Outdated
| PropertyExtension.clear(); | ||
| }) | ||
| MockTransport.event("Simple","onPlainPropertyChanged", [1,2,3]); | ||
| MockTransport.event("PropertyExtension", "onbasicPropertyChanged", [1, 2, 3]); |
There was a problem hiding this comment.
Inconsistent event name casing. The event name is "onbasicPropertyChanged" (lowercase 'b') but should be "onBasicPropertyChanged" (uppercase 'B') to match the naming pattern used elsewhere in the code and to match the expected event name from the basicProperty property.
test_sdk/suite/properties.test.js
Outdated
| }); | ||
|
|
||
| //test listen to "onPlainPropertyChanged" event | ||
| //test listen to "onbasicPropertyChanged" event |
There was a problem hiding this comment.
Inconsistent event name casing. The event name is "onbasicPropertyChanged" (lowercase 'b') but should be "onBasicPropertyChanged" (uppercase 'B') to match the naming pattern used elsewhere in the code and to match the expected event name from the basicProperty property.
test_sdk/suite/properties.test.js
Outdated
| let p = Simple.listen("onPlainPropertyChanged", value => { | ||
| expect(value).toBe( "value 123") | ||
| PropertyExtension.clear("onbasicPropertyChanged"); | ||
| let p = PropertyExtension.listen("onbasicPropertyChanged", value => { |
There was a problem hiding this comment.
Inconsistent event name casing. The event name is "onbasicPropertyChanged" (lowercase 'b') but should be "onBasicPropertyChanged" (uppercase 'B') to match the naming pattern used elsewhere in the code and to match the expected event name from the basicProperty property.
test_sdk/suite/properties.test.js
Outdated
| expect(value).toBe("value 123") | ||
| }) | ||
| MockTransport.event("Simple","onPlainPropertyChanged", "value 123"); | ||
| MockTransport.event("PropertyExtension", "onbasicPropertyChanged", "value 123"); |
There was a problem hiding this comment.
Inconsistent event name casing. The event name is "onbasicPropertyChanged" (lowercase 'b') but should be "onBasicPropertyChanged" (uppercase 'B') to match the naming pattern used elsewhere in the code and to match the expected event name from the basicProperty property.
test_sdk/suite/properties.test.js
Outdated
| Simple.clear("onPlainPropertyChanged"); | ||
| let p = Simple.listen("onPlainPropertyChanged", value => { | ||
| expect(value).toBe( "value 123") | ||
| PropertyExtension.clear("onbasicPropertyChanged"); |
There was a problem hiding this comment.
Inconsistent event name casing. The event name is "onbasicPropertyChanged" (lowercase 'b') but should be "onBasicPropertyChanged" (uppercase 'B') to match the naming pattern used elsewhere in the code and to match the expected event name from the basicProperty property.
| */ | ||
|
|
||
| import { transport } from '../TransportHarness.js' | ||
| import MockTransport from '../../build/sdk/javascript/src/Transport/MockTransport.mjs' |
There was a problem hiding this comment.
Unused import MockTransport.
| import MockTransport from '../../build/sdk/javascript/src/Transport/MockTransport.mjs' |
No description provided.