gnodet-bot commented on code in PR #27433:
URL: https://github.com/apache/camel/pull/27433#discussion_r4346028999
##########
core/camel-core/src/test/java/org/apache/camel/support/jsse/SSLContextParametersTest.java:
##########
@@ -1063,20 +1069,17 @@ public void testSignatureSchemesFilter_postJDK26()
throws Exception {
int defaultSignatureSchemeNumber =
engine.getSSLParameters().getSignatureSchemes().length;
- // a filter cannot be applied as there are no default schemes to
filter, so the defaults are kept (null)
- // instead of an empty list (which would fail every handshake)
+ // empty filter - no includes means no schemes match (empty array)
FilterParameters filter = new FilterParameters();
- filter.getInclude().add(".*");
scp.setSignatureSchemesFilter(filter);
context = scp.createSSLContext(null);
engine = context.createSSLEngine();
socket = (SSLSocket) context.getSocketFactory().createSocket();
serverSocket = (SSLServerSocket)
context.getServerSocketFactory().createServerSocket();
- assertNull(engine.getSSLParameters().getSignatureSchemes());
- assertNull(socket.getSSLParameters().getSignatureSchemes());
- assertNull(serverSocket.getSSLParameters().getSignatureSchemes());
- filter.getInclude().clear();
+ assertThat(engine.getSSLParameters().getSignatureSchemes()).isEmpty();
+ assertThat(socket.getSSLParameters().getSignatureSchemes()).isEmpty();
+
assertThat(serverSocket.getSSLParameters().getSignatureSchemes()).isEmpty();
Review Comment:
🔧 **Assertion style (still open):** These three `assertThat(...).isEmpty()`
calls still use AssertJ. If `@Disabled` is removed (as recommended above),
switch to JUnit-only:
```suggestion
assertEquals(0,
engine.getSSLParameters().getSignatureSchemes().length);
assertEquals(0,
socket.getSSLParameters().getSignatureSchemes().length);
assertEquals(0,
serverSocket.getSSLParameters().getSignatureSchemes().length);
```
##########
core/camel-core/src/test/java/org/apache/camel/support/jsse/SSLContextParametersTest.java:
##########
@@ -29,11 +29,13 @@
import javax.net.ssl.SSLSocket;
import org.apache.camel.CamelContext;
+import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.EnabledForJreRange;
import org.junit.jupiter.api.condition.JRE;
import org.junit.jupiter.api.parallel.Isolated;
+import static org.assertj.core.api.Assertions.assertThat;
Review Comment:
🔧 **Assertion style mismatch (still open from previous review):** AssertJ is
still being imported, despite all reviewers (@oscerd, @davsclaus, and previous
bot review) requesting JUnit-only style in this file.
Since the test is now `@Disabled` anyway, the assertions in this block will
never run — but when the test is eventually re-enabled, the project rule
applies. If keeping the assertions, switch to JUnit:
```suggestion
import static org.junit.jupiter.api.Assertions.*;
```
(remove the `assertThat` import — `assertEquals(0,
engine.getSSLParameters().getSignatureSchemes().length)` is equivalent)
##########
core/camel-core/src/test/java/org/apache/camel/support/jsse/SSLContextParametersTest.java:
##########
@@ -1044,12 +1046,16 @@ public void testSignatureSchemesFilter() throws
Exception {
assertNull(serverSocket.getSSLParameters().getSignatureSchemes());
}
+ // This test only runs on JDK 26 onwards, where
SSLParameters.getSignatureSchemes() returns the JVM defaults
+ // instead of null. Camel supports JDK 21 and 25, which CI builds with, so
the test is disabled until a JDK 26+
+ // release is supported.
@Test
+ @Disabled("Requires JDK 26+ which Camel does not support yet (only JDK 21
and 25)")
Review Comment:
⚠️ **`@Disabled` and `@EnabledForJreRange` are redundant:** `@Disabled`
unconditionally skips the test regardless of JRE version. The
`@EnabledForJreRange(min = JRE.JAVA_26)` annotation on the next line is now
dead code — it will never be evaluated.
If the intent is to disable temporarily and re-enable when JDK 26 is
supported, consider using `@DisabledIfSystemProperty` tied to the JDK version
check, or simply leave `@EnabledForJreRange` as the sole gating condition (it
already guarantees the test won't run on JDK 25-). With `@Disabled`, you'll
need a manual code change to re-enable it later, which is easy to forget.
Suggested minimal fix — remove `@Disabled` and keep only the existing JRE
guard:
```suggestion
@Test
@EnabledForJreRange(min = JRE.JAVA_26)
```
--
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]
