Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add server extra tests #1497

Merged
merged 8 commits into from
Aug 24, 2022
Merged

Add server extra tests #1497

merged 8 commits into from
Aug 24, 2022

Conversation

82marbag
Copy link
Contributor

Motivation and Context

See: #1164

Description

Tests:

Testing

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag requested a review from a team as a code owner June 24, 2022 14:07
Signed-off-by: Daniele Ahmed <[email protected]>
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

codegen-test/model/rest-json-extras.smithy Outdated Show resolved Hide resolved
codegen-test/model/rest-json-extras.smithy Outdated Show resolved Hide resolved
codegen-test/model/rest-json-extras.smithy Outdated Show resolved Hide resolved
codegen-test/model/rest-json-extras.smithy Outdated Show resolved Hide resolved
@david-perez
Copy link
Contributor

If you want, just add the suite in this PR without adding the tests mentioned in #1164. We can tackle #1164 in a separate PR.

#1164 also talks about doing this:

Fixed tests should also go there, instead of hot-patching them at runtime like we currently do.

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag
Copy link
Contributor Author

If you want, just add the suite in this PR without adding the tests mentioned in #1164. We can tackle #1164 in a separate PR.

#1164 also talks about doing this:

Fixed tests should also go there, instead of hot-patching them at runtime like we currently do.

Ok, changed this PR to move towards that instead

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@crisidev
Copy link
Contributor

crisidev commented Aug 2, 2022

Is this PR still relevant?

@82marbag 82marbag requested a review from a team as a code owner August 23, 2022 14:25
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

codegen-test/model/rest-json-extras.smithy Show resolved Hide resolved
@@ -78,7 +78,8 @@ service RestJsonExtras {
code: 500,
body: "",
headers: { "X-Amzn-Errortype": "ExtraError" },
params: {}
params: {},
appliesTo: "client",
Copy link
Contributor

@david-perez david-perez Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails for the server because servers are expected to serialize {} when the output parameters are empty.

Hence why we have a serverStructureSerializer that behaves like that. We should arguably modify the Kotlin docs for serverStructureSerializer calling this out.

Anyway, this test can remain for the client only, but we should duplicate it in a new test that only applies to the server and that has body: "{}", and copy over the documentation from RestJsonGreetingWithErrors to justify why we're doing so.

codegen-test/model/rest-json-extras.smithy Outdated Show resolved Hide resolved
codegen-test/model/rest-json-extras.smithy Outdated Show resolved Hide resolved
codegen-test/model/rest-json-extras.smithy Outdated Show resolved Hide resolved
@@ -302,7 +307,8 @@ operation CaseInsensitiveErrorOperation {
code: 500,
body: "{\"Message\": \"hello\"}",
headers: { "X-Amzn-Errortype": "CaseInsensitiveError" },
params: { message: "hello" }
params: { message: "hello" },
appliesTo: "client",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expand the documentation to justify why this is a client-only test; took me a while to figure out why. It turns out that smithy-rs clients should be able to parse error messages if the JSON keys them as message, Message, or errorMessage. The relevant parsing code is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment with the link

Signed-off-by: Daniele Ahmed <[email protected]>
Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i'd place documentation inside the documentation field instead of in a comment in the Smithy file, so that it gets rendered in the protocol test.

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag enabled auto-merge (squash) August 23, 2022 19:42
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@82marbag 82marbag merged commit 39be2d1 into main Aug 24, 2022
@82marbag 82marbag deleted the fix-tests-3 branch August 24, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants