Protocol tests parsed response validation by SergeyRyabinin · Pull Request #3414 · aws/aws-sdk-cpp

SergeyRyabinin · 2025-05-08T23:07:31Z

Issue #, if available:
Protocol tests
Description of changes:
Generate parsed response structure validation
Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sbiscigl · 2025-05-19T16:07:23Z

tools/scripts/run_protocol_tests.py

+            else:  # Unix-like
+                self.process.send_signal(signal.SIGINT)
+
+            time.sleep(0.5)


do we need static sleep? what are we waiting for?

we need to wait a bit to let the process and OS to terminate the process.

sbiscigl · 2025-05-19T16:09:18Z

tools/scripts/run_protocol_tests.py

+    def wait(self):
+        """Wait for the subprocess to complete."""
+        if self.process:
+            return self.process.wait()


waits without timeouts have heavily traumatized my up to this point, lets add a reasonable timeout to prevent zombie hosts

this is actually a left-over from the AI generated slop I used as an initial draft.
I just removed it.

sbiscigl · 2025-05-19T16:14:19Z

...main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CppProtocolTestGenerator.java

+                .collect(Collectors.toSet());
+        // include the request shapes
+        Set<String> requestShapes = testSuite.getCases().stream()
+                .map(entry -> {


looks like we only ever add one item to the set ever, simplified this could just be

testSuite.getCases().stream() .map(entry -> { return String.format("<aws/%s/model/%s.h>", serviceModel.getMetadata().getProjectName(), entry.getGiven().getName() + "Request")}) .collect(Collectors.toSet());

to avoid creating a extra set per test case

copy-paste mistake, thank you!

sbiscigl · 2025-05-19T16:16:39Z

...s/com/amazonaws/util/awsclientgenerator/velocity/cpp/protocoltests/result/ValidateOutcome.vm

+#if(!$expectedResult.isEmpty())
+  const ${responseShape.name}& result = outcome.GetResult();
+#end
+  /* expectedResult = R"( ${expectedResult} )" */


love the debug comment

sbera87 · 2025-05-21T19:01:34Z

...main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CppProtocolTestGenerator.java

+    /**
+     * Perform legacy patching of the ec2 model present from the very beginning.
+     */
+    private void legacyPatchEc2Model() {


I think this can be optimized to use one loop

private void legacyPatchEc2Model() { if (!"ec2".equals(serviceModel.getMetadata().getProtocol())) { return; } Map<String, Shape> shapes = serviceModel.getShapes(); Map<String, Shape> updatedShapes = new HashMap<>(); for (Map.Entry<String, Shape> entry : shapes.entrySet()) { String key = entry.getKey(); Shape shape = entry.getValue(); String newKey = key.replaceAll("Result$", "Response"); shape.setName(shape.getName().replaceAll("Result$", "Response")); shape.setType(shape.getType().replaceAll("Result$", "Response")); updatedShapes.put(newKey, shape); } shapes.clear(); shapes.putAll(updatedShapes); }

The complexity is the same, in addition, this was a copy-pasted method from the actual EC2 client generator.
I refactored it a bit and re-using the existing one.

Additionally,

Map<String, Shape> updatedShapes = new HashMap<>();

would re-shuffle shapes in a map as far as I remember, Json parsers use Linked structures.

sbera87 · 2025-05-21T19:04:58Z

tools/scripts/run_protocol_tests.py

+            print(f"No protocol tests found in {self.build_dir}")
+            self.fail = True
+        for test in tests:
+            mock_server = MockHttpServerHandle()


should be used with 'with' for cleanup

thank you, I've refactored this piece with enter/exit methods and with.

sbera87 · 2025-05-21T19:05:04Z

tools/scripts/run_protocol_tests.py

+    """
+
+    def __init__(self, args):
+        self.debug = args.get("debug", False)


yes, unused.
Thank you for noticing!

sbera87 · 2025-05-21T19:11:50Z

tools/scripts/run_protocol_tests.py

+        self.process = subprocess.Popen(python_cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        print(f"Started mock server with PID {self.process.pid}")
+
+    def __del__(self):


This may not be deterministically called by the interpreter per https://docs.python.org/3.4/reference/datamodel.html
Better to use https://book.pythontips.com/en/latest/context_managers.html

Thank you, I've refactored the code to explicitly implicitly call new method stop() with the with statement.

sbera87 · 2025-05-21T19:12:37Z

tools/scripts/run_protocol_tests.py

+    """
+
+    def __init__(self):
+        python_cmd = [shutil.which("python3"), PROTO_TEST_MOCK_HANDLER]


No exception handling found

I've added a bit of exception handling here, but can't do much more than that.
Catch-trace-crash/return -1 is the current approach.

sbera87 · 2025-05-21T19:16:25Z

tools/scripts/run_protocol_tests.py

+            mock_server = MockHttpServerHandle()
+            try:
+                process = subprocess.run([test], timeout=6 * 60, check=True)
+                process.check_returncode()


this is redundant as check is True
per python documentation
If check is true, and the process exits with a non-zero exit code, a [CalledProcessError](https://docs.python.org/3/library/subprocess.html#subprocess.CalledProcessError) exception will be raised. Attributes of that exception hold the arguments, the exit code, and stdout and stderr if they were captured.

Thank you, I removed the check.

DmitriyMusatkin · 2025-05-21T23:30:50Z

tools/scripts/run_protocol_tests.py

+    :return: dict with parsed arguments
+    """
+    parser = argparse.ArgumentParser(description="Protocol tests driver for AWS-SDK-CPP")
+    parser.add_argument("--debug", action="store_true", help="Print additional information")


tiny suggestion: parser.add_argument("--debug", action="store_true", default = False, help="Print additional information")

and remove logic later

DmitriyMusatkin · 2025-05-21T23:34:54Z

tools/scripts/run_protocol_tests.py

+
+    def __init__(self):
+        try:
+            python_cmd = [shutil.which("python3"), PROTO_TEST_MOCK_HANDLER]


sys.executable if you want to make sure its the same python?

No need to be sure it is the same interpreter

test driver

mock server

test app
All can be seen as an independent apps.

SergeyRyabinin added a commit that referenced this pull request May 8, 2025

Codegen for #3414: Protocol tests response object validation

ee87925

SergeyRyabinin force-pushed the sr/pt/response branch from 1491dca to ee87925 Compare May 8, 2025 23:09

SergeyRyabinin added a commit that referenced this pull request May 9, 2025

Codegen for #3414: Protocol tests response object validation

2f031e3

SergeyRyabinin force-pushed the sr/pt/response branch from ee87925 to 2f031e3 Compare May 9, 2025 22:32

SergeyRyabinin marked this pull request as ready for review May 9, 2025 22:39

SergeyRyabinin requested review from sbera87 and sbiscigl May 9, 2025 22:39

SergeyRyabinin added a commit that referenced this pull request May 12, 2025

Codegen for #3414: Protocol tests response object validation

98cd28b

SergeyRyabinin force-pushed the sr/pt/response branch from 2f031e3 to 98cd28b Compare May 12, 2025 07:46

SergeyRyabinin added a commit that referenced this pull request May 16, 2025

Codegen for #3414: Protocol tests response object validation

605eb6b

SergeyRyabinin force-pushed the sr/pt/response branch from 98cd28b to 605eb6b Compare May 16, 2025 18:59

sbiscigl approved these changes May 19, 2025

View reviewed changes

SergeyRyabinin added a commit that referenced this pull request May 21, 2025

Codegen for #3414: Protocol tests response object validation

b7768e6

SergeyRyabinin force-pushed the sr/pt/response branch from 605eb6b to b7768e6 Compare May 21, 2025 16:40

sbera87 reviewed May 21, 2025

View reviewed changes

SergeyRyabinin added a commit that referenced this pull request May 21, 2025

Codegen for #3414: Protocol tests response object validation

fcffa00

SergeyRyabinin force-pushed the sr/pt/response branch from b7768e6 to fcffa00 Compare May 21, 2025 22:22

aws deleted a comment from sbera87 May 21, 2025

DmitriyMusatkin reviewed May 21, 2025

View reviewed changes

DmitriyMusatkin approved these changes May 21, 2025

View reviewed changes

SergeyRyabinin added 4 commits May 22, 2025 07:01

Protocol tests parsed response validation

4f82caf

Protocol tests runner

22e082d

Code review comments

a9e5c01

Codegen for #3414: Protocol tests response object validation

acefa26

SergeyRyabinin force-pushed the sr/pt/response branch from fcffa00 to acefa26 Compare May 22, 2025 14:01

sbera87 self-requested a review May 22, 2025 16:47

sbera87 approved these changes May 22, 2025

View reviewed changes

SergeyRyabinin merged commit 9225f08 into main May 22, 2025
4 checks passed

SergeyRyabinin deleted the sr/pt/response branch May 22, 2025 17:15

Conversation

SergeyRyabinin commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SergeyRyabinin commented May 8, 2025 •

edited

Loading