Modify and Merge protocol test request unit tests codegen logic #445
Conversation
| @@ -282,7 +274,8 @@ protected void generateTestServer( | |||
| String name, | |||
| Consumer<GoWriter> handler | |||
| ) { | |||
| super.generateTestServer(writer, name, handler); | |||
| // We aren't using a test server, but we do need a URL to set. | |||
| writer.write("serverURL := \"http://localhost:8888/\""); | |||
There was a problem hiding this comment.
Can't we just remove this entire generateTestServer method? Is its code needed at all anymore?
There was a problem hiding this comment.
If we do that we have to remove it from super class HttpProtocolUnitTestGenerator and simply provide this fake url to request and response generator separately.
There was a problem hiding this comment.
Also we need the test cases' host info in some host test cases, I prefer to keep it now in case of breaking changes.
| const captureRequestID = "CaptureProtocolTestRequest" | ||
|
|
||
| // AddCaptureRequestMiddleware captures serialized http request during protocol test for check | ||
| func AddCaptureRequestMiddleware(stack *middleware.Stack, req *http.Request) error { |
There was a problem hiding this comment.
Question: Do we have to expose this in the public API?
If I understand correctly, the generated protocol tests in the SDK repo will have to see it to use it, but I think I'd rather we just generate it as unexported code in the protocol test instead of having it be exported in smithy-go just to be used for testing.
There was a problem hiding this comment.
Note that if/when we go that route you can just drop the unit test. The protocol tests needing to pass based on it are assertion enough imo.
There was a problem hiding this comment.
Alternatively we can put it somewhere under private/ like we've done in a few other places - to have it usable by the SDK, but to signal that we don't want other people doing so.
There was a problem hiding this comment.
I prefer alternative method cause the it prevent smithy go codegen using some sdk dependency of middleware. I will create a new private/ directory under smithy-go repo and move the middleware there.
| @@ -250,20 +241,21 @@ protected void generateTestAssertions(GoWriter writer) { | |||
| writeAssertNil(writer, "err"); | |||
| writeAssertNotNil(writer, "result"); | |||
|
|
|||
| writeAssertScalarEqual(writer, "c.ExpectMethod", "actualReq.Method", "method"); | |||
| writeAssertScalarEqual(writer, "c.ExpectURIPath", "actualReq.URL.RawPath", "path"); | |||
| writeAssertScalarEqual(writer, "c.ExpectMethod", "capturedReq.Method", "method"); | |||
There was a problem hiding this comment.
This is totally fine but you might have been able to get away with less changes by just connecting your new middleware to the originally declared actualReq request.
Modify and Merge protocol test request unit tests codegen logic to isolate cases from local server and instead capture and check request directly via middleware