Skip to content

Auto-discover protocoltest instead of manually adding them to build#3392

Open
Madrigal wants to merge 6 commits into
mainfrom
feat-rebuild-protocol-test-gen
Open

Auto-discover protocoltest instead of manually adding them to build#3392
Madrigal wants to merge 6 commits into
mainfrom
feat-rebuild-protocol-test-gen

Conversation

@Madrigal
Copy link
Copy Markdown
Contributor

Currently, protocol tests are created by manually updating codegen/protocol-test-codegen/smithy-build.json, where we manually input which service to use according to a template.

This means when a new protocol test gets added, we need to read the release notes and remember to manually add them to this file.

With this PR, protocol tests are instead read from the runtime class path. We find all Smithy services available and generate the smithy-build.json file manually. This is done on as a new step generate-smithy-build on protocol test gradle file which is attached to the build step, which now generates the whole content of internal/protocoltest.

This PR leaves existing tests exactly as-is, name and everything. The diff of smithy-build.json only indicates changes on position, but all entries are the same.

Removing services in this script instead of ProtocolUtils in smithy-go since we're removing whole services instead of just individual tests.

Added a step to add manual files to internal/protocoltest since there are some tests that we wrote manually and would be lost.

@Madrigal Madrigal requested a review from a team as a code owner April 20, 2026 21:48
@@ -0,0 +1,43 @@
# Protocol Test Codegen – Manual Files
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: why we need 2 same README here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's an artifact of manually copying all files from manual to here. There's some discussion about just not checking them in, which may be the way we go instead.

Comment thread codegen/protocol-test-codegen/build.gradle.kts
@@ -1,240 +1,281 @@
{
"version": "1.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The sdk-codegen project does the same programmatic generation but we don't check in the file at all, we should do the same here, mainly so someone doesn't edit this one by mistake (but also we just don't need it).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, this sounds good to me, will remove it and just keep it as part of generate

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed it from checking it in, but it is kind of annoying that every time I run generate the file is there preventing me from switching to a different branch if it exists on that branch.

I could add it to .gitignore, but I'm not sure if that would cause any issue with workflows that expect the file to exist.

@@ -0,0 +1,43 @@
# Protocol Test Codegen – Manual Files
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might want to just have a GoIntegration that generates this stuff instead of having to have this new mechanism but it's nbd. I imagine it could just gate on "is service in protocoltests namespace" etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This sounds good, but if we have a GoIntegration writing manual files will mean writing a Java file that creates a Go template, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

correct yes. up to you

@Madrigal Madrigal force-pushed the feat-rebuild-protocol-test-gen branch from 8b466d4 to b5585d1 Compare April 23, 2026 19:03
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