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

Server protocol tests extras #1164

Closed
david-perez opened this issue Feb 7, 2022 · 4 comments
Closed

Server protocol tests extras #1164

david-perez opened this issue Feb 7, 2022 · 4 comments
Assignees
Labels
good first issue Good for newcomers server Rust server SDK small

Comments

@david-perez
Copy link
Contributor

The client has suites like rest-json-extras.smithy that contains extra protocol tests, some of which are upstreamed in the awslabs/smithy repo and will be hosted there in the next Smithy release.

Ideally the server should also pass these suites, and we should put there additional tests we come up with.

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

  • We can more easily upstream the fixed test to awslabs/smithy repo since we write Smithy and not Kotlin.
  • When we upgrade to the next Smithy release, the broken ones in ExpectFailing will fail because they now pass, so we will get alerted and remove the fixed tests from the "extra" suite.
@david-perez david-perez added the server Rust server SDK label Feb 7, 2022
@david-perez
Copy link
Contributor Author

A first test we should add to not regress on #1163 (but to the extras suite, not to simple.smithy like the diff below shows):

diff --git a/codegen-server-test/model/simple.smithy b/codegen-server-test/model/simple.smithy
index 70dee327..2feae1d2 100644
--- a/codegen-server-test/model/simple.smithy
+++ b/codegen-server-test/model/simple.smithy
@@ -63,7 +63,10 @@ resource Service {
         id: "RegisterServiceResponseTest",
         protocol: "aws.protocols#restJson1",
         params: { id: "1", name: "TestService" },
-        body: "{\"id\":\"1\",\"name\":\"TestService\"}",
+        headers: {
+            "Content-Type": "TestService",
+        },
+        body: "{\"id\":\"1\"}",
         code: 200,
     }
 ])
@@ -85,6 +88,9 @@ structure RegisterServiceInputRequest {
 structure RegisterServiceOutputResponse {
     @required
     id: ServiceId,
+
+    @required
+    @httpHeader("Content-Type")
     name: ServiceName,
 }

@david-perez
Copy link
Contributor Author

Another one: httpQueryParams duplicate query keys behavior: smithy-lang/smithy#1071

@82marbag 82marbag self-assigned this May 31, 2022
82marbag pushed a commit that referenced this issue May 31, 2022
Add rest-json-extras.smithy and protocol tests that are now failing in smithy-rs,
but are not correct from smithy

Closes: #1164

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag mentioned this issue May 31, 2022
2 tasks
82marbag pushed a commit that referenced this issue Jun 7, 2022
Add rest-json-extras.smithy and protocol tests that are now failing in smithy-rs,
but are not correct from smithy

Closes: #1164

Signed-off-by: Daniele Ahmed <[email protected]>
82marbag pushed a commit that referenced this issue Jun 7, 2022
Add rest-json-extras.smithy and protocol tests that are now failing in smithy-rs,
but are not correct from smithy

Closes: #1164

Signed-off-by: Daniele Ahmed <[email protected]>
@82marbag 82marbag mentioned this issue Jun 24, 2022
2 tasks
@drganjoo
Copy link
Contributor

We can now remove the only remaining broken test as it has been fixed upstream

@drganjoo drganjoo added good first issue Good for newcomers small labels May 17, 2024
@david-perez
Copy link
Contributor Author

I'm going to close this since I no longer stand by this:

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

Now that we have #3726, there are no advantages to putting broken protocol tests in the <protocol>-extras.smithy models.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers server Rust server SDK small
Projects
None yet
Development

No branches or pull requests

3 participants