Additional unit tests for OpenAPIObjectContainer#24
Conversation
|
@swift-server-bot add to allowlist |
simonjbeaumont
left a comment
simonjbeaumont
left a comment
There was a problem hiding this comment.
@rboyce Thanks for the patch—and for adding some tests for it.
simonjbeaumont
left a comment
simonjbeaumont
left a comment
There was a problem hiding this comment.
Oops, meant to mark as "Request changes", with some minor fixups in https://github.com/apple/swift-openapi-runtime/pull/24/files#r1262219996.
Fix OpenAPIValueContainer serialization of nested values ### Motivation Credit goes to @rboyce for discovering this bug! When I saw the PR #24, I realized we didn't have any unit tests for the `OpenAPI*Container` types, which was an oversight. This PR adds those missing unit tests, and fixes the bug in casting that motivated #24. ### Modifications Adds unit tests and fixes the casting bug that expected wrapped, instead of raw stdlib values. ### Result Now all the new unit tests pass and serializing nested values in OpenAPIValueContainer and friends works correctly. ### Test Plan Most of this PR is new unit tests, verified they pass locally. Reviewed by: simonjbeaumont Builds: ✔︎ pull request validation (5.8) - Build finished. ✔︎ pull request validation (5.9) - Build finished. ✔︎ pull request validation (docc test) - Build finished. ✔︎ pull request validation (integration test) - Build finished. ✔︎ pull request validation (nightly) - Build finished. ✔︎ pull request validation (soundness) - Build finished. ✖︎ pull request validation (api breakage) - Build finished. #25
|
@rboyce Okay I landed the base unit tests - if you'd like, you could port your additional unit tests to the new place ( |
rboyce
force-pushed
the
anyValueContainer
branch
from
538c735 to
eadd49a
Compare
rboyce
changed the title
rboyce
force-pushed
the
anyValueContainer
branch
from
eadd49a to
9297cc4
Compare
|
Rebased past #25. This PR now just contains the additional unit tests for mixed nested values; thanks for the fix @czechboy0! |
| let encoder: JSONEncoder = .init() | ||
| encoder.outputFormatting = [.prettyPrinted, .sortedKeys] | ||
| let data = try encoder.encode(value) | ||
| XCTAssertEqual( |
There was a problem hiding this comment.
Please make it consistent with the other tests by using the utility functions that does the encoding/decoding and equality checks.
| var dict: OpenAPIObjectContainer = .init() | ||
| } | ||
|
|
||
| do { |
There was a problem hiding this comment.
Why is this extra do scope here?
| var dict: OpenAPIObjectContainer = .init() | ||
| } | ||
|
|
||
| do { |
There was a problem hiding this comment.
Same here about the do scope.
|
Thanks, please also update the PR description to reflect the remaining changes. |
czechboy0
left a comment
czechboy0
left a comment
There was a problem hiding this comment.
Looks good overall, I'll open another PR bringing it to our style in a bit.
|
Fixing up the style: #33 |
3 participants
Motivation
Trying to encode an
OpenAPIValueContainerwould previously throw anEncodingErrorif the untyped container contained nested dictionary or array structures. (e.g.{"dict": {"foo": "bar"}}) Data could be decoded into this format, but it could not be re-encoded successfully.Modifications
When encoding the value of a nested JSON array or object in
OpenAPIValueContainer, we now handle arrays and dicts by wrapping the inner value inOpenAPIValueContainerbefore passing it to the encoder.(note: Based on the rest of the
encode()implementation, it seems that this may have been expected to happen previously during decoding. Let me know if making changes on the decode side of things is preferable.)Result
Additional unit tests added to validate that encoder generates correct output for nested data structures in
OpenAPIValueContainer, and that these data structures can be decoded and successfully re-endcoded afterward.Test Plan
Run
Test_CodableExtensionstests, see all tests pass.