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

Protocol tests for ACCEPT header with * #1648

Merged
merged 6 commits into from
Aug 23, 2022
Merged

Protocol tests for ACCEPT header with * #1648

merged 6 commits into from
Aug 23, 2022

Conversation

82marbag
Copy link
Contributor

Motivation and Context

Add protocol tests for #1646

Testing

./gradlew :codegen-server-test:test && ./gradlew :codegen-test:test

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.

@82marbag 82marbag requested a review from a team as a code owner August 18, 2022 21:45
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.

@82marbag 82marbag enabled auto-merge (squash) August 19, 2022 02:32
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.

Can you link to the Smithy PR in a TODO somewhere in the model? So that we don't forget to remove this test when we upgrade to the next Smithy version.

Comment on lines 238 to 241
@output
structure AcceptHeaderStarServiceOutput {}
@input
structure AcceptHeaderStarServiceInput {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should dispense of superfluous modeling in the protocol tests, even if they are best practices, to keep the files minimal so readers can easily focus on the modeling aspects of the test that are strictly relevant to what it is testing.

So, e.g. not using @input/@output, using a single AcceptHeaderStarServiceInputOutput structure, not using @idempotent etc.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@@ -18,6 +18,7 @@ service MiscService {
ResponseCodeRequiredOperation,
ResponseCodeHttpFallbackOperation,
ResponseCodeDefaultOperation,
AcceptHeaderStarService,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be added to codegen-server-test?

Copy link
Contributor Author

@82marbag 82marbag Aug 19, 2022

Choose a reason for hiding this comment

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

It's a symlink:

$ ls -l codegen-server-test/model/misc.smithy
codegen-server-test/model/misc.smithy -> ../../codegen-test/model/misc.smithy

I added a appliesTo: "server", to run those tests only in the server.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@82marbag
Copy link
Contributor Author

82marbag commented Aug 22, 2022

It was called out in http-rs/http-types#349 that values can take a quoted string in the form of e.g. "x, y" so splitting by comma isn't always the correct approach. However for us I don't think we will have this issue. We will move to an implementation by a library, but for now, I'll leave it like this.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@82marbag 82marbag merged commit 7025253 into main Aug 23, 2022
@82marbag 82marbag deleted the accept_header branch August 23, 2022 14:20
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.

3 participants