Copilot commented on code in PR #13718:
URL: https://github.com/apache/skywalking/pull/13718#discussion_r2870034447
##########
docs/en/changes/changes.md:
##########
@@ -66,7 +66,7 @@
| Schedulers + others | ~24 | ~24 | Mostly
unchanged |
| **Total (OAP threads)** | **150+** | **~72** |
**~50% reduction, stable in high payload.** |
-* Replace PowerMock Whitebox with standard Java Reflection in
`server-library`, `server-core`, and `server-configuration` to support JDK 25+.
+* Replace PowerMock Whitebox with standard Java Reflection in
`server-library`, `server-core`, `server-configuration`, `analyzer`,
`server-cluster-plugin`, `server-receiver-plugin`, `server-fetcher-plugin` to
support JDK 25+.
Review Comment:
There is an extra space before `server-cluster-plugin` in the changelog
entry: `` `analyzer`, `server-cluster-plugin` `` has two spaces instead of one
after the comma.
```suggestion
* Replace PowerMock Whitebox with standard Java Reflection in
`server-library`, `server-core`, `server-configuration`, `analyzer`,
`server-cluster-plugin`, `server-receiver-plugin`, `server-fetcher-plugin` to
support JDK 25+.
```
##########
oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/K8sTagTest.java:
##########
@@ -237,29 +240,61 @@ public static Collection<Object[]> data() {
@SneakyThrows
@BeforeEach
public void setup() {
- Whitebox.setInternalState(KubernetesServices.class, "INSTANCE",
- Mockito.mock(KubernetesServices.class)
- );
- Whitebox.setInternalState(KubernetesPods.class, "INSTANCE",
- Mockito.mock(KubernetesPods.class)
- );
+ LoadingCache<KubernetesServices, Object> mockServicesCache =
mock(LoadingCache.class);
+ LoadingCache<ObjectID, Object> mockServiceByIDCache =
mock(LoadingCache.class);
+
+ Field servicesField =
KubernetesServices.class.getDeclaredField("services");
+ servicesField.setAccessible(true);
+ servicesField.set(KubernetesServices.INSTANCE, mockServicesCache);
+
+ Field serviceByIDField =
KubernetesServices.class.getDeclaredField("serviceByID");
+ serviceByIDField.setAccessible(true);
+ serviceByIDField.set(KubernetesServices.INSTANCE,
mockServiceByIDCache);
+
+ LoadingCache<String, Object> mockPodByIPCache =
mock(LoadingCache.class);
+ LoadingCache<ObjectID, Object> mockPodByObjectIDCache =
mock(LoadingCache.class);
+
+ Field podByIPField = KubernetesPods.class.getDeclaredField("podByIP");
+ podByIPField.setAccessible(true);
+ podByIPField.set(KubernetesPods.INSTANCE, mockPodByIPCache);
+
+ Field podByObjectIDField =
KubernetesPods.class.getDeclaredField("podByObjectID");
+ podByObjectIDField.setAccessible(true);
+ podByObjectIDField.set(KubernetesPods.INSTANCE,
mockPodByObjectIDCache);
- when(KubernetesServices.INSTANCE.list()).thenReturn(ImmutableList.of(
+ try {
+ when(mockServicesCache.get(any())).thenReturn(ImmutableList.of());
+ when(mockServiceByIDCache.get(any())).thenReturn(Optional.empty());
+ when(mockPodByIPCache.get(any())).thenReturn(Optional.empty());
+
when(mockPodByObjectIDCache.get(any())).thenReturn(Optional.empty());
+ } catch (Exception e) {
+ // ignore
+ }
+
+
when(mockServicesCache.get(KubernetesServices.INSTANCE)).thenReturn(ImmutableList.of(
mockService("nginx-service", "default", of("run", "nginx"),
"2.2.2.1"),
mockService("kube-state-metrics", "kube-system", of("run",
"kube-state-metrics"), "2.2.2.2")));
ImmutableList.of(
mockService("nginx-service", "default", of("run", "nginx"),
"2.2.2.1"),
mockService("kube-state-metrics", "kube-system", of("run",
"kube-state-metrics"), "2.2.2.2"))
- .forEach(svc ->
-
when(KubernetesServices.INSTANCE.findByID(ObjectID.builder().namespace(svc.getMetadata().getNamespace()).name(svc.getMetadata().getName()).build()))
- .thenReturn(Optional.of(svc))
- );
+ .forEach(svc -> {
+ try {
+
when(mockServiceByIDCache.get(ObjectID.builder().namespace(svc.getMetadata().getNamespace()).name(svc.getMetadata().getName()).build()))
+ .thenReturn(Optional.of(svc));
+ } catch (Exception e) {
+ // ignore
+ }
+ });
ImmutableList.of(
mockPod("my-nginx-5dc4865748-mbczh", "default", of("run",
"nginx"), "1.1.1.1"),
mockPod("kube-state-metrics-6f979fd498-z7xwx", "kube-system",
of("run", "kube-state-metrics"), "1.1.1.2"))
.forEach(pod -> {
-
when(KubernetesPods.INSTANCE.findByIP(pod.getStatus().getPodIP())).thenReturn(Optional.of(pod));
-
when(KubernetesPods.INSTANCE.findByObjectID(ObjectID.builder().name(pod.getMetadata().getName()).namespace(pod.getMetadata().getNamespace()).build())).thenReturn(Optional.of(pod));
+ try {
+
when(mockPodByIPCache.get(pod.getStatus().getPodIP())).thenReturn(Optional.of(pod));
+
when(mockPodByObjectIDCache.get(ObjectID.builder().name(pod.getMetadata().getName()).namespace(pod.getMetadata().getNamespace()).build())).thenReturn(Optional.of(pod));
+ } catch (Exception e) {
+ // ignore
+ }
});
Review Comment:
Several `catch (Exception e) { // ignore }` blocks silently swallow
exceptions from `when(mockCache.get(...)).thenReturn(...)` calls. While
`LoadingCache.get()` declares a checked `ExecutionException`, it will never
actually be thrown when stubbing with Mockito — the `when()` call is only
setting up expectations, not actually invoking the method. However, silently
catching and ignoring exceptions in test code is dangerous: if a real exception
occurs (e.g., `UnstubbedMethodInvocationException`, or wrong argument type),
the test setup would fail silently, leading to test failures with misleading
error messages further downstream rather than at the point of failure. Consider
either propagating the exception or at minimum logging/rethrowing it.
##########
oap-server/server-cluster-plugin/cluster-consul-plugin/src/test/java/org/apache/skywalking/oap/server/cluster/plugin/consul/ConsulCoordinatorTest.java:
##########
@@ -32,7 +32,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
-import org.powermock.reflect.Whitebox;
+import java.lang.reflect.Field;
Review Comment:
The `import java.lang.reflect.Field;` statement is placed between `org.*`
imports and the existing `java.util.*` imports block, breaking the established
import ordering convention used throughout the codebase (where `java.*` imports
are grouped separately from third-party imports). The `java.lang.reflect.Field`
import should be placed in the `java.*` import section rather than after
`org.mockito.*` imports.
```suggestion
```
##########
oap-server/server-receiver-plugin/skywalking-zabbix-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/zabbix/provider/ZabbixMetricsTest.java:
##########
@@ -45,7 +45,9 @@
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;
import org.mockito.junit.jupiter.MockitoSettings;
-import org.powermock.reflect.Whitebox;
+import java.lang.reflect.Field;
+import org.junit.jupiter.api.AfterEach;
+import org.mockito.MockedStatic;
Review Comment:
The newly added imports (`import java.lang.reflect.Field`, `import
org.junit.jupiter.api.AfterEach`, `import org.mockito.MockedStatic`) are placed
in the middle of other grouped imports, breaking the import ordering
convention. Specifically, `import java.lang.reflect.Field` should be in the
`java.*` section, `import org.junit.jupiter.api.AfterEach` should be adjacent
to the other `org.junit.jupiter.api.*` imports, and `import
org.mockito.MockedStatic` should be adjacent to the other `org.mockito.*`
imports.
##########
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/envoy/ClusterManagerMetricsAdapterTest.java:
##########
@@ -41,8 +42,11 @@ public class ClusterManagerMetricsAdapterTest {
@SneakyThrows
@BeforeEach
- public void setUp() {
- Whitebox.setInternalState(FieldsHelper.forClass(this.getClass()),
"initialized", false);
+ public void setUp() throws Exception {
Review Comment:
The `setUp()` method has both `@SneakyThrows` (from Lombok) and `throws
Exception` declared on the same method signature. These are redundant —
`@SneakyThrows` suppresses the need to declare checked exceptions, so the
`throws Exception` declaration is unnecessary. However, the original method
already had `@SneakyThrows` before this change, so the only new addition here
is `throws Exception`, which is the redundant part.
```suggestion
public void setUp() {
```
##########
oap-server/server-cluster-plugin/cluster-zookeeper-plugin/src/test/java/org/apache/skywalking/oap/server/cluster/plugin/zookeeper/ZookeeperCoordinatorTest.java:
##########
@@ -30,7 +30,7 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
-import org.powermock.reflect.Whitebox;
+import java.lang.reflect.Field;
Review Comment:
The `import java.lang.reflect.Field;` statement is placed after
`org.mockito.*` imports, breaking the established import ordering convention
where `java.*` imports are grouped together (as seen in the project's other
production and test files, e.g., `ConsulCoordinator.java`). The new import
should be placed within or adjacent to the existing `java.util.*` import block.
##########
oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/dsl/AnalyzerTest.java:
##########
@@ -86,6 +88,13 @@ public static void tearDown() {
MeterEntity.setNamingControl(null);
}
+ @org.junit.jupiter.api.AfterEach
+ public void tearDownEach() {
Review Comment:
The `@AfterEach` annotation is used as a fully-qualified annotation
(`@org.junit.jupiter.api.AfterEach`) rather than using the existing import
convention. The `@BeforeEach` is imported and used without full qualification
in the same file. The `@AfterEach` import should be added and used consistently
without full qualification.
--
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]
