Http compression integration tests; combatibility with Netty #1239
Conversation
| } | ||
| } | ||
|
|
||
| protected enum Client { |
There was a problem hiding this comment.
Client and Server enums are identical. Can we rename it to the generic name and reuse?
There was a problem hiding this comment.
Used different names to make the whenread better in English.
"when(H1, Server.DEFAULT, Client.DEFAULT, Request.ID, SHOULD_PASS)"
| } | ||
| } | ||
|
|
||
| protected enum Protocol { |
There was a problem hiding this comment.
Consider reusing HttpProtocol enum we have for tests
There was a problem hiding this comment.
Made it public to access it, it was in different pkg.
There was a problem hiding this comment.
Hm, why do you use http.api package here. This is http-netty module, we should use http.netty package for tests.
There was a problem hiding this comment.
IIRC the 'encodingFor' is part of api and pkg private.
There was a problem hiding this comment.
Consider creating a hook specifically for that method and keep everything else in http.netty
|
|
||
| protected enum Expectation { | ||
| SHOULD_PASS(true), | ||
| SHOULD_FAIL(false); |
There was a problem hiding this comment.
If we do not expect more cases consider using boolean. Otherwise, let's kill the SHOULD_ prefix.
| } | ||
| } | ||
|
|
||
| protected enum Request { |
There was a problem hiding this comment.
Consider naming it Compression or Coding
|
|
||
| public BaseContentCodingTest(final Scenario scenario) { | ||
| this.scenario = scenario; | ||
| this.expectedSuccess = scenario.valid; |
There was a problem hiding this comment.
Consider using scenario.valid instead of duplicating in a new variable
| public String toString() { | ||
| return "When a client that supports " + encString(clientSupported) + ", sends a request encoded with " | ||
| + requestEncoding.name() + ", to an " + (isH2 ? "H2" : "H1") + " server that supports " | ||
| + encString(serverSupported) + ", the request should be " + (valid ? "ACCEPTED" : "REJECTED"); |
There was a problem hiding this comment.
Consider simplified parametrized format:
client=[...], server=[...], request={}, protocol={}, shouldPass={}
Current format is hard to read in intellij idea and hard to run via command line a specific gradle test.
There was a problem hiding this comment.
Included index in the params (to allow running from cmd easier [1]), and simplified the text.
./gradlew --info :servicetalk-http-netty:test --tests "io.servicetalk.http.netty.ServiceTalkContentCodingTest.testCompatibility[15*]"
| .collect(StringBuilder::new, StringBuilder::append) | ||
| .toFuture().get().toString(); | ||
|
|
||
| assertEquals(payloadAsString((byte) 'a'), requestPayload); |
There was a problem hiding this comment.
Assertions work only in main test thread. If you use assertion in offloaded thread or on a server side, they won't fail the test.
We usually use a BlockingQueue to save the result we observed on the server side and validate the values in the queue in the main test thread.
There was a problem hiding this comment.
Discussed offline, the errors are surfaced with the assertion of expected successful response + printed stack trace.
However, changing the logic to capture the error and re-throw under main, to help with readability.
| } | ||
| } | ||
|
|
||
| assertEquals(expected, encodingFor(client, response.headers() |
There was a problem hiding this comment.
Similar logic here. If you verify in the filter, it can be executed in offloaded thread. You need to verify results on the caller side. Read the response via client API.
| assertResponse(client().request(client() | ||
| .get("/") | ||
| .encoding(encoding) | ||
| .payloadBody(payloadAsString((byte) 'a'), textSerializer())).toFuture().get().toStreamingResponse()); |
There was a problem hiding this comment.
Consider using BlockingHttpClient by default
|
|
||
| ChannelFuture f = ctx.write(response); | ||
|
|
||
| if (!keepAlive) { |
There was a problem hiding this comment.
For the purpose of the test, you don't need keep-alive logic. Just leave the connection open (@After will shutdown everything) or always close it, doesn't matter.
tkountis
force-pushed
the
impr/http-compr-compatibility
branch
from
e4d7fb6 to
0a926ce
Compare
Motivation:
Add integration tests between Netty & ServiceTalk with compression
Modifications:
Modified current compression tests to allow extending them for other server/client flavors.
Result:
Happy path scenarios with enabled compression is now tested both for ST/ST & ST/Netty