-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add test coverage for improvement multipart encoded mode in rest-client #1847
Add test coverage for improvement multipart encoded mode in rest-client #1847
Conversation
3f1bf8d
to
a77ecdd
Compare
Following jobs contain at least one flaky test: 'PR - Windows - JVM build - Latest Version' |
8232bc8
to
9013951
Compare
|
||
@ParameterizedTest | ||
@ValueSource(strings = { "HTML5", "RFC1738", "RFC3986" }) | ||
public void testMultipartEncodeMode(String encoderMode) { |
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 are not using encoderMode
which means you are not testing modes here
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.
Well, currently I am using by default HTML5 doing this:
@QuarkusApplication(properties = "test.properties") static RestService app = new RestService() .withProperty("quarkus.rest-client.multipart-post-encoder-mode", "HTML5");
Correct me if I am wrong
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.
that's fine if you are going to use encoderMode
variable in the future
...ctive/src/main/java/io/quarkus/ts/http/restclient/reactive/resources/EncodeModeResource.java
Outdated
Show resolved
Hide resolved
...ctive/src/main/java/io/quarkus/ts/http/restclient/reactive/resources/EncodeModeResource.java
Outdated
Show resolved
Hide resolved
Following jobs contain at least one flaky test: 'PR - Linux - JVM build - Latest Version' |
00b5751
to
a381294
Compare
@AfterAll | ||
public static void tearDown() throws IOException { | ||
serverCloseable.close(); | ||
vertx.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.
IMO you need to wait till future completes otherwise this process may be terminated prematurely by JUnit
|
||
}); | ||
|
||
return httpServer::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.
I think you are misunderstanding Closeable
. TBH I didn't see this before, but I think here the method reference httpServer::close
becomes same as
var closeable = new Closeable {
void close() {
httpServer.close();
}
};
return closeable;
which means you are not waiting for the Future.
550e70d
to
cab4ae9
Compare
CI is failing, please look into it @jcarranzan |
Thanks @rsvoboda but I will close this PR in favor of--> #1853 |
Summary
Cover this backport--> quarkusio/quarkus#39770 (Improve the multipart encoded mode handling in the rest client)
Please select the relevant options.
run tests
phrase in comment)Checklist: