add SdkAsyncHttpClientH1TestSuite #829
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There is a task associated with this. #655 I'd prefer to cover the integration test CI in a different PR. The Scala 3 build is failing for this PR. Some classloading issue for junit. |
This is a bit different, as these are not integration tests and do not require an AWS account
Agree
I will have a look |
I managed to reproduce locally, but only in java 8. I will keep digging... |
The error was where the important part is I tried to upgrade scala to 3.3.4, upgrade |
|
@jtjeferreira I merged #827 - can you rebase this and get it ready for merging? |
exceptions in toPekkoRequest should be wrapped in a future failed
SdkAsyncHttpClientH1TestSuite uses netty to simulate a server...
…ala 3 Error was: org.junit.platform.commons.JUnitException: ClassSelector [className = 'org.apache.pekko.stream.connectors.awsspi.RequestRunnerSpec$$anon$1', classLoader = java.net.URLClassLoader@79baaf07] resolution failed : ClassSelector [className = 'org.apache.pekko.stream.connectors.awsspi.RequestRunnerSpec$$anon$1', classLoader = java.net.URLClassLoader@79baaf07] resolution failed
jtjeferreira
force-pushed
the
SdkAsyncHttpClientH1TestSuite
branch
from
5ce5778 to
e593362
Compare
done |
jtjeferreira
commented
Sep 27, 2024
| @@ -53,9 +60,11 @@ class PekkoHttpClient(shutdownHandle: () => Unit, private[awsspi] val connection | |||
| lazy val runner = new RequestRunner() | |||
|
|
|||
| override def execute(request: AsyncExecuteRequest): CompletableFuture[Void] = { | |||
| val pekkoHttpRequest = toPekkoRequest(request.request(), request.requestContentPublisher()) | |||
There was a problem hiding this comment.
moving pekkoHttpRequest inside runner.run and changes below in RequestRunner.scala were needed to make test naughtyHeaderCharactersDoNotGetToServer pass.
The test is defined as:
@Test
public void naughtyHeaderCharactersDoNotGetToServer() {
String naughtyHeader = "foo\r\nbar";
assertThatThrownBy(() -> HttpTestUtils.sendRequest(client,
SdkHttpFullRequest.builder()
.uri(URI.create("https://localhost:" + server.port()))
.method(SdkHttpMethod.POST)
.appendHeader("h", naughtyHeader)
.build())
.join())
.hasCauseInstanceOf(Exception.class);
}and it was failing because toPekkoRequest is throwing an exception, but that exception should be inside a Future.failed.
See a734b17
| @@ -185,14 +194,20 @@ object PekkoHttpClient { | |||
| connectionPoolSettings.getOrElse(ConnectionPoolSettings(as)), | |||
| resolvedOptions | |||
| ) | |||
|
|
|||
| val connectionContext = | |||
| if (resolvedOptions.get(SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES).booleanValue()) | |||
There was a problem hiding this comment.
I had to add support for TRUST_ALL_CERTIFICATES option so the test suite can connect to an insecure server. Unfortunately, skipping cert validation is not so easy (see for example akka/akka-http#4042)
Comment on lines
+37
to
+38
| // Future.unit.flatMap(expr) is a scala 2.12 equivalent of Future.delegate(expr) | ||
| val result = Future.unit.flatMap(_ => runRequest()).flatMap { response => |
There was a problem hiding this comment.
open for suggestions
| @@ -44,7 +43,7 @@ | |||
|
|
|||
| import static org.junit.Assert.assertEquals; | |||
|
|
|||
| public class S3Test extends JUnitSuite { | |||
| public class S3Test { | |||
There was a problem hiding this comment.
AFAICT, using JUnitSuite from scalatest+junit is not needed, and we can remove dependency on "org.scalatestplus" %% "junit-4-13"
| @@ -15,7 +15,7 @@ | |||
| <appender-ref ref="STDOUT" /> | |||
| </root> | |||
|
|
|||
| <logger name="io.netty" level="trace" additivity="false"> | |||
| <logger name="io.netty" level="info" additivity="false"> | |||
There was a problem hiding this comment.
The SdkAsyncHttpClientH1TestSuite uses netty, and netty bootstrapping is very verbose, so I increased the log level
There was a problem hiding this comment.
See this comment why these changes were needed...
jtjeferreira
changed the title
pjfanning
approved these changes
Sep 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
depends on #827
There is a junit test suite in AWS SDK that might be useful to test the PekkoHttpClient