Skip to content

Expand protocol test coverage for json1.0, query and cbor #6245

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

Merged
merged 15 commits into from
Jul 10, 2025

Conversation

alextwoods
Copy link
Contributor

@alextwoods alextwoods commented Jul 8, 2025

This PR does three things (detailed in modifications section below):

  1. Expand protocol test coverage for json1.0, query and cbor using directly translated smithy protocol tests.
  2. Add support for new protocol test runner features to support the new tests.
  3. Minor codegen updates to handle the protocol test models

Motivation and Context

This is a followup/expansion of #6230.

This change improves our protocol test coverage for the json1.0, query and cbor protocols and ensures that all cases covered by the official Smithy protocol tests are run.

Modifications

SDK Behavior changes:

  • fix codegeneration for enums with numeric string values (eg "0" -> "VALUE_0"). There are NO existing AWS services using these - they are only present in protocol tests and without this change cause a compilation error.
  • support XmlNamespace with non-nested uri (adds new constructor)

Protocol Test Runner updates:

  • add a cborEquals body matcher (equivalent functionality to jsonEquals). Ensures that encoded bodies are functionally equal while allowing differences in the exact cbor bytes.
  • add mustContain matchers to header and query params.
  • jsonEquals body matcher - relax equality on whole number double vs integer (eg: we now consider 1.0 to be equal 1)
  • queryEquals body matcher - adds a new body matcher for query that ensures encoded body (or request parameters) are functionally equal.
  • add resolvedHost assertions
  • support host given (allowing configuring custom endpoint)
  • support requests with hostPrefixes/labels added (requires workarounds for wiremock - we need to avoid dns lookups being done for endpoints like "foo.localhost"
  • Dynamic parsing of expected timestamp values - allowing fractional seconds with double values + either second or millisecond epoch values. Legacy tests use epoch milliseconds and smithy tests use epoch seconds.

Protocol Test Suite changes/Additions:

  • Add a new query protocol service (under smithy-query) - used for the ported smithy query tests.
  • Replace the old query protocol test suite with the new official ported smithy tests.
  • Add a new json1.0 service + official ported smithy tests
  • Replace the protocol tests for cbor with the official ported smithy tests.

FAQ

Why add a new query protocol service instead of replacing the existing one?
The existing query model/service is used in a large number of custom/hand written tests cases (eg: QueryExceptionTests). Adding a new service for the new protocol test suite allows us to leave these existing tests alone.

Testing

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@alextwoods alextwoods requested a review from a team as a code owner July 8, 2025 20:13
@alextwoods alextwoods added the changelog-not-required Indicate changelog entry is not required for a specific PR label Jul 8, 2025
Copy link

sonarqubecloud bot commented Jul 9, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
8.2% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@@ -302,6 +302,11 @@ public String getEnumValueName(String enumValue) {
// Special cases
result = result.replace("textORcsv", "TEXT_OR_CSV");

// leading digits, add a prefix
if (result.matches("^\\d.*")) {
result = "VALUE_" + result;
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the java limitation of not being able to have a number as an enum value, but can you explain how this substitution works in practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the value has leading digits (eg: 0 or 123_some_value) the regex will match, and then we just prefix it with "VALUE", so we end up with VALUE_0 and VALUE_123_SOME_VALUE for those cases.

@alextwoods alextwoods added this pull request to the merge queue Jul 10, 2025
Merged via the queue into master with commit e97b94b Jul 10, 2025
34 of 35 checks passed
Copy link

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
changelog-not-required Indicate changelog entry is not required for a specific PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants