-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[ProtocolTests] #1 Protocol tests generation stub and cmake #3269
Conversation
048a95c
to
46d9a02
Compare
@Data | ||
public class C2jGiven { | ||
@Data | ||
public class C2jGivenHttp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could be static class i believe
|
||
@Data | ||
public class C2jOutputResponse { | ||
Integer status_code; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use stack int
instead of heap Integer
since we're not making a collection
String serviceName) throws Exception { | ||
GsonBuilder gsonBuilder = new GsonBuilder(); | ||
gsonBuilder.registerTypeAdapter(C2jXmlNamespace.class, new C2jXmlNamespaceDeserializer()); | ||
gsonBuilder.registerTypeAdapter(JsonNode.class, new JsonNodeDeserializer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we actually need JsonNodeDeserializer
and a type adapter registered? this pattern is kind of weird. we use a jackson type adapter in gson. we should really only be using one or the other. is there any reason we actually need this type adapter? from what i can tell we dont do any special serialization we just call object mapper, so I'm pretty sure we dont actually need this adapter to exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in protocol tests, there is a test suite definition, with a pre-defined schema which is known and can be coded into a known object model tree.
However, the input/output parameters definition is not known and evaluated only during runtime:
https://github.com/aws/aws-sdk-cpp/blob/main/tools/code-generation/protocol-tests/output/json_1_0.json#L882-L888
The tests are design to hold an arbitrary json, such as:
"result": {
"stringValue": "string",
"documentValue": [
true,
false
]
},
and it will be different per each test/operation.
My plan is to not parse this structure into an object tree, but evaluate during the runtime of test generation.
I've chosen JsonNode because I've already used it on a similar project.
Essentially, I need just some duck-typed dict()
class here, and JsonNode would suite it.
Also, having gson to parse the known Json structure and Jackson JsonNode is a kind of mental distinguishing exercise here.
import org.apache.velocity.app.VelocityEngine; | ||
import org.apache.velocity.runtime.RuntimeConstants; | ||
import org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader; | ||
import org.slf4j.helpers.NOPLoggerFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesnt seem like we use this import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we actually use
velocityEngine.addProperty(RuntimeConstants.RUNTIME_LOG_INSTANCE, new NOPLoggerFactory().getLogger(""));
template.merge(context, sw); | ||
|
||
try { | ||
sw.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be using the try with resources pattern here i.e.
try(final StringWriter sw = new StringWriter();) {
//...do stuff with sw
}
46d9a02
to
fa7096d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
fa7096d
to
30ae219
Compare
Issue #, if available:
Protocol tests
Description of changes:
Adds generation of stubs (empty dummy projects) for protocol tests.
Also, generates the cmake script and wires them to a corresponding test client.
Build of protocol tests can be enabled in cmake by adding
-DENABLE_PROTOCOL_TESTS=ON
(OFF by default).However, if attempted to build tests, the build fail because of some issues that these tests uncover, I will cover actual tests generation and build issues fixes in the following PRs.
Tested by re-generating all and building.
The generated tests will look like:
Check all that applies:
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.