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

EventStreamUnmarshallerGeneratorTest is running misconfigured tests for the server #1442

Open
david-perez opened this issue Jun 6, 2022 · 1 comment
Labels
bug Something isn't working server Rust server SDK

Comments

@david-perez
Copy link
Contributor

The tests in EventStreamUnmarshallerGeneratorTest.kt are being run for the server because in EventStreamTestTools.kt we have:

https://github.com/awslabs/smithy-rs/blob/242e6bcd677b5600f27106f8bc45f411c3c429da/codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/EventStreamTestTools.kt#L306-L306

That is, each test case is duplicated: one with CodegenTarget.CLIENT, one with CodegenTarget.SERVER.

The problem is this is incorrect, because both test types are using the testSymbolProvider from TestHelpers.kt

https://github.com/awslabs/smithy-rs/blob/242e6bcd677b5600f27106f8bc45f411c3c429da/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/testutil/TestHelpers.kt#L66-L71

That is the symbol provider that the client uses, which is very different from the server one (for example, the client generates Optional types for @required members, and the server doesn't).

It should instead be using serverTestSymbolProvider:

https://github.com/awslabs/smithy-rs/blob/242e6bcd677b5600f27106f8bc45f411c3c429da/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/testutil/ServerTestHelpers.kt#L30-L35

However, we can't make it use that symbol provider, because the server symbol provider is in ServerTestHelpers.kt, part of the codegen-server subproject's test sources. But EventStreamUnmarshallerGeneratorTest.kt is in the codegen subproject. codegen cannot depend on codegen-server, it's the other way around.

One way to fix this would be to have codegen-server's test sources depend on codegen's test sources. We would rewrite the test case provider in the server subproject to use the serverTestSymbolProvider, and then the test would just delegate to EventStreamUnmarshallerGeneratorTest.kt passing it the modified test case. I've tried looking into this and it's not straightforward.

The examples above are in Groovy; I don't know how to do it in the Kotlin DSL. Gradle's official documentation seems to recommend the testFixtures approach for sharing test code across projects, but in our case we would not only want to share the test cases but also the test code from EventStreamUnmarshallerGeneratorTest.kt. I don't know if that's abusing the testFixtures pattern.

To me it seems like this is yet another indication that we should restructure smithy-rs to have three subprojects: a core library, the client plugin, and the server plugin. Both the client and server plugins would depend on the core library's implementation and test code.

@rcoh
Copy link
Collaborator

rcoh commented Jun 9, 2022

To me it seems like this is yet another indication that we should restructure smithy-rs to have three subprojects: a core library, the client plugin, and the server plugin. Both the client and server plugins would depend on the core library's implementation and test code.

I'm definitely in favor of this

@jdisanti jdisanti added bug Something isn't working server Rust server SDK labels Sep 30, 2022
jdisanti added a commit that referenced this issue Dec 20, 2022
* Move the allow lints customization into `codegen-core`
* Move the crate version customization into `codegen-core`
* Move "pub use" extra into `codegen-core`
* Move `EventStreamSymbolProvider` into `codegen-core`
* Move the streaming shape providers into `codegen-core`
* Refactor event stream marshall/unmarshall tests
* Break `codegen-server` dependency on `codegen-client`
* Split up `EventStreamTestTools`
* Move codegen context creation in event stream tests
* Restructure tests so that #1442 is easier to resolve in the future
* Add client/server prefixes to test classes
* Improve TODO comments in server event stream tests
* Use correct builders for `ServerEventStreamMarshallerGeneratorTest`
* Remove test cases for protocols that don't support event streams
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server Rust server SDK
Projects
None yet
Development

No branches or pull requests

3 participants