misc: make route 53 uri tests resilient to model version changes #1540
Conversation
0marperez
added
the
no-changelog
0marperez
changed the title
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
… route53-hardcoded-tests
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
|
|
||
| // `Route53UriTest` E2E tests depend on `TestEngine` | ||
| if (ctx.model.expectShape<ServiceShape>(ctx.settings.service).sdkId.lowercase() == "route 53") { | ||
| withBlock("jvmE2eTest {", "}") { | ||
| withBlock("dependencies {", "}") { | ||
| write(KotlinDependency.HTTP_TEST.dependencyNotation()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Correctness: We shouldn't hardcode this service-specific logic into the main GradleGenerator. We should either:
- Create a
Sectionand a customization for Route53 which adds the necessary code in that section –or– - Just add the http-test dependency to the E2E source set for all services
There was a problem hiding this comment.
I tried creating a Section and a customization for Route53 but our section logic doesn't seem to handle sections in KotlinIntegrations
There was a problem hiding this comment.
Just add the http-test dependency to the E2E source set for all services
You mean hardcoding it how it is now but for every service? i.e. removing the if statement
There was a problem hiding this comment.
our section logic doesn't seem to handle sections in
KotlinIntegrations
What do you mean by that? We use SectionWriter all over in our integrations: https://github.com/search?q=repo%3Aawslabs%2Faws-sdk-kotlin%20path%3A%2F%5Ecodegen%5C%2Faws-sdk-codegen%5C%2Fsrc%5C%2Fmain%5C%2Fkotlin%5C%2Faws%5C%2Fsdk%5C%2Fkotlin%5C%2Fcodegen%5C%2F%2F%20sectionWriter&type=code
There was a problem hiding this comment.
Oh I meant a SectionId that is declared in a KotlinIntegration can't be processed by a SectionWriter in another KotlinIntegration
There was a problem hiding this comment.
Oh ok. I think it should as long as the SectionId is accessible and the order is set up correctly? If not, I'd say that's a bug and we should open an issue for it
I also think just adding the http-test dependency to all services is fine
There was a problem hiding this comment.
Cool, I'll try again later and open an issue for it if needed
There was a problem hiding this comment.
Oh I meant a SectionId that is declared in a KotlinIntegration can't be processed by a SectionWriter in another KotlinIntegration
I found out why this isn't working for the GradleGenerator, it's because of the way that the build.gradle.kts file is created. The GradleGenerator uses the fileManifest instead of useFileWriter so this step is skipped
| withBlock("jvmE2eTest {", "}") { | ||
| withBlock("dependencies {", "}") { | ||
| write(KotlinDependency.HTTP_TEST.dependencyNotation()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry I may have been misleading in the last thread. You can't do this in the GradleGenerator because not every service will have a jvmE2eTest source set. You'll need to add it to E2E test dependencies another way. I think we have a different part of the codebase that maintains a list of E2E test dependencies
There was a problem hiding this comment.
There was a problem hiding this comment.
You could also just add another case if (project.name == "route53") to scope this down again
There was a problem hiding this comment.
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
services/build.gradle.kts
Outdated
| implementation(libraries.kotlin.test.junit5) | ||
| implementation(project(":tests:e2e-test-util")) | ||
| implementation(libraries.slf4j.simple) | ||
| implementation(libraries.smithy.kotlin.http.test) |
There was a problem hiding this comment.
Can this be scoped down using if (project.name == "route53") { ... }? Similar to how we do it for other services.
|
|
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
3 participants
Issue #
closes #1370
Description of changes
Refactors modeled tests into E2E tests that are resilient to model version changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.