davidradl commented on code in PR #52:
URL:
https://github.com/apache/flink-connector-http/pull/52#discussion_r3557681985
##########
flink-connector-http/src/main/java/org/apache/flink/connector/http/table/lookup/RequestFactoryBase.java:
##########
@@ -44,14 +44,12 @@
@Slf4j
public abstract class RequestFactoryBase implements HttpRequestFactory {
- public static final String DEFAULT_REQUEST_TIMEOUT_SECONDS = "30";
Review Comment:
the AI is saying the points have been addressed and raises a nit —
requestTimeoutOptionTest assertion
The catchThrowable approach in
http/src/test/java/org/apache/flink/connector/http/table/sink/HttpDynamicTableSinkFactoryTest.java:127
still passes vacuously when INSERT succeeds (returns null):
Throwable insertFailure =
catchThrowable(() -> tEnv.executeSql("INSERT INTO httpTimeout VALUES
(1)").await());
assertThat(insertFailure instanceof ValidationException)
.as("request.timeout must be a recognized option")
.isFalse();
insertFailure instanceof ValidationException evaluates to false whether
insertFailure is null (success) or any non-ValidationException throwable — so
the assertion never actually validates the contract in the success case. The
cleaner form that was raised in the earlier review round would be:
assertThatCode(() -> tEnv.executeSql("INSERT INTO httpTimeout VALUES
(1)").await())
.as("request.timeout must not raise ValidationException")
.doesNotThrowAnyExceptionOfType(ValidationException.class);
// or if INSERT may legitimately fail for runtime reasons:
Throwable t = catchThrowable(...);
if (t != null) {
assertThat(t).isNotInstanceOf(ValidationException.class);
}
The original
assertThatThrownBy(...).isNotInstanceOf(ValidationException.class) assumed an
exception always throws — replacing it with a null-safe check is the right
idea, but the current form inverts the test logic in a way that makes the
assertion always pass. This isn't a blocker (the intent is clear and the other
tests cover the real contract), but it's worth a one-line fix before merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
