Test `httpRequestTests` against actual Services #1708

weihanglo · 2022-09-05T17:14:47Z

Motivation and Context

Description

Before this PR, we constructs Request manually to test against @httpRequestTests of protocol compliances tests. After this PR, we construct a tower service and turn it into a Router.

That makes our test cases

exercise against a larger surface from routing to parameter validation.
avoids calling non-public method to exercise tests
test against HTTP method field

The implementation is quite simple. We add a helper module renderTestHelper which contains fn validate_request(…) sharing the logic of building a Router and exercise against each @httpRequestTests. We then put the assertion logic inside each operation handler. Some type coercions are needed inside fn create_operation_registry_builder(…) since we want a builder with blanket operations at first, and then set the actual operation handler for each test case.

Testing

Run ./gradlew codegen-server-test:test and see if all protocol tests are passed.

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.

.../amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/Instantiator.kt

...en/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/InstantiatorTest.kt

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

Co-authored-by: david-perez <d@vidp.dev>

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

weihanglo · 2022-09-08T11:40:52Z

Most of the issues have been resolved. I marked minor refactoring comments as resolve and left other major issues unresolved. Thank you for your review, @david-perez!

Missing `X-Amz-Target` in response header

github-actions · 2022-09-09T06:39:19Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Velfi · 2022-09-09T17:55:13Z

...client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/Instantiator.kt

-        val variant = data.members.iterator().next()
-        val memberName = variant.key.value
+
+        val variant = if (ctx.defaultsForRequiredFields && data.members.isEmpty()) {


It'd be nice to destructure here, maybe something like this?

Suggested change

val variant = if (ctx.defaultsForRequiredFields && data.members.isEmpty()) {

val (memberName, memberDefaultValue) = if (ctx.defaultsForRequiredFields && data.members.isEmpty()) {

Velfi · 2022-09-09T17:58:00Z

...client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/Instantiator.kt

+    /**
+     * Returns a default value for a shape.
+     *
+     * Warning: this method does not take into account any constraint traits attached to the shape.


It'd be nice if these docs could include some examples of what this function does. You could take one of your tests and convert it.

jdisanti

Client changes look good to me!

jdisanti · 2022-09-09T17:58:48Z

...client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/Instantiator.kt

    )

+    companion object {
+        fun defaultContext() = Ctx(lowercaseMapKeys = false, streaming = false, builder = false, defaultsForRequiredFields = false)
+    }


This is fine, but it's a lot more flexible if you just set the defaults using default args directly on Ctx, since then you can override one of the defaults easily. For example, if you only wanted to override streaming: Ctx(streaming = true).

weihanglo · 2022-09-09T18:21:46Z

Since this PR somehow blocks #1693 I'll merge it as is and do some follow-ups. Thank you all for the reviews!

github-actions · 2022-09-09T19:47:19Z

A new generated diff is ready to view.

A new doc preview is ready to view.

This comment was marked as outdated.

Sign in to view

weihanglo marked this pull request as ready for review September 5, 2022 18:20

weihanglo requested review from a team, crisidev and david-perez as code owners September 5, 2022 18:20

weihanglo added the needs-server-review label Sep 5, 2022

This comment was marked as outdated.

Sign in to view

david-perez reviewed Sep 6, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

weihanglo and others added 12 commits September 8, 2022 00:09

Make Instantiator generate default values for required field on demand

f022ca2

Move looping over operations into ServerProtocolTestGenerator

8182c21

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

Add protocol test helper functions

9474fa2

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

Add method param to construct http request

ca2ea33

Put request validation logic inside closure

21c3a6d

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

Make protocol test response instantiate with default values

ed129b3

Add module meta for helper module

d003bf6

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

Apply suggestions from code review

727fc1b

Co-authored-by: david-perez <d@vidp.dev>

Address most style suggestions

ad43acb

add companion object for attribute #[allow(dead_code)]

7c2672e

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

Use writable to make code readable

2b058d0

recursively call filldefaultValue

4212818

Signed-off-by: Weihang Lo <weihanglo@users.noreply.github.com>

weihanglo force-pushed the weihanglo/protocol-test-enhancement branch from 3ed0ad4 to 4212818 Compare September 8, 2022 00:09

This comment was marked as outdated.

Sign in to view

Exercise with OperationExtension

440dbae

weihanglo requested a review from david-perez September 8, 2022 11:40

This comment was marked as outdated.

Sign in to view

david-perez mentioned this pull request Sep 8, 2022

Invalid AWS JSON 1.1 protocol tests smithy-lang/smithy#1391

Closed

hlbarber mentioned this pull request Sep 8, 2022

Escape routing regex in path segment literals #1719

Merged

Merge branch 'main' into weihanglo/protocol-test-enhancement

6413996

This comment was marked as outdated.

Sign in to view

Temporary protocol tests fix for smithy-lang/smithy#1391

2a5768c

Missing `X-Amz-Target` in response header

This comment was marked as outdated.

Sign in to view

Add X-Amz-Target for common models

3342693

david-perez approved these changes Sep 9, 2022

View reviewed changes

david-perez added needs-sdk-review and removed needs-server-review labels Sep 9, 2022

Velfi reviewed Sep 9, 2022

View reviewed changes

Velfi approved these changes Sep 9, 2022

View reviewed changes

jdisanti approved these changes Sep 9, 2022

View reviewed changes

Merge branch 'main' into weihanglo/protocol-test-enhancement

5b3d4fd

weihanglo enabled auto-merge (squash) September 9, 2022 18:21

This comment was marked as outdated.

Sign in to view

Merge branch 'main' into weihanglo/protocol-test-enhancement

96ad669

weihanglo merged commit f27aa54 into main Sep 9, 2022

weihanglo deleted the weihanglo/protocol-test-enhancement branch September 9, 2022 20:07

Velfi removed the needs-sdk-review label Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test `httpRequestTests` against actual Services #1708

Test `httpRequestTests` against actual Services #1708

weihanglo commented Sep 5, 2022 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

weihanglo commented Sep 8, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot commented Sep 9, 2022

Velfi Sep 9, 2022 •

edited

Loading

Velfi Sep 9, 2022

jdisanti left a comment

jdisanti Sep 9, 2022

weihanglo commented Sep 9, 2022

This comment was marked as outdated.

github-actions bot commented Sep 9, 2022

	val variant = if (ctx.defaultsForRequiredFields && data.members.isEmpty()) {
	val (memberName, memberDefaultValue) = if (ctx.defaultsForRequiredFields && data.members.isEmpty()) {

Test httpRequestTests against actual Services #1708

Test httpRequestTests against actual Services #1708

Conversation

weihanglo commented Sep 5, 2022 • edited Loading

Motivation and Context

Description

Testing

Checklist

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

weihanglo commented Sep 8, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot commented Sep 9, 2022

Velfi Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Velfi Sep 9, 2022

Choose a reason for hiding this comment

jdisanti left a comment

Choose a reason for hiding this comment

jdisanti Sep 9, 2022

Choose a reason for hiding this comment

weihanglo commented Sep 9, 2022

This comment was marked as outdated.

github-actions bot commented Sep 9, 2022

Test `httpRequestTests` against actual Services #1708

Test `httpRequestTests` against actual Services #1708

weihanglo commented Sep 5, 2022 •

edited

Loading

Velfi Sep 9, 2022 •

edited

Loading