Add property coverage to @uncompliant tests - #294
Closed
smanes0213 wants to merge 14 commits into
Closed
smanes0213 wants to merge 14 commits into
smanes0213 wants to merge 14 commits into
Conversation
Both @uncompliant:extended and @uncompliant:collapsed are deprecated (Thunder docs/plugin/interfaces/tags.md) and tested here solely to pin existing generator behaviour against regressions Both test interfaces previously only contained a single echo method — for @uncompliant:extended this meant the test exercised zero distinguishing behaviour: the mode's only effect is on property parameter encoding, not methods, making the test identical to @compliant Added a Value read/write @Property to both interfaces so the tests actually expose what each mode does differently: @uncompliant:extended: property SET sends a bare scalar (42 not {"value":42}); methods remain wrapped — only properties are affected @uncompliant:collapsed: property SET and single-param methods both send bare scalars; collapsed tests additionally assert that sending a wrapped object to a collapsed method is rejected Signed-off-by: smanes0213 <sankalpmaneshwar46@outlook.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens the functional test suite coverage for deprecated JSON-RPC generator modes @uncompliant:extended and @uncompliant:collapsed by adding a read/write @property (Value) to the test interfaces and validating the resulting wire-format differences (property SET/GET and method parameter shaping).
Changes:
- Add
Valueread/write@propertyto both uncompliant test interfaces and their implementations. - Expand JSON-RPC and COM-RPC functional tests to cover property SET/GET behavior and boundary values.
- Update functional test documentation tables/matrix and adjust the CI workflow trigger (currently includes a temporary branch).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/FunctionalTests/README.md | Expands the test-interface matrix (COM-RPC vs JSON-RPC) and documents additional JSON format tests. |
| tests/FunctionalTests/jsonrpc/tests/TestJsonUncompliantExtendedJsonRpc.cpp | Adds JSON-RPC tests asserting extended-mode method wrapping and property bare-scalar behavior. |
| tests/FunctionalTests/jsonrpc/tests/TestJsonUncompliantCollapsedJsonRpc.cpp | Adds JSON-RPC tests asserting collapsed-mode bare-scalar method/property behavior and an extra wrapped-param case. |
| tests/FunctionalTests/comrpc/tests/TestJsonUncompliantExtended.cpp | Adds COM-RPC property round-trip tests via the proxy. |
| tests/FunctionalTests/comrpc/tests/TestJsonUncompliantCollapsed.cpp | Adds COM-RPC property round-trip tests via the proxy. |
| tests/FunctionalTests/common/interfaces/README.md | Documents new interfaces/annotations covered and updates the coverage matrix. |
| tests/FunctionalTests/common/interfaces/ITestJsonUncompliantExtended.h | Adds Value property and clarifies extended-mode behavior in comments. |
| tests/FunctionalTests/common/interfaces/ITestJsonUncompliantCollapsed.h | Adds Value property and clarifies collapsed-mode behavior in comments. |
| tests/FunctionalTests/common/implementations/TestJsonUncompliantExtendedImpl.cpp | Implements the new Value property with stored state. |
| tests/FunctionalTests/common/implementations/TestJsonUncompliantCollapsedImpl.cpp | Implements the new Value property with stored state. |
| .github/workflows/ProxyStubFunctionalTests.yml | Modifies PR branch filter (includes a temporary feature branch). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
tests/FunctionalTests/comrpc/tests/TestJsonUncompliantExtended.cpp:44
- The comment here is misleading:
Testing::TestHarnesskeeps a single static proxy for the whole test suite (SetUpTestSuite), so the initial value is not guaranteed to be 0 and can depend on prior tests. The test itself is fine, but the comment should reflect the shared-state behavior.
// A fresh singleton implementation starts at 0 unless a previous test wrote to it.
// This test just verifies the getter returns a valid uint32 without error.
.github/workflows/ProxyStubFunctionalTests.yml:16
- The pull_request trigger is now scoped to a temporary development branch name. If this is only for short-term CI on the current branch (as noted in the PR description), it should be removed before merging so CI continues to run for PRs targeting other branches without needing additional workflow edits.
pull_request:
branches: [ "master", "development/uncompliant-property-coverage" ]
paths:
| | `TEST_STRUCTS` | `ITestStructs` | ✓ | ✓ | POD struct marshalling: in/out/inout parameters, nested structs, `std::vector<struct>`, `@opaque`, `@index` (property slot), `@restrict` on vectors | | ||
| | `TEST_EVENTS` | `ITestEvents` | ✓ | — | Event callback pattern (`@event`, `INotification`): scalar payloads, struct payloads, `std::vector` payloads, `OptionalType` payloads, `@statuslistener` | | ||
| | `TEST_ASYNC` | `ITestAsync` | ✓ | ✓ | `@async` pattern: concurrent slots, `ICallback` interface, `@property` with `@index`, `OptionalType` in a callback | | ||
| | `TEST_INTERFACES` | `ITestInterfaces` | ✓ | — | Generator control annotations: `@interface` (void\* + ID dynamic typing), `@stub` (server-side only), `@omit` (excluded from both proxy and stub) | |
Comment on lines
+44
to
+48
| TEST_F(TestJsonUncompliantCollapsedJsonRpc, Method_WrappedParam_NotRejected) { | ||
| string response; | ||
| EXPECT_EQ(Core::ERROR_NONE, | ||
| CallMethod("pingCollapsed", R"({"payload":"abc"})", response)); | ||
| } |
Comment on lines
+39
to
+44
| // @uncompliant:collapsed does NOT reject wrapped-object params for methods. | ||
| // The stub registers Core::JSON::String for the payload parameter. Core::JSON::String | ||
| // accepts non-quoted input as a raw string value, so {"payload":"abc"} is stored | ||
| // as the literal text and the call succeeds. This pins that the stub has no | ||
| // object-vs-scalar discrimination for string parameters. | ||
| TEST_F(TestJsonUncompliantCollapsedJsonRpc, Method_WrappedParam_NotRejected) { |
Contributor
Author
|
Closing this pull request as it is already present in this PR |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
NOTE: The Branch addition in ProxyStubFunctionalTest.yml is temporary to run the tests on current branch.