Upgrade boot4 #118
Upgrade boot4 #118
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
📝 WalkthroughWalkthroughMajor platform upgrade: Java 8 → 17, Spring Boot → 4.0.1, broad JUnit4→JUnit5 migration across tests, servlet javax→jakarta updates, WebClient/OAuth2 WebFlux deps added, Jetty test handler refactor, new JUnit 5 test extensions/helpers, and multiple dependency/test framework adjustments. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (27)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/DefaultMetaServerProviderTest.java (1)
19-19: Complete the JUnit 5 migration by updating assertions.The import at line 19 still uses JUnit 4's
Assert.assertEquals, while lines 23-24 correctly import JUnit 5 annotations. This creates an inconsistent mix of JUnit 4 and JUnit 5 APIs.🔎 Proposed fix to complete the JUnit 5 migration
-import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals;Also applies to: 23-24
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApiTest.java (1)
21-21: Incomplete JUnit 5 migration: assertion import still uses JUnit 4.The test annotations have been migrated to JUnit 5 (
org.junit.jupiter.api.BeforeEach,org.junit.jupiter.api.Test), but the assertions are still imported from JUnit 4'sorg.junit.Assert. This inconsistency should be corrected by using JUnit 5's assertion class.🔎 Proposed fix
-import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*;apollo-client/src/test/java/com/ctrip/framework/apollo/spi/DefaultConfigFactoryFileCachePropertyTest.java (1)
19-19: Complete the JUnit 5 migration by updating the assertion import.The file still imports
assertSamefrom JUnit 4'sorg.junit.Assert, which is inconsistent with the migration to JUnit 5. This creates a dependency on both JUnit versions and contradicts the PR objective.🔎 Proposed fix
-import static org.junit.Assert.assertSame; +import static org.junit.jupiter.api.Assertions.assertSame;apollo-client/src/test/java/com/ctrip/framework/apollo/spring/boot/ApolloApplicationContextInitializerTest.java (1)
19-19: Incomplete JUnit 5 migration: assertions still use JUnit 4.The test class has been partially migrated to JUnit 5 (lifecycle annotations and imports updated), but the assertions are still imported from JUnit 4's
org.junit.Assert. This creates an inconsistent state. All assertions should be migrated to JUnit 5'sorg.junit.jupiter.api.Assertions.🔎 Proposed fix
-import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*;apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApiTest.java (1)
21-21: Complete the JUnit 5 migration by updating assertion imports.The test still imports JUnit 4's
Assertclass. For consistency with the JUnit 5 migration (as stated in the PR objectives), update to JUnit 5'sAssertionsclass.🔎 Proposed fix
-import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*;Note: All assertion method calls (
assertEquals,assertTrue) remain syntactically identical, so no changes to the test method bodies are required.apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContextTest.java (1)
18-18: Incomplete JUnit 5 migration: update assertion imports.The static import still references JUnit 4 assertions (
org.junit.Assert.*). This will cause compilation errors if JUnit 4 is removed from the classpath, which is the goal of this upgrade.🔎 Proposed fix
-import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*;apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
90-90: Replace Javaassertwith JUnit assertions.These lines use Java's
assertkeyword, which requires the-eaJVM flag to be enabled and may not execute during test runs. Use JUnit'sassertEqualsfor reliable test assertions.🔎 Proposed fix
- assert name.equals(result); + assertEquals(name, result);Apply this change to both lines 90 and 108.
Also applies to: 108-108
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/ClassLoaderUtilTest.java (1)
1-1: Critical: Build failure due to Java 21 compiler target mismatch.The pipeline failure indicates a build configuration issue affecting the entire project, not specific to this file. The error "invalid target release: 21" suggests the
maven-compiler-pluginis configured to target Java 21, but the build environment's JDK doesn't support it.Ensure either:
- The build environment uses JDK 21+, or
- The
maven-compiler-plugintarget is set to a version supported by the available JDKThis requires changes to the Maven POM configuration (likely in the parent POM or the apollo-core module POM), not to this test file.
apollo-client/src/test/java/com/ctrip/framework/apollo/ConfigServiceTest.java (1)
19-19: Incomplete JUnit 4 → JUnit 5 migration: Update assertion imports.The static import still references JUnit 4's
Assertclass while the lifecycle annotations have been migrated to JUnit 5. This inconsistency could cause compilation failures if JUnit 4 dependencies are removed (likely in a Spring Boot 4 upgrade) and violates migration best practices.🔎 Proposed fix
-import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals;apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientSystemPropertiesCompatibleTest.java (1)
51-56: Update assertions to JUnit 5 style.The test still uses JUnit 4's
Assert.assertEquals(). Update to JUnit 5'sAssertions.assertEquals()for consistency with the migration and other tests in this PR.🔎 Proposed fix
- Assert.assertEquals("test-3/cacheDir", + Assertions.assertEquals("test-3/cacheDir", this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR)); - Assert.assertEquals("test-3-secret", + Assertions.assertEquals("test-3-secret", this.environment.getProperty(ApolloClientSystemConsts.APOLLO_ACCESS_KEY_SECRET)); - Assert.assertEquals("https://test-3-config-service", + Assertions.assertEquals("https://test-3-config-service", this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CONFIG_SERVICE));apollo-client/src/test/java/com/ctrip/framework/apollo/spring/config/ConfigPropertySourceTest.java (1)
19-21: Complete the JUnit 5 migration by updating assertions.The test still uses JUnit 4 assertions from
org.junit.Assert. For a complete and consistent JUnit 5 migration, update these to useorg.junit.jupiter.api.Assertions.🔎 Proposed fix
-import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any;apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMessageProducerCompositeTest.java (1)
19-20: Complete the JUnit 5 migration by updating assertion imports.The test lifecycle annotations have been migrated to JUnit 5, but the assertions still use JUnit 4's
Assertclass. For consistency and to complete the migration, update the import to use JUnit 5'sAssertionsclass.🔎 Proposed fix to complete the JUnit 5 migration
import static org.mockito.Mockito.*; -import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/TxtConfigFileTest.java (1)
19-19: Complete the migration to JUnit 5 assertions.The test still imports JUnit 4 assertions (
org.junit.Assert.*), which is inconsistent with the JUnit 5 migration. For a complete and clean migration, update to use JUnit 5 assertions.🔎 Proposed fix to use JUnit 5 assertions
-import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/XmlConfigFileTest.java (1)
19-22: Incomplete JUnit 5 migration: Update assertion imports.These imports are from JUnit 4 (
org.junit.Assert). For a complete JUnit 5 migration, useorg.junit.jupiter.api.Assertionsinstead.🔎 Proposed fix
-import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue;apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientApplicationPropertiesCompatibleTest.java (1)
45-53: Update assertions to use JUnit 5 API.The test method still uses JUnit 4's
Assert.assertEquals. Update to JUnit 5'sAssertions.assertEqualsto complete the migration.🔎 Proposed fix to use JUnit 5 assertions
@Test public void testApplicationPropertiesCompatible() { - Assert.assertEquals("test-1/cacheDir", + Assertions.assertEquals("test-1/cacheDir", this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR)); - Assert.assertEquals("test-1-secret", + Assertions.assertEquals("test-1-secret", this.environment.getProperty(ApolloClientSystemConsts.APOLLO_ACCESS_KEY_SECRET)); - Assert.assertEquals("https://test-1-config-service", + Assertions.assertEquals("https://test-1-config-service", this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CONFIG_SERVICE)); }apollo-client/src/test/java/com/ctrip/framework/apollo/spring/util/BeanRegistrationUtilTest.java (1)
17-18: Incomplete JUnit 5 migration: assertions still reference JUnit 4.The test framework annotations have been migrated to JUnit 5, but the assertion imports still reference
org.junit.Assertfrom JUnit 4. This incomplete migration may cause issues if JUnit 4 dependencies are removed.🔎 Proposed fix to complete the migration
-import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue;apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java (1)
19-19: Complete the JUnit 5 migration by replacing JUnit 4 assertions.The test still imports
org.junit.Assert.*from JUnit 4. To complete the migration, replace it with JUnit 5'sAssertionsclass.🔎 Proposed fix
-import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*;apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/ByteUtilTest.java (1)
1-1: Upgrade maven-compiler-plugin to support Java 21.The project is configured to use Java 21 as the target/source, but
maven-compiler-pluginversion 3.10.1 only supports up to Java 18. Java 21 requiresmaven-compiler-plugin3.11.0 or later.Update the property in the root
pom.xml:<maven-compiler-plugin.version>3.11.0</maven-compiler-plugin.version>apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (2)
57-58: Remove JUnit 4@RunWith(Enclosed.class)annotation.The outer class still uses JUnit 4's
@RunWith(Enclosed.class), while inner classes use JUnit 5's@Nestedand@ExtendWith. This mixing of JUnit 4 and JUnit 5 APIs will cause test execution issues. Since the PR migrates to JUnit 5, remove the@RunWith(Enclosed.class)annotation.🔎 Proposed fix
-@RunWith(Enclosed.class) public class BootstrapConfigTest {
63-106: Replace JUnit 4 lifecycle annotations with JUnit 5 equivalents.All nested test classes use JUnit 4's
@BeforeClassand@AfterClassannotations, but are configured for JUnit 5 with@ExtendWith(SpringExtension.class)and@Nested. These JUnit 4 lifecycle methods will not execute in a JUnit 5 context.Additionally, the nested classes are no longer
static, but they containstaticlifecycle methods. In JUnit 5, non-static@Nestedclasses cannot have static@BeforeAll/@AfterAllmethods unless annotated with@TestInstance(Lifecycle.PER_CLASS).🔎 Recommended solution
For each nested class, apply one of these solutions:
Option 1 (Recommended): Make nested classes static and update lifecycle annotations:
@Nested @ExtendWith(SpringExtension.class) @SpringBootTest(classes = ConfigurationWithConditionalOnProperty.class) @DirtiesContext - class TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOn extends + static class TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOn extends AbstractSpringIntegrationTest { - @BeforeClass + @BeforeAll public static void beforeClass() throws Exception { // ... } - @AfterClass + @AfterAll public static void afterClass() throws Exception { // ... }Option 2: Keep classes non-static and add
@TestInstance:@Nested @ExtendWith(SpringExtension.class) + @TestInstance(TestInstance.Lifecycle.PER_CLASS) @SpringBootTest(classes = ConfigurationWithConditionalOnProperty.class) @DirtiesContext class TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOn extends AbstractSpringIntegrationTest { - @BeforeClass - public static void beforeClass() throws Exception { + @BeforeAll + public void beforeClass() throws Exception { // ... } - @AfterClass - public static void afterClass() throws Exception { + @AfterAll + public void afterClass() throws Exception { // ... }Apply this fix to all 10 nested test classes.
Also applies to: 119-160, 162-200, 202-237, 239-271, 274-307, 309-343, 346-377, 379-411, 414-453
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/PropertiesUtilTest.java (1)
1-1: Upgrade maven-compiler-plugin to support Java 21 compilation.The project is configured for Java 21 but uses maven-compiler-plugin 3.10.1, which was released before Java 21 support was added. Java 21 support is available in maven-compiler-plugin 3.11.0+.
In the root pom.xml, update:
<maven-compiler-plugin.version>3.10.1</maven-compiler-plugin.version>to:
<maven-compiler-plugin.version>3.11.0</maven-compiler-plugin.version>(or any newer version)
apollo-core/src/test/java/com/ctrip/framework/foundation/FoundationTest.java (1)
1-1: Critical: Java 21 target configured but CI/CD environment doesn't support it.The project's pom.xml specifies
<java.version>21</java.version>with maven-compiler-plugin configured to compile for Java 21, but the CI/CD pipeline (.github/workflows/build.yml) only tests against JDK 8, 11, and 17. This causes the "invalid target release: 21" error when the build runs on these earlier JDK versions.Configuration mismatch:
- pom.xml: java.version=21, maven-compiler-plugin source/target=${java.version}
- CI/CD matrix (.github/workflows/build.yml): Tests only JDK 8, 11, 17
Either update the CI/CD workflow to include JDK 21 in the test matrix, or downgrade the project's java.version to match the supported JDK versions (currently 8, 11, 17).
apollo-core/src/test/java/com/ctrip/framework/apollo/tracer/internals/DefaultMessageProducerManagerTest.java (1)
1-1: Critical: Java 21 target configured in pom.xml but not available in GitHub Actions build matrix.The pom.xml specifies
<java.version>21</java.version>with maven-compiler-plugin 3.10.1 properly configured to target Java 21. However, the GitHub Actions workflow (build.yml) only provides JDK versions 8, 11, and 17 in its test matrix. This causes the "invalid target release: 21" error when the build attempts to compile with a JDK that doesn't support Java 21 as a target.Add Java 21 to the build matrix in
.github/workflows/build.yml:matrix: jdk: [8, 11, 17, 21]Alternatively, revert the pom.xml
java.versionto 17 or another version available in the workflow if Java 21 support is not required.apollo-core/src/test/java/com/ctrip/framework/apollo/tracer/internals/NullMessageProducerManagerTest.java (1)
1-1: Critical: Pipeline failure due to Java 21 compilation issue.The build is failing with "Fatal error compiling: invalid target release: 21" for the Apollo Core module. This indicates that either:
- The maven-compiler-plugin is not configured to support Java 21
- The build environment doesn't have Java 21 available
Since Spring Boot 4.0.1 requires Java 21, ensure:
- The maven-compiler-plugin is configured with
<release>21</release>or appropriate source/target versions- The build environment has Java 21 installed
- All modules consistently specify Java 21 as the target
Based on the pipeline failure logs provided.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/JsonConfigFileTest.java (1)
19-22: Incomplete JUnit 5 migration: using JUnit 4 assertion imports.Similar to other test files in this PR, the assertions are imported from JUnit 4 (
org.junit.Assert) instead of JUnit 5 (org.junit.jupiter.api.Assertions).🔎 Proposed fix
-import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/PropertiesConfigFileTest.java (1)
19-22: Incomplete JUnit 5 migration: using JUnit 4 assertion imports.Consistent with other test files, assertions should be imported from
org.junit.jupiter.api.Assertions.🔎 Proposed fix
-import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/SimpleConfigTest.java (1)
34-37: Remove unused JUnit 4 imports.These imports are no longer needed after migration to JUnit 5.
🔎 Proposed fix
-import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; -import org.mockito.junit.MockitoJUnitRunner;
🤖 Fix all issues with AI agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java:
- Line 66: The test imports javax.servlet.http.HttpServletResponse which is
incompatible with Spring Boot 4.x; change the import to
jakarta.servlet.http.HttpServletResponse and update any usages in
RemoteConfigRepositoryTest (references to HttpServletResponse) so they use the
Jakarta Servlet API type instead of the javax one to avoid compile/runtime
errors.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventFactoryTest.java:
- Line 19: The test class ApolloClientMonitorEventFactoryTest still imports
JUnit 4 assertions; replace the static import "import static org.junit.Assert.*"
with the JUnit 5 assertions import (e.g. "import static
org.junit.jupiter.api.Assertions.*") and update any other test files flagged
(lines 22-23) to the same org.junit.jupiter.api.Assertions static import so the
existing assertXYZ calls (assertEquals, assertTrue, assertNotNull, etc.) compile
under JUnit 5.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spi/DefaultConfigFactoryManagerTest.java:
- Line 21: The test still imports JUnit 4's assertEquals; replace the static
import "import static org.junit.Assert.assertEquals;" with the JUnit 5
equivalent "import static org.junit.jupiter.api.Assertions.assertEquals;" (and
scan DefaultConfigFactoryManagerTest for any other org.junit.* imports to update
to org.junit.jupiter.api.* as needed) so the class uses JUnit 5 assertions
consistently with the migrated lifecycle annotations.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigPlaceholderTest.java:
- Line 21: The test imports JUnit 4's assertThrows (import static
org.junit.Assert.assertThrows) while the class uses JUnit 5 tests; replace that
import with JUnit 5's static import
(org.junit.jupiter.api.Assertions.assertThrows) and scan
JavaConfigPlaceholderTest and related imports to remove any leftover
org.junit.Assert usages so the test consistently uses JUnit 5 assertions.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XMLConfigAnnotationTest.java:
- Line 20: The test imports the non-existent JUnit4 method assertThrows from
org.junit.Assert; replace that import with the JUnit 5 static import
org.junit.jupiter.api.Assertions.assertThrows (or alternatively switch the test
to a JUnit4-compatible pattern), update the import statement in
XMLConfigAnnotationTest accordingly, and ensure the test run
configuration/dependencies include JUnit 5 if you choose the
org.junit.jupiter.api.Assertions route.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XmlConfigPlaceholderTest.java:
- Around line 19-20: The imports are inconsistent: remove JUnit 4 imports and
switch to JUnit 5 assertions by replacing both static imports (assertEquals and
assertThrows) from org.junit.Assert with static imports from
org.junit.jupiter.api.Assertions; update any usages in XmlConfigPlaceholderTest
to use the JUnit 5 assertions (no code changes needed if method names/signatures
match) and delete the old org.junit.Assert import to fix the compilation error.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java:
- Around line 24-25: Replace the JUnit4 assertion import with JUnit5's
Assertions: change the import that brings in org.junit.Assert.* to
org.junit.jupiter.api.Assertions.* so methods like assertEquals, assertTrue and
assertFalse used in ConfigUtilTest (the test methods annotated with @Test and
@AfterEach) resolve to JUnit 5; no other code changes are needed.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/util/parser/DateParserTest.java:
- Around line 19-20: The test imports use JUnit 4's org.junit.Assert for
assertEquals and assertThrows, but assertThrows is a JUnit 5 API; update the
static imports in DateParserTest to import assertEquals and assertThrows from
org.junit.jupiter.api.Assertions instead of org.junit.Assert and ensure the test
class uses JUnit 5 annotations/dependencies; specifically replace the static
imports for assertEquals and assertThrows to refer to Assertions
(org.junit.jupiter.api.Assertions) so the code compiles and aligns with the
JUnit 5 migration.
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java:
- Around line 76-90: The mockServerHandler currently casts Jetty Response to
HttpServletResponse and accidentally prints the Response object instead of the
method's response string; update the handle implementation to (1) stop casting
and use Jetty APIs: call response.getHeaders().put(HttpHeader.CONTENT_TYPE,
"text/plain;charset=UTF-8") and response.setStatus(statusCode), and (2) write
the provided response body string (rename the String parameter to avoid
shadowing the Response object, e.g., responseBody) using response.write(...)
with ByteBuffer.wrap(responseBody.getBytes(StandardCharsets.UTF_8)) and the
provided Callback so the correct text is returned.
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/MetaDomainTest.java:
- Around line 60-61: Restore the commented-out assertions in testGetValidAddress
so the test actually verifies behavior: uncomment the assertions that compare
validServer.trim() against MetaDomainConsts.getDomain(Env.FAT) and
MetaDomainConsts.getDomain(Env.UAT), and if they fail due to the Spring Boot
4.0.1 behavior change, update the expected values or adjust the test to
normalize/derive the expected domain (e.g., trim/normalize both sides or mock
the environment) rather than leaving the assertions removed.
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/NetUtilTest.java:
- Line 40: The test testPingUrlWithStatusCode200 asserts the wrong outcome:
NetUtil.pingUrl(...) is expected to return true for HTTP 200 responses
(NetUtil.pingUrl handles 200-399 as success). Update the assertion in
testPingUrlWithStatusCode200 from assertFalse(...) to assertTrue(...) so it
matches NetUtil.pingUrl's contract and verifies a 200 response is treated as a
successful ping.
In
@apollo-mockserver/src/test/java/com/ctrip/framework/apollo/mockserver/ApolloMockServerSpringIntegrationTest.java:
- Around line 42-43: The test currently statically imports assertThrows from
org.junit.Assert which is JUnit 5 API mismatch and breaks compilation; update
the static import to import assertThrows from org.junit.jupiter.api.Assertions
(or replace usages of assertThrows with an equivalent JUnit 4 pattern), e.g.,
change the static import line that references assertThrows to use
org.junit.jupiter.api.Assertions.assertThrows so the
ApolloMockServerSpringIntegrationTest compiles and runs with JUnit 5 assertions.
In
@apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/ReleaseOpenApiServiceTest.java:
- Line 19: The test class imports JUnit 4's org.junit.Assert.* but uses JUnit
5's assertThrows in ReleaseOpenApiServiceTest, causing compilation errors;
update the static import to org.junit.jupiter.api.Assertions.* (or explicitly
import assertThrows and other assertion methods used) so methods like
assertThrows used in tests (and other assertions such as
assertEquals/assertNotNull if present) resolve to JUnit 5 implementations in the
ReleaseOpenApiServiceTest class.
In @pom.xml:
- Around line 173-177: Replace the Apache HttpClient 4.x dependency with the
HttpClient 5.x coordinates: remove org.apache.httpcomponents:httpclient:4.5.14
and add the 5.x artifact (org.apache.hc.client5:httpclient5) at version 5.1 or
newer, ensuring any required companion modules (httpcore5) are present so
Spring's HttpComponentsClientHttpRequestFactory/RestTemplate integrations work
with Spring Boot 4.0.1; update the dependency element for artifactId/ groupId/
version accordingly and verify code imports/usages still compile against the 5.x
API.
🟠 Major comments (24)
apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java-26-28 (1)
26-28: Complete the JUnit 5 migration by updating assertions.The lifecycle annotations have been correctly migrated to JUnit 5. However, the static assertion imports (lines 36-48) still reference
org.junit.Assertfrom JUnit 4. For a complete migration, these should be updated toorg.junit.jupiter.api.Assertions.🔎 Proposed fix to complete the JUnit 5 migration
-import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue;Also applies to: 55-55, 64-64
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientExceptionMonitorApiTest.java-19-22 (1)
19-22: Complete the JUnit 5 migration by updating assertion imports.The JUnit 5 migration is incomplete. While lifecycle and test annotations were updated, the assertion imports at line 19 still use JUnit 4's
org.junit.Assert. For consistency and to leverage JUnit 5's improved assertion messages, migrate toorg.junit.jupiter.api.Assertions.🔎 Proposed fix to complete the JUnit 5 migration
-import static org.junit.Assert.*; +import static org.junit.jupiter.api.Assertions.*; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test;apollo-client/src/test/java/com/ctrip/framework/apollo/spring/config/ConfigPropertySourceTest.java-38-38 (1)
38-38: Remove unused JUnit 4 imports.Lines 38 and 41 contain imports from JUnit 4 and the old Mockito runner that are no longer used after migrating to
@ExtendWith(MockitoExtension.class)on line 45.🔎 Proposed fix
-import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; -import org.mockito.junit.MockitoJUnitRunner; import org.mockito.junit.jupiter.MockitoExtension;Also applies to: 41-41
apollo-core/src/test/java/com/ctrip/framework/apollo/core/internals/LegacyMetaServerProviderTest.java-19-19 (1)
19-19: Complete the JUnit 5 migration for assertions.The static import still uses JUnit 4's
Assert.assertEquals. For consistency with the JUnit 5 migration, update toAssertions.assertEquals.🔎 Proposed fix to complete the JUnit 5 migration
-import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals;apollo-core/src/test/java/com/ctrip/framework/apollo/tracer/TracerTest.java-27-28 (1)
27-28: Complete the JUnit 5 migration by updating assertion imports.The test lifecycle has been migrated to JUnit 5 (
@BeforeEach,@Test), but the assertion imports still reference JUnit 4'sorg.junit.Assert. While this works due to backward compatibility, it creates an inconsistent migration.🔎 Proposed fix to complete the migration
-import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/XmlConfigFileTest.java-39-42 (1)
39-42: Remove unused JUnit 4 imports.
RunWithandMockitoJUnitRunnerare JUnit 4 artifacts that are no longer used since the class now uses@ExtendWith(MockitoExtension.class).🔎 Proposed fix
-import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; -import org.mockito.junit.MockitoJUnitRunner;apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientApplicationPropertiesCompatibleTest.java-19-31 (1)
19-31: Complete the JUnit 5 migration by removing unused JUnit 4 imports.The migration is incomplete. Lines 22, 25, and 31 contain unused JUnit 4 imports that should be removed and replaced with JUnit 5 equivalents.
🔎 Proposed fix to complete the import migration
package com.ctrip.framework.apollo.config.data.system; import com.ctrip.framework.apollo.core.ApolloClientSystemConsts; import com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer; import org.junit.jupiter.api.AfterEach; -import org.junit.Assert; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.test.context.ActiveProfiles; import org.springframework.test.context.junit.jupiter.SpringExtension; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;Committable suggestion skipped: line range outside the PR's diff.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java-55-55 (1)
55-55: Remove unused JUnit 4 imports.After migrating to JUnit 5, the imports for
org.junit.runner.RunWith(line 55) andorg.mockito.junit.MockitoJUnitRunner(line 60) are no longer needed.🔎 Proposed fix
-import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.invocation.InvocationOnMock; -import org.mockito.junit.MockitoJUnitRunner; import org.mockito.junit.jupiter.MockitoExtension;Also applies to: 60-60
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java-68-68 (1)
68-68: Remove commented-out duplicate annotation.Line 68 contains a commented-out duplicate of the active annotation on line 69. This leftover from the migration should be removed.
🔎 Proposed fix
-//@ExtendWith(MockitoExtension.class) @ExtendWith(MockitoExtension.class) public class RemoteConfigLongPollServiceTest {apollo-client/src/test/java/com/ctrip/framework/apollo/spring/util/BeanRegistrationUtilTest.java-26-26 (1)
26-26: Remove unused JUnit 4 imports.These JUnit 4 imports are no longer used after migrating to JUnit 5's
@ExtendWith(MockitoExtension.class).🔎 Proposed fix to remove dead imports
-import org.junit.runner.RunWith; import org.mockito.InjectMocks; import org.mockito.Mockito; -import org.mockito.junit.MockitoJUnitRunner; import org.mockito.junit.jupiter.MockitoExtension;Also applies to: 29-29
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java-45-50 (1)
45-50: Replace deprecatedinitMockswith modern Mockito initialization.
MockitoAnnotations.initMocks(this)was deprecated in Mockito 3.4.0. Use@ExtendWith(MockitoExtension.class)at the class level for cleaner integration with JUnit 5.🔎 Proposed fix
Add the extension at the class level and remove manual initialization:
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class DefaultApolloClientMetricsExporterFactoryTest {Then remove the manual initialization from setUp:
@BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); MockInjector.setInstance(ConfigUtil.class, configUtil); factory = new DefaultApolloClientMetricsExporterFactory(); }Committable suggestion skipped: line range outside the PR's diff.
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/ByteUtilTest.java-21-21 (1)
21-21: Incomplete JUnit 5 migration: Assert import not updated.While the
@Testimport was correctly updated to JUnit 5, line 20 still importsorg.junit.Assertfrom JUnit 4. For a complete migration, you should also update toorg.junit.jupiter.api.Assertionsand adjust the assertion calls accordingly.🔎 Proposed fix to complete the JUnit 5 migration
import com.ctrip.framework.apollo.core.utils.ByteUtil; -import org.junit.Assert; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; public class ByteUtilTest { @Test public void testInt3() { - Assert.assertEquals((byte)0, ByteUtil.int3(0)); - Assert.assertEquals((byte)0, ByteUtil.int3(1)); + Assertions.assertEquals((byte)0, ByteUtil.int3(0)); + Assertions.assertEquals((byte)0, ByteUtil.int3(1)); } @Test public void testInt2() { - Assert.assertEquals((byte)0, ByteUtil.int2(0)); - Assert.assertEquals((byte)0, ByteUtil.int2(1)); + Assertions.assertEquals((byte)0, ByteUtil.int2(0)); + Assertions.assertEquals((byte)0, ByteUtil.int2(1)); } @Test public void testInt1() { - Assert.assertEquals((byte)0, ByteUtil.int1(0)); - Assert.assertEquals((byte)0, ByteUtil.int1(1)); + Assertions.assertEquals((byte)0, ByteUtil.int1(0)); + Assertions.assertEquals((byte)0, ByteUtil.int1(1)); } @Test public void testInt0() { - Assert.assertEquals((byte)0, ByteUtil.int0(0)); - Assert.assertEquals((byte)1, ByteUtil.int0(1)); + Assertions.assertEquals((byte)0, ByteUtil.int0(0)); + Assertions.assertEquals((byte)1, ByteUtil.int0(1)); } @Test public void testToHexString() { - Assert.assertEquals("", ByteUtil.toHexString(new byte[] {})); - Assert.assertEquals("98", ByteUtil.toHexString(new byte[] {(byte)-104})); + Assertions.assertEquals("", ByteUtil.toHexString(new byte[] {})); + Assertions.assertEquals("98", ByteUtil.toHexString(new byte[] {(byte)-104})); } }Committable suggestion skipped: line range outside the PR's diff.
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java-26-26 (1)
26-26: Complete the JUnit 5 migration for@Ignoreannotation.The
@Ignoreannotation is from JUnit 4. For JUnit 5, use@Disabledinstead to maintain consistency with the rest of the migration.🔎 Proposed fix
-import org.junit.Ignore; +import org.junit.jupiter.api.Disabled; import org.junit.Rule;-@Ignore("Stress test") +@Disabled("Stress test") @ExtendWith(SpringExtension.class)Also applies to: 35-35
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java-30-30 (1)
30-30: Remove unused JUnit 4 imports.These imports are no longer needed after migrating to JUnit 5 with
@ExtendWith(SpringExtension.class).🔎 Proposed fix
-import org.junit.runner.RunWith; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.test.context.junit.jupiter.SpringExtension; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;Also applies to: 33-33
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java-27-27 (1)
27-27: Migrate from JUnit 4 @rule to JUnit 5 @ExtendWith for JUnitPerf.The test file mixes JUnit 4 and JUnit 5. Replace the
@Ruleannotation with JUnit 5's extension mechanism by:
- Adding the JUnit 5 JUnitPerf dependency:
junitperf-junit5:1.37.0- Replacing line 40-41
@Rule public JUnitPerfRulewith@ExtendWith(JUnitPerfInterceptor.class)on the class- Importing
JUnitPerfInterceptorfromcom.github.noconnor.junitperf.JUnitPerfInterceptor- Removing
import org.junit.Rule;and other unused JUnit 4 imports (RunWith,SpringJUnit4ClassRunner)- Also replace
@Ignore(line 26) with@Disabledfromorg.junit.jupiter.apifor consistencyThe project already uses JUnit 5 exclusively elsewhere; this aligns this stress test with the rest of the codebase.
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java-25-27 (1)
25-27: Clean up unused JUnit 4 imports after migration.Several JUnit 4 imports remain after the migration to JUnit 5:
org.junit.AfterClass→ should beorg.junit.jupiter.api.AfterAllorg.junit.BeforeClass→ should beorg.junit.jupiter.api.BeforeAllorg.junit.Assert→ considerorg.junit.jupiter.api.Assertions(though JUnit 4 Assert still works)org.junit.experimental.runners.Enclosed→ no longer needed with@Nestedorg.junit.runner.RunWith→ no longer needed with@ExtendWithorg.springframework.test.context.junit4.SpringJUnit4ClassRunner→ replaced bySpringExtension🔎 Proposed cleanup
-import org.junit.AfterClass; -import org.junit.Assert; -import org.junit.BeforeClass; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import org.junit.experimental.runners.Enclosed; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.runner.RunWith; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;Then update all
Assert.calls toAssertions.throughout the file.Also applies to: 30-32, 45-45
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java-108-116 (1)
108-116: Re-enable commented-out test assertions.Three test methods have all assertions commented out, which disables testing without removing the code. They will pass without verifying any behavior, creating false test coverage.
Either fix the underlying issues preventing these assertions from passing, or remove the test methods if they're no longer valid.
Lines affected: 108-116, 155-159, 195-199
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/AbstractOpenApiServiceTest.java-32-34 (1)
32-34: Remove unused JUnit 4 imports.Lines 32-34 contain unused JUnit 4 imports (
RunWith,MockitoJUnitRunner) that should be removed since the class now uses JUnit 5's@ExtendWith(MockitoExtension.class).Proposed fix to remove unused imports
import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.runner.RunWith; import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; import org.mockito.junit.jupiter.MockitoExtension;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/JsonConfigFileTest.java-31-33 (1)
31-33: Remove unused JUnit 4 imports.These JUnit 4 imports are no longer needed after migrating to JUnit 5 with
@ExtendWith(MockitoExtension.class).🔎 Proposed fix
-import org.junit.runner.RunWith; import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/PropertiesConfigFileTest.java-39-42 (1)
39-42: Remove unused JUnit 4 imports.These imports are no longer needed after migration to JUnit 5.
🔎 Proposed fix
-import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; -import org.mockito.junit.MockitoJUnitRunner;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java-71-76 (1)
71-76: Remove unused JUnit 4 imports.These imports are no longer needed after migration to JUnit 5.
🔎 Proposed fix
-import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.invocation.InvocationOnMock; -import org.mockito.junit.MockitoJUnitRunner; import org.mockito.junit.jupiter.MockitoExtension;apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.java-45-62 (1)
45-62: Test purpose has changed from verifying deprecated environment variable compatibility.The original test (now commented out) verified that deprecated environment variables (e.g.,
DEPRECATED_APOLLO_CACHE_DIR_ENVIRONMENT_VARIABLES) were properly mapped to standard properties. The new implementation directly sets standard property keys viaDynamicPropertySource, which no longer tests the backward compatibility behavior described in the test class name and method name (testEnvironmentVariablesCompatible).Consider either:
- Restoring the original test logic to verify deprecated environment variable compatibility, or
- Renaming the test class/method to reflect that it now tests direct property configuration rather than environment variable compatibility.
apollo-core/src/test/java/com/ctrip/framework/test/tools/AloneClassLoader.java-38-55 (1)
38-55: Address error handling in the classpath fallback mechanism.The
getClasspathUrls()method has critical robustness issues for test isolation:
- The empty catch block silently swallows exceptions, making it impossible to debug if the fallback fails during test execution.
- The
getResources("")fallback is fragile and may not capture all classpath entries across different Java versions or containerized environments.- If both branches fail, the method returns an empty URL array, silently breaking test isolation without any indication to developers.
Add exception logging and verify that the method returns valid URLs. Consider logging a warning when the fallback is used or when the result is empty to aid in debugging test isolation issues.
Suggested improvements
private static URL[] getClasspathUrls() { ClassLoader cl = Thread.currentThread().getContextClassLoader(); if (cl instanceof URLClassLoader) { return ((URLClassLoader) cl).getURLs(); } - // Java 9+ fallback（安全） + // Java 9+ safe fallback when URLClassLoader is not available List<URL> urls = new ArrayList<>(); try { var resources = cl.getResources(""); while (resources.hasMoreElements()) { urls.add(resources.nextElement()); } - } catch (Exception ignored) { + } catch (Exception e) { + System.err.println("Warning: Failed to enumerate classpath URLs: " + e.getMessage()); } + if (urls.isEmpty()) { + System.err.println("Warning: No classpath URLs found, test isolation may not work correctly"); + } return urls.toArray(new URL[0]); }apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitorTest.java-52-52 (1)
52-52: Replace deprecatedMockitoAnnotations.initMocks()with@ExtendWith(MockitoExtension.class).
MockitoAnnotations.initMocks(this)is deprecated in Mockito 5.20.0 (used by Spring Boot 4.0.1). Instead of manually callingopenMocks(), the cleanest approach for JUnit 5 is to add@ExtendWith(MockitoExtension.class)to the test class and remove the manual initialization, which aligns with how other tests in the codebase have been modernized.🔎 Recommended refactor
+import org.mockito.junit.jupiter.MockitoExtension; +import org.junit.jupiter.api.extension.ExtendWith; + +@ExtendWith(MockitoExtension.class) public class DefaultConfigMonitorTest { @Mock private ApolloClientExceptionMonitorApi exceptionMonitorApi; ... @BeforeEach public void setUp(){ - MockitoAnnotations.initMocks(this); when(monitorContext.getExceptionApi()).thenReturn(exceptionMonitorApi); ... } @AfterEach public void tearDown() throws Exception { MockInjector.reset(); }Alternatively, if manual initialization is preferred, replace
initMocks(this)withopenMocks(this)and handle the returnedAutoCloseable:@BeforeEach public void setUp(){ - MockitoAnnotations.initMocks(this); + MockitoAnnotations.openMocks(this); when(monitorContext.getExceptionApi()).thenReturn(exceptionMonitorApi); ... }Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (10)
apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientSystemPropertiesCompatibleTest.java-21-30 (1)
21-30: Incomplete JUnit 5 migration – leftover JUnit 4 imports and assertions.The migration adds JUnit 5 imports but leaves behind unused JUnit 4 imports:
- Line 22:
org.junit.Assertshould be replaced withorg.junit.jupiter.api.Assertions- Line 25:
org.junit.runner.RunWithis unused- Line 30:
org.springframework.test.context.junit4.SpringJUnit4ClassRunneris unusedThe sibling test
ApolloClientEnvironmentVariablesCompatibleTest.javacorrectly usesAssertions.assertEqualsfrom JUnit 5.🔎 Proposed fix
import com.ctrip.framework.apollo.core.ApolloClientSystemConsts; import com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer; import org.junit.jupiter.api.AfterEach; -import org.junit.Assert; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.test.context.junit.jupiter.SpringExtension; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;apollo-client/src/test/java/com/ctrip/framework/apollo/spi/DefaultConfigRegistryTest.java-20-20 (1)
20-20: Complete the JUnit 5 migration for assertion imports.The import still references JUnit 4's
org.junit.Assert.assertNull, which is inconsistent with the JUnit 5 migration applied to the rest of the file.🔎 Proposed fix
-import static org.junit.Assert.assertNull; +import static org.junit.jupiter.api.Assertions.assertNull;apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java-34-34 (1)
34-34: Remove unused import.The
ParserExceptionimport is not used anywhere in this test file.🔎 Proposed fix
-import com.ctrip.framework.apollo.util.parser.ParserException;apollo-core/src/test/java/com/ctrip/framework/apollo/core/MetaDomainTest.java-20-20 (1)
20-20: Remove unused import.The
assertNotEqualsimport is unused since the only assertions using it (lines 60-61) are commented out.🔎 Proposed fix
-import static org.junit.Assert.assertNotEquals;apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java-273-277 (1)
273-277: Remove redundant commented-out annotations.Several test classes have commented-out
@ExtendWith(SpringExtension.class)annotations immediately followed by the same annotation uncommented. This is confusing and appears to be leftover from debugging or refactoring.🔎 Proposed cleanup
-// @ExtendWith(SpringExtension.class) @Nested @SpringBootTest(classes = ConfigurationWithoutConditionalOnProperty.class) @DirtiesContext @ExtendWith(SpringExtension.class) class TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOff extendsApply to all affected classes (lines 273, 310, 345, 380, 413).
Also applies to: 310-313, 345-349, 380-383, 413-417
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/SimpleConfigTest.java-19-19 (1)
19-19: Incomplete JUnit 5 migration: using JUnit 4 assertion import.The assertion import should be from JUnit 5.
🔎 Proposed fix
-import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals;apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java-19-20 (1)
19-20: Incomplete JUnit 5 migration: using JUnit 4 assertion imports.The test uses JUnit 5 annotations (
@BeforeEach,@Testfromorg.junit.jupiter.api) but imports assertions from JUnit 4 (org.junit.Assert). For consistency and proper JUnit 5 usage, import assertions fromorg.junit.jupiter.api.Assertions.🔎 Proposed fix
-import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertThrows; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows;apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java-19-24 (1)
19-24: Incomplete JUnit 5 migration: using JUnit 4 assertion imports.Assertions should be imported from
org.junit.jupiter.api.Assertionsfor JUnit 5 consistency.🔎 Proposed fix
-import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue;apollo-core/pom.xml-73-77 (1)
73-77: Missing<scope>test</scope>on jakarta.servlet-api dependency.The
jakarta.servlet-apidependency is placed in the test dependencies section but lacks the<scope>test</scope>declaration. This will cause it to be included in the compile scope by default, potentially affecting downstream consumers.🔎 Proposed fix
<dependency> <groupId>jakarta.servlet</groupId> <artifactId>jakarta.servlet-api</artifactId> <version>6.1.0</version> + <scope>test</scope> </dependency>apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/AppOpenApiServiceTest.java-33-33 (1)
33-33: Remove unusedTimeoutExceptionimport. This import on line 33 is not referenced anywhere in the test file.
...a/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventFactoryTest.java
Outdated
Show resolved
Hide resolved
apollo-client/src/test/java/com/ctrip/framework/apollo/spi/DefaultConfigFactoryManagerTest.java
Outdated
Show resolved
Hide resolved
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigPlaceholderTest.java
Outdated
Show resolved
Hide resolved
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XMLConfigAnnotationTest.java
Outdated
Show resolved
Hide resolved
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XmlConfigPlaceholderTest.java
Outdated
Show resolved
Hide resolved
apollo-core/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java
Outdated
Show resolved
Hide resolved
apollo-core/src/test/java/com/ctrip/framework/apollo/core/MetaDomainTest.java
Outdated
Show resolved
Hide resolved
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/NetUtilTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/com/ctrip/framework/apollo/mockserver/ApolloMockServerSpringIntegrationTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/com/ctrip/framework/apollo/openapi/client/service/ReleaseOpenApiServiceTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContextTest.java (1)
42-61: Complete the JUnit 5 migration by using@ExtendWith(MockitoExtension.class).The test class uses
@Mockannotations but still relies on the deprecatedMockitoAnnotations.initMocks(this)method. For a proper JUnit 5 + Mockito integration, use the@ExtendWith(MockitoExtension.class)annotation on the test class, which will automatically initialize mocks and eliminate the need for manual setup.♻️ Proposed refactor to complete JUnit 5 migration
Add the extension at the class level:
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class ApolloClientMonitorContextTest {Remove the deprecated initialization:
- @BeforeEach - public void setUp() { - MockitoAnnotations.initMocks(this); + @BeforeEach + public void setUp() { monitorContext = new ApolloClientMonitorContext(); }apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
89-89: Replace Javaassertwith JUnit assertions.Lines 89 and 107 use Java's
assertkeyword instead of JUnit assertions. Java assertions require the-eaflag to be enabled and don't provide informative test failure messages. This is inconsistent with the rest of the test file.✅ Proposed fix
- assert name.equals(result); + assertEquals(name, result);Apply this change to both lines 89 and 107.
Also applies to: 107-107
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/ClusterOpenApiServiceTest.java (1)
107-120: Improve formatting of assertThrows lambda expression.Similar to the previous test method, the
assertThrowsusage is functionally correct but has formatting issues.♻️ Proposed fix for consistent formatting
@Test public void testCreateClusterWithError() throws Exception { String someCluster = "someCluster"; String someCreatedBy = "someCreatedBy"; OpenClusterDTO clusterDTO = new OpenClusterDTO(); clusterDTO.setAppId(someAppId); clusterDTO.setName(someCluster); clusterDTO.setDataChangeCreatedBy(someCreatedBy); when(statusLine.getStatusCode()).thenReturn(400); - assertThrows(RuntimeException.class,()-> - clusterOpenApiService.createCluster(someEnv, clusterDTO)); + assertThrows(RuntimeException.class, () -> + clusterOpenApiService.createCluster(someEnv, clusterDTO)); }apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (2)
53-54: Remove JUnit 4@RunWith(Enclosed.class)annotation.JUnit 5 natively supports
@Nestedtest classes without requiring a custom runner. The@RunWith(Enclosed.class)annotation is a JUnit 4 construct and should be removed.🔧 Proposed fix
-@RunWith(Enclosed.class) public class BootstrapConfigTest {
81-102: Migrate lifecycle methods to JUnit 5.The
@BeforeClassand@AfterClassannotations are JUnit 4 constructs. For JUnit 5, these must be replaced with@BeforeAlland@AfterAll. Additionally, these methods must bestaticin JUnit 5 just as they were in JUnit 4.🔧 Proposed fix
- @BeforeClass + @BeforeAll public static void beforeClass() throws Exception { doSetUp(); System.setProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_ENABLED, "true"); mockedConfig = mock(Config.class); when(mockedConfig.getPropertyNames()).thenReturn(Sets.newHashSet(TEST_BEAN_CONDITIONAL_ON_KEY, someProperty)); when(mockedConfig.getProperty(eq(TEST_BEAN_CONDITIONAL_ON_KEY), Mockito.nullable(String.class))).thenReturn(Boolean.TRUE.toString()); when(mockedConfig.getProperty(eq(someProperty), Mockito.nullable(String.class))).thenReturn(someValue); mockConfig(someAppId, ConfigConsts.NAMESPACE_APPLICATION, mockedConfig); } - @AfterClass + @AfterAll public static void afterClass() throws Exception { System.clearProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_ENABLED); doTearDown(); }Note: This same change is required for all
@BeforeClassand@AfterClassannotations throughout the file (lines 125, 143, 168, 183, 208, 222, 245, 255, 280, 291, 316, 326, 352, 364, 386, 395, 417, 429).
🤖 Fix all issues with AI agents
In
@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.java:
- Around line 60-89: The test testEnvironmentVariablesCompatible no longer
verifies deprecated environment variables mapping because it sets new properties
directly; restore the original compatibility check by using
SystemLambda.withEnvironmentVariable to set
DEPRECATED_APOLLO_CACHE_DIR_ENVIRONMENT_VARIABLES,
DEPRECATED_APOLLO_ACCESS_KEY_SECRET_ENVIRONMENT_VARIABLES, and
DEPRECATED_APOLLO_CONFIG_SERVICE_ENVIRONMENT_VARIABLES (from
ApolloClientSystemConsts) and execute assertions that
environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR),
APOLLO_ACCESS_KEY_SECRET, and APOLLO_CONFIG_SERVICE return the expected values,
or if deprecation support was intentionally removed, rename
testEnvironmentVariablesCompatible to reflect it only validates direct property
injection; update the test body accordingly and remove the current assertions
that bypass deprecated env var mapping.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepositoryTest.java:
- Around line 153-154: The assertEquals call in LocalFileConfigRepositoryTest
currently uses the wrong parameter order; swap the expected and actual arguments
so the call becomes assertEquals(someProperties, result,
"LocalFileConfigRepository's properties should be the same as fallback repo's
when there is no local cache") — keep the same message and only flip the
variables `result` and `someProperties` in the assertEquals invocation to match
JUnit 5's expected, actual signature.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java:
- Line 83: Remove the unused lenient stubs from the test setup: in the setUp()
method delete the lenient() stubbing calls for serviceDTO.getHomepageUrl() and
configServiceLocator.getConfigServices(), since neither
serviceDTO.getHomepageUrl() nor configServiceLocator.getConfigServices() are
invoked by any tests in RemoteConfigLongPollServiceTest and the lenient()
markers are redundant; ensure no other tests rely on those mocks before
committing the removal.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java:
- Line 66: The test imports the old javax.servlet API which is incompatible with
Spring Boot 4.x; in RemoteConfigRepositoryTest replace the
javax.servlet.http.HttpServletResponse import with the Jakarta equivalent
(jakarta.servlet.http.HttpServletResponse) so the test compiles and runs under
Spring Boot 4; search for the import statement in RemoteConfigRepositoryTest and
update it, and run the test suite to ensure no other javax.* servlet references
remain.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java:
- Around line 104-112: The test method test() in BootstrapConfigTest has all
assertions commented out, disabling validation; re-enable or restore assertions
for testBean, testBean.execute(), config, and someInjectedValue by uncommenting
them (or replacing with updated assertions) and ensure the test setup provides
the mockedConfig and injected values before assertions; specifically check the
test method, the testBean field, the config variable (mockedConfig), and
someInjectedValue to confirm they are initialized/mocked in the
@Before/@BeforeEach or with proper test annotations so the assertions can run
and validate expected behavior.
- Around line 191-195: The test method test() in BootstrapConfigTest has its
assertions commented out; restore the two assertions by uncommenting or
re-adding Assertions.assertNotNull(testBean) and
Assertions.assertTrue(testBean.execute()) inside the test() method, and ensure
the testBean field used by the test is properly provided (e.g., an @Autowired or
@MockBean instance or a test fixture setup) so the assertions can run reliably;
also make sure org.junit.jupiter.api.Assertions is imported if missing.
- Around line 151-155: The test method test() in BootstrapConfigTest has its
assertions commented out so it performs no checks; restore the assertions by
uncommenting or re-adding Assertions.assertNotNull(testBean) and
Assertions.assertTrue(testBean.execute()), ensure the testBean field is properly
initialized (e.g., @Autowired or set up in a @BeforeEach/@Before method) and
available to the test, and add any missing imports
(org.junit.jupiter.api.Assertions) or adjust the test signature as needed so the
assertions run.
- Around line 29-36: The test class still imports JUnit 4 symbols and uses a
JUnit 4 runner; replace org.junit.AfterClass and org.junit.BeforeClass imports
with org.junit.jupiter.api.AfterAll and org.junit.jupiter.api.BeforeAll, remove
org.junit.experimental.runners.Enclosed and org.junit.runner.RunWith imports,
replace any @BeforeClass/@AfterClass annotations on methods with
@BeforeAll/@AfterAll, and remove the @RunWith(Enclosed.class) annotation (nested
tests use @Nested in JUnit 5 natively); also delete any now-unused JUnit 4
imports so only JUnit 5 imports (e.g., @Test, @Nested, @BeforeAll, @AfterAll,
Assertions) remain.
- Around line 341-346: Remove the misplaced commented and duplicate @ExtendWith
annotations in the TestWithBootstrapDisabledAndDefaultNamespacesAndConditionalOn
class: delete the commented-out "// @ExtendWith(SpringExtension.class)" and
ensure only a single @ExtendWith(SpringExtension.class) annotation is present
immediately above the class declaration alongside @Nested,
@SpringBootTest(classes = ConfigurationWithConditionalOnProperty.class) and
@DirtiesContext so the class has one valid @ExtendWith attachment instead of
duplicates or commented variants.
- Around line 375-380: The @ExtendWith(SpringExtension.class) annotation is
incorrectly commented out and duplicated/misplaced around the nested test class
TestWithBootstrapDisabledAndDefaultNamespacesAndConditionalOff; uncomment the
commented @ExtendWith(SpringExtension.class) and ensure the annotations are
ordered directly above the class declaration (e.g., @Nested,
@ExtendWith(SpringExtension.class), @SpringBootTest(...), @DirtiesContext) so
there are no commented or duplicate annotations and the class-level annotations
apply correctly to
TestWithBootstrapDisabledAndDefaultNamespacesAndConditionalOff.
- Around line 409-414: Uncomment and relocate the duplicate
@ExtendWith(SpringExtension.class) and ensure the annotations are directly above
the class declaration for TestWithBootstrapEnabledAndEagerLoadEnabled: remove
the commented-out line, and place @ExtendWith(SpringExtension.class) together
with @Nested, @SpringBootTest(classes =
{ConfigurationWithoutConditionalOnProperty.class, TestBean.class}) and
@DirtiesContext immediately above the class declaration for
TestWithBootstrapEnabledAndEagerLoadEnabled so all four annotations apply to
that class.
- Around line 269-274: Remove the commented-out duplicate annotation and ensure
@ExtendWith(SpringExtension.class) appears exactly once immediately before the
class declaration for
TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOff; move the active
@ExtendWith so it directly precedes the class declaration line (not after
@DirtiesContext) and delete the commented-out @ExtendWith line to avoid
duplication and misplaced annotations.
- Around line 305-310: Remove the stray commented-out @ExtendWith and
consolidate the annotations so they sit immediately above the nested test class
declaration: ensure @Nested, @ExtendWith(SpringExtension.class),
@SpringBootTest(classes = ConfigurationWithoutConditionalOnProperty.class) and
@DirtiesContext are all active and placed directly above the class
TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOffWithYamlFile
(remove the commented line and any duplicated @ExtendWith).
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/ResetDeferredLoggerExtension.java:
- Around line 28-41: Replace the reflection-based reset in
ResetDeferredLoggerExtension by adding a package-private/test-visible reset API
on DeferredLogger (e.g., void resetForTests() or reset()) that clears
DeferredLogCache and resets the internal AtomicInteger state to -1 without
replacing the AtomicInteger object; update
ResetDeferredLoggerExtension.beforeEach to call DeferredLogger.reset() (and keep
DeferredLogCache.clear() if still desired) so there is no use of
DeferredLogger.state via reflection and no brittle field access.
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/TestListAppender.java:
- Around line 42-44: getEvents() exposes the internal mutable list 'events' from
TestListAppender, allowing callers to modify captured events; change getEvents()
to return an unmodifiable view (e.g., Collections.unmodifiableList(events) or
List.copyOf(events) for Java 10+) so callers cannot alter the internal list, and
add the required import if missing.
In
@apollo-core/src/test/java/com/ctrip/framework/foundation/FoundationTest.java:
- Around line 45-46: The assertNotNull calls use JUnit5 ordering incorrectly:
pass the actual value first and the message second; update the two calls to
Assertions.assertNotNull(hostAddress, "No host address detected.") and
Assertions.assertNotNull(hostName, "No host name resolved.") so that hostAddress
and hostName are actually checked (refer to Assertions.assertNotNull and the
hostAddress/hostName locals in FoundationTest).
In
@apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/ClusterOpenApiServiceTest.java:
- Around line 71-78: The assertThrows lambda in testGetClusterWithError is
poorly formatted; reformat the call to assertThrows(RuntimeException.class, ()
-> clusterOpenApiService.getCluster(someAppId, someEnv, someCluster)); so the
lambda is on a single line with proper spacing and indentation inside the
testGetClusterWithError method (keep the
when(statusLine.getStatusCode()).thenReturn(404) line above unchanged).
🟡 Minor comments (8)
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/AppOpenApiServiceTest.java-72-77 (1)
72-77: Improve assertThrows formatting for better readability.The migration from
@Test(expected = ...)toassertThrowsis correct, but the formatting is inconsistent. The lambda expression and method call are split across lines without proper formatting.✨ Suggested formatting improvement
Format the
assertThrowscalls consistently. For example, fortestGetEnvClusterInfoWithError:- assertThrows(RuntimeException.class,()-> - appOpenApiService.getEnvClusterInfo(someAppId)); + assertThrows(RuntimeException.class, () -> + appOpenApiService.getEnvClusterInfo(someAppId));Apply similar formatting to the other test methods:
testCreateAppNullApp,testCreateAppEmptyAppId,testCreateAppEmptyAppName, andtestCreateAppFail.Also applies to: 79-84, 86-92, 94-103, 105-118
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/AppOpenApiServiceTest.java-146-146 (1)
146-146: Consider using full URI validation for better test precision.The
endsWith("apps")check is less precise than a full URI validation and could match unintended endpoints. For consistency withtestGetEnvClusterInfo(lines 68-69), which validates the complete URI, consider checking the full path here as well.🎯 Suggested improvement
- return request.getURI().toString().endsWith("apps"); + return request.getURI().toString().equals(String.format("%s/apps", someBaseUrl));Committable suggestion skipped: line range outside the PR's diff.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java-80-81 (1)
80-81: Remove commented-out duplicate annotation.Line 80 contains a commented-out duplicate of the active
@ExtendWithannotation on Line 81. Remove the commented line to keep the code clean.🧹 Proposed fix
-//@ExtendWith(MockitoExtension.class) @ExtendWith(MockitoExtension.class) public class RemoteConfigRepositoryTest {apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.java-41-43 (1)
41-43: Use English for comments in an international open-source project.The Chinese comment reduces accessibility for international contributors.
📝 Proposed fix
- /** - * ⚠️ 在 Spring Context 初始化前执行 - */ + /** + * ⚠️ Executed before Spring Context initialization + */apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/ResetDeferredLoggerExtension.java-31-31 (1)
31-31: Replace Chinese comments with English.The comments at lines 31, 34, 37, and 39 are in Chinese. For an international open-source project, all code comments should be in English to ensure accessibility for all contributors.
📝 Proposed translation
- // 清理 DeferredLogCache + // Clear DeferredLogCache DeferredLogCache.clear(); - // 通过反射重置 DeferredLogger.state + // Reset DeferredLogger.state via reflection Field stateField = DeferredLogger.class.getDeclaredField("state"); stateField.setAccessible(true); - // 获取 AtomicInteger 对象 + // Get the AtomicInteger object AtomicInteger state = (AtomicInteger) stateField.get(null); - // 只修改它的值，不替换对象 + // Only modify its value, do not replace the object state.set(-1);Also applies to: 34-34, 37-37, 39-39
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/DeferredLoggerTest.java-47-47 (1)
47-47: Replace Chinese comments with English.Comments at lines 47, 50, 54, 57, 106, 112, and 115 are in Chinese. For consistency and international collaboration, use English for all code comments.
📝 Proposed translations
- // 获取 Log4j2 核心 Logger + // Get Log4j2 core Logger logger = (Logger) LogManager.getLogger("DeferredLoggerTest"); - // 创建 ListAppender 用于捕获日志 + // Create ListAppender to capture logs listAppender = new TestListAppender("TestAppender"); listAppender.start(); - // 将 ListAppender 添加到 logger + // Attach ListAppender to logger logger.addAppender(listAppender); - // 清空 DeferredLogger 状态 + // Clear DeferredLogger state DeferredLogger.disable(); DeferredLogCache.clear(); }And in testDeferredLog:
- // 不再断言 false，因为 MemoryAppender 会立即捕获日志 + // No longer asserting false because the appender captures logs immediately // assertFalse(containsMessage("errorLogger_testDeferredLog")); - // 回放缓存 + // Replay cache DeferredLogCache.replayTo(); - // 断言日志已被输出（MemoryAppender 捕获） + // Assert logs have been output (captured by appender) assertTrue(containsMessage("errorLogger_testDeferredLog"));Also applies to: 50-50, 54-54, 57-57, 106-106, 112-112, 115-115
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/DeferredLoggerTest.java-97-120 (1)
97-120: Add assertions to directly verify deferred behavior through cache state.The test correctly enables deferred logging and verifies logs appear after replay. However, it lacks verification that logs were actually cached and deferred before replay. The commented-out
assertFalsechecks were valid and should be replaced with explicit cache size assertions usingDeferredLogCache.logSize():Suggested assertion additions
DeferredLogger.enable(); slf4jLogger.error("errorLogger_testDeferredLog"); slf4jLogger.info("inFoLogger_testDeferredLog"); slf4jLogger.warn("warnLogger_testDeferredLog"); slf4jLogger.debug("debugLogger_testDeferredLog"); // Verify logs are cached and NOT yet in appender assertEquals(4, DeferredLogCache.logSize(), "Logs should be cached before replay"); assertFalse(containsMessage("errorLogger_testDeferredLog"), "Logs should not reach appender during deferred mode"); // Replay and verify cache is flushed DeferredLogCache.replayTo(); assertEquals(0, DeferredLogCache.logSize(), "Cache should be empty after replay"); assertTrue(containsMessage("errorLogger_testDeferredLog")); // ... other assertionsThis directly tests the core deferred logging contract: logs are held in cache during deferred mode and flushed to the appender on replay.
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/DeferredLoggerTest.java-106-110 (1)
106-110: Remove commented assertions or restore test coverage for deferred behavior.Lines 106-110 contain commented assertions that verify logs do NOT appear in the appender before replay. The inline comment explaining this removal is misleading: TestListAppender is a custom appender, not a "MemoryAppender," and based on the DeferredLogger implementation, when deferred logging is enabled, log calls are stored in DeferredLogCache (lines 156-160 of DeferredLogger.java) without triggering the appender immediately.
The test currently validates that logs appear after replay (lines 116-119), but by removing the assertions that logs do NOT appear before replay, it no longer fully validates that deferred logging actually defers log output. This is a critical behavior to test.
Either:
- Remove the commented code entirely with a clear explanation of why pre-replay verification is no longer needed
- Restore the assertions—if they fail, it indicates a regression in deferred logging behavior
- Replace them with direct cache verification: check
DeferredLogCache.logSize()before/after replay to confirm deferral is working
🧹 Nitpick comments (29)
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMessageProducerCompositeTest.java (1)
43-48: Consider using MockitoExtension for JUnit 5.While
MockitoAnnotations.initMocks(this)works, the idiomatic JUnit 5 approach is to use the@ExtendWith(MockitoExtension.class)annotation at the class level, which automatically initializes mocks.♻️ Suggested refactor
Add the extension at the class level:
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class ApolloClientMessageProducerCompositeTest {Then remove the manual initialization:
@BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); List<MessageProducer> producers = Arrays.asList(producer1, producer2); composite = new ApolloClientMessageProducerComposite(producers); }apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java (1)
40-55: Consider using MockitoExtension for JUnit 5.Since this test is now using JUnit 5, the idiomatic approach is to use
@ExtendWith(MockitoExtension.class)at the class level instead of manually initializing mocks. This eliminates the need for thesetUp()method and aligns better with JUnit 5 conventions.♻️ Refactor to use MockitoExtension
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class DefaultApolloClientNamespaceApiTest { @Mock private ConfigManager configManager; @Mock private Config config; @InjectMocks private DefaultApolloClientNamespaceApi namespaceApi; @BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); when(configManager.getConfig(anyString())).thenReturn(config); }apollo-client/src/test/java/com/ctrip/framework/apollo/spring/boot/ApolloApplicationContextInitializerTest.java (1)
139-139: Consider using assertNotInstanceOf for consistency.Line 139 uses the older
assertFalse(...instanceof...)pattern, while Line 151 uses the newer JUnit 5assertInstanceOf. For consistency and better error messages, consider updating this line.♻️ Proposed refactor for consistency
- assertFalse(propertySources.iterator().next() instanceof CachedCompositePropertySource); + assertNotInstanceOf(CachedCompositePropertySource.class, propertySources.iterator().next());Don't forget to add the static import:
+import static org.junit.jupiter.api.Assertions.assertNotInstanceOf;apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XMLConfigAnnotationTest.java (1)
67-75: Improve formatting ofassertThrowscalls.The
assertThrowscalls have non-standard formatting where the lambda arrow is split across lines with inconsistent indentation. While functionally correct, this reduces readability.♻️ Suggested formatting improvements
For testApolloConfigWithWrongFieldType (lines 73-74):
- assertThrows(BeanCreationException.class,()-> - getBean("spring/XmlConfigAnnotationTest2.xml", TestApolloConfigBean2.class)); + assertThrows(BeanCreationException.class, () -> + getBean("spring/XmlConfigAnnotationTest2.xml", TestApolloConfigBean2.class));For testApolloConfigChangeListenerWithWrongParamType (lines 139-140):
- assertThrows(BeanCreationException.class,()-> - getBean("spring/XmlConfigAnnotationTest4.xml", TestApolloConfigChangeListenerBean2.class)); + assertThrows(BeanCreationException.class, () -> + getBean("spring/XmlConfigAnnotationTest4.xml", TestApolloConfigChangeListenerBean2.class));For testApolloConfigChangeListenerWithWrongParamCount (lines 148-149):
- assertThrows(BeanCreationException.class,()-> - getBean("spring/XmlConfigAnnotationTest5.xml", TestApolloConfigChangeListenerBean3.class)); + assertThrows(BeanCreationException.class, () -> + getBean("spring/XmlConfigAnnotationTest5.xml", TestApolloConfigChangeListenerBean3.class));Also applies to: 133-141, 143-150
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java (1)
86-90: Improve assertThrows formatting for readability.The
assertThrowscalls have inconsistent formatting: missing space after the comma and awkward line breaks that split the lambda from its body. This pattern appears in six test methods.♻️ Suggested formatting improvement
Apply consistent formatting with proper spacing and lambda body placement. Example for
testGetNamespaceWithError:- @Test public void testGetNamespaceWithError() throws Exception { when(statusLine.getStatusCode()).thenReturn(404); - assertThrows(RuntimeException.class,()-> - namespaceOpenApiService.getNamespace(someAppId, someEnv, someCluster, someNamespace, true)); + assertThrows(RuntimeException.class, () -> + namespaceOpenApiService.getNamespace(someAppId, someEnv, someCluster, someNamespace, true)); }Apply the same pattern to the other five test methods:
testGetNamespaceWithErrorAndFillItemDetailFalse(lines 93-97)testGetNamespacesWithError(lines 130-134)testGetNamespacesWithErrorAndFillItemDetailFalse(lines 137-141)testCreateAppNamespaceWithError(lines 165-177)testGetNamespaceLockWithError(lines 195-199)Also applies to: 93-97, 130-134, 137-141, 165-177, 195-199
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/url/OpenApiPathBuilderTest.java (2)
244-248: Improve lambda formatting for readability.The
assertThrowsmigration is correct, but the lambda formatting could be improved. Consider adding proper spacing and line breaks.♻️ Proposed formatting improvement
@Test public void testAddParamKeyEmpty() { - assertThrows(IllegalArgumentException.class,()-> - OpenApiPathBuilder.newBuilder().addParam("", "")); + assertThrows(IllegalArgumentException.class, () -> { + OpenApiPathBuilder.newBuilder().addParam("", ""); + }); }
250-254: Improve lambda formatting for readability.Similar to the previous test method, the lambda formatting could be improved with proper spacing and line breaks.
♻️ Proposed formatting improvement
@Test public void testBuildPathURLEmpty() { - assertThrows(IllegalArgumentException.class,()-> - OpenApiPathBuilder.newBuilder().buildPath("")); + assertThrows(IllegalArgumentException.class, () -> { + OpenApiPathBuilder.newBuilder().buildPath(""); + }); }apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
189-192: Consider using static imports consistently.Lines 189-192 use qualified
Mockito.verifyandMockito.times, while line 88 uses the statically imported versions. For consistency with the rest of the test file, consider using the static imports.♻️ Proposed refactor
- Mockito.verify(coreV1Api, Mockito.times(1)).listNamespacedPod(namespace, null, null, + verify(coreV1Api, times(1)).listNamespacedPod(namespace, null, null,apollo-client/src/test/java/com/ctrip/framework/apollo/util/OrderedPropertiesTest.java (1)
100-104: Consider reformatting the assertThrows lambda for better readability.The lambda expression is split awkwardly across lines. The current formatting places the arrow
->at the end of line 102 and the lambda body on line 103, which is unconventional and reduces readability.♻️ Proposed formatting improvement
@Test public void testPutNull() { - assertThrows(NullPointerException.class,()-> - orderedProperties.put("key3", null)); + assertThrows(NullPointerException.class, () -> { + orderedProperties.put("key3", null); + }); }Or as a one-liner:
@Test public void testPutNull() { - assertThrows(NullPointerException.class,()-> - orderedProperties.put("key3", null)); + assertThrows(NullPointerException.class, () -> orderedProperties.put("key3", null)); }apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1)
377-384: Consider renaming the test method for clarity.The method name
test()is not descriptive. Consider renaming it to something liketestPropertyNamesCacheEnabled()ortestPropertyNamesCacheEnabledWithSystemProperty()to match the naming convention used by other test methods in this file.apollo-core/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java (1)
77-96: Previous issues resolved - implementation now correct.The critical issues from the prior review have been properly addressed:
- ✅ No longer casts to
HttpServletResponse- uses native Jetty 12 core Response API- ✅ Parameter renamed to
responseStrto avoid shadowing, and correctly writes this value to the response body- ✅ Uses
response.getHeaders().put()for Content-Type instead of the non-existentsetContentType()- ✅ Uses
response.write(true, content, callback)pattern correctly for Jetty 12Minor: Consider translating the Chinese comments to English for codebase consistency:
- // 设置响应头 + // Set response headers response.setStatus(statusCode); response.getHeaders().put("Content-Type", "text/plain;charset=UTF-8"); - // 写入响应体 + // Write response body ByteBuffer content = ByteBuffer.wrap(responseStr.getBytes(StandardCharsets.UTF_8)); response.write(true, content, callback); - return true; // 表示请求已处理 + return true; // Indicates request was handledapollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java (1)
127-135: Consider improving assertThrows formatting for consistency.The
assertThrowscalls are functionally correct but have inconsistent formatting: missing space after the comma and unconventional line breaks. For better readability, consider:- assertThrows(BeanCreationException.class,()-> - getBean(TestApolloConfigBean2.class, AppConfig2.class)); + assertThrows(BeanCreationException.class, () -> + getBean(TestApolloConfigBean2.class, AppConfig2.class));This pattern applies to all similar assertThrows calls in this file.
Also applies to: 192-204, 261-278, 581-587, 630-635
apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/ItemOpenApiServiceTest.java (1)
140-154: Consider improving assertThrows formatting for consistency.The exception assertion logic is correct, but the formatting could be more readable and consistent with Java conventions.
♻️ Suggested formatting improvements
Current pattern:
assertThrows(RuntimeException.class,()-> itemOpenApiService.createItem(someAppId, someEnv, someCluster, someNamespace, itemDTO));More idiomatic formatting:
assertThrows(RuntimeException.class, () -> itemOpenApiService.createItem(someAppId, someEnv, someCluster, someNamespace, itemDTO) );Key improvements:
- Add space after comma and before arrow in lambda:
, () ->- Place opening of lambda body on same line as arrow
- Indent method call for clarity
- Place closing parenthesis on separate line
Also applies to: 207-221, 247-261, 302-310, 328-336, 338-344, 346-352, 354-360
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/PropertiesCompatibleFileConfigRepositoryTest.java (2)
50-56: LGTM! Setup method correctly migrated with appropriate lenient stubbing.The changes are correct:
@BeforeEachis the proper JUnit 5 replacement for@Beforelenient()stubbing is appropriate here since not all tests use all three stubs (e.g.,testGetSourceTypedoesn't need them, and some tests overrideasProperties())- This pattern is consistent with other migrated test files in the codebase
♻️ Optional: Fix indentation inconsistency
Lines 55-56 have extra leading spaces compared to line 54:
lenient().when(configFile.getNamespace()).thenReturn(someNamespaceName); - lenient().when(configFile.getAppId()).thenReturn(someAppId); - lenient().when(configFile.asProperties()).thenReturn(someProperties); + lenient().when(configFile.getAppId()).thenReturn(someAppId); + lenient().when(configFile.asProperties()).thenReturn(someProperties);
96-96: LGTM! Exception testing correctly migrated to JUnit 5.The migration from
@Test(expected = IllegalStateException.class)toassertThrows(IllegalStateException.class, () -> ...)is the correct JUnit 5 approach and matches the pattern used in other migrated tests likeRemoteConfigRepositoryTest.♻️ Optional: Improve lambda formatting for readability
The lambda could be formatted more consistently:
- assertThrows(IllegalStateException.class,()-> - configFileRepository.getConfig()); + assertThrows(IllegalStateException.class, () -> + configFileRepository.getConfig());Also applies to: 103-104
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/XmlConfigFileTest.java (1)
158-158: Good refactoring: More idiomatic null assertions.Replacing
assertEquals(null, ...)withassertNull(...)improves readability and aligns with JUnit 5 best practices for null checks.Also applies to: 179-179, 213-213, 216-216
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/SimpleConfigTest.java (1)
162-162: Good refactoring: Idiomatic null assertions.Using
assertNull()instead ofassertEquals(null, ...)is clearer and follows JUnit 5 conventions.Also applies to: 166-166
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java (1)
66-67: Remove the commented-out duplicate annotation.Line 66 contains a commented-out duplicate of the active
@ExtendWithannotation on line 67. This dead code should be removed.🧹 Proposed cleanup
-//@ExtendWith(MockitoExtension.class) @ExtendWith(MockitoExtension.class)apollo-mockserver/src/test/java/com/ctrip/framework/apollo/mockserver/ExtensionLifecycleTest.java (1)
19-20: Minor style inconsistency: Mixed assertion import styles.The file imports both static assertion methods (lines 19-20) and the
Assertionsclass (line 28), then uses them inconsistently—static imports intestParameterInjection(lines 51-56) butAssertions.assertTrueinafterand parameter checks (lines 39, 44).Consider using only static imports for consistency:
♻️ Suggested cleanup
import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import com.ctrip.framework.apollo.Config; import com.ctrip.framework.apollo.ConfigService; import com.ctrip.framework.apollo.core.ConfigConsts; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith;Then update usages:
@AfterEach public void after() { - Assertions.assertTrue(target.isClosed()); + assertTrue(target.isClosed()); } @Test public void testParameterInjection(ApolloTestingServer server) throws Exception { - Assertions.assertTrue(server.isStarted()); + assertTrue(server.isStarted());Also applies to: 27-29
apollo-mockserver/src/test/java/com/ctrip/framework/apollo/mockserver/ApolloMockServerSpringIntegrationTest.java (2)
44-44: Minor formatting: Missing space after comma in@ExtendWith.♻️ Suggested fix
-@ExtendWith(value = {SpringExtension.class,EmbeddedApollo.class}) +@ExtendWith(value = {SpringExtension.class, EmbeddedApollo.class})
94-100: Inconsistent indentation inassertThrowslambda.The lambda body has inconsistent indentation. Consider reformatting for readability:
♻️ Suggested fix
@Test @DirtiesContext public void shouldNotNotifyOnUninterestedPatterns(EmbeddedApollo embeddedApollo) throws Exception { embeddedApollo.addOrModifyProperty(otherNamespace, "spring.application.name", "apollo"); - assertThrows(TimeoutException.class,()-> - testInterestedKeyPrefixesBean.futureData.get(5000, TimeUnit.MILLISECONDS)); + assertThrows(TimeoutException.class, () -> + testInterestedKeyPrefixesBean.futureData.get(5000, TimeUnit.MILLISECONDS)); }apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/jmx/ApolloClientJmxMBeanRegisterTest.java (1)
35-40: Remove deprecatedinitMocks()call—it serves no purpose and is redundant.Line 37 uses
MockitoAnnotations.initMocks(this), which has been deprecated since Mockito 3.x. Since there are no@Mock-annotated fields in the test class, this call does nothing. ThemockMBeanServeris created manually on line 38 viamock(MBeanServer.class), making theinitMocks()invocation unnecessary.Option 1 (simplest): Remove line 37 entirely.
Option 2 (idiomatic): Adopt JUnit 5 + Mockito best practices with
@ExtendWith(MockitoExtension.class)and@Mockannotation.♻️ Option 1: Remove unnecessary initMocks call
@BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); mockMBeanServer = mock(MBeanServer.class); ApolloClientJmxMBeanRegister.setMBeanServer(mockMBeanServer); }♻️ Option 2: Use MockitoExtension (JUnit 5 + Mockito pattern)
At class level, add:
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class ApolloClientJmxMBeanRegisterTest { + @Mock private MBeanServer mockMBeanServer;Then simplify setUp:
@BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); - mockMBeanServer = mock(MBeanServer.class); ApolloClientJmxMBeanRegister.setMBeanServer(mockMBeanServer); }apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (1)
437-448: Rename misleading loop variable.The factory loading test has been correctly updated to work with
SpringFactoriesLoader.loadFactories()returning instances rather than class names. However, the loop variable is namedname(line 441) but contains anEnvironmentPostProcessorinstance, which is confusing.♻️ Proposed refactor for clarity
@Test public void test() { List<EnvironmentPostProcessor> names = SpringFactoriesLoader.loadFactories(EnvironmentPostProcessor.class, getClass().getClassLoader()); boolean containsApollo = false; - for (EnvironmentPostProcessor name : names) { - if (name.getClass().getName().equals("com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer")) { + for (EnvironmentPostProcessor processor : names) { + if (processor.getClass().getName().equals("com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer")) { containsApollo = true; break; } } Assertions.assertTrue(containsApollo); }Also consider renaming the
namesvariable toprocessorsfor consistency:- List<EnvironmentPostProcessor> names = SpringFactoriesLoader.loadFactories(EnvironmentPostProcessor.class, getClass().getClassLoader()); + List<EnvironmentPostProcessor> processors = SpringFactoriesLoader.loadFactories(EnvironmentPostProcessor.class, getClass().getClassLoader()); boolean containsApollo = false; - for (EnvironmentPostProcessor processor : names) { + for (EnvironmentPostProcessor processor : processors) {apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java (1)
60-92: Fix indentation on line 81.The test logic is correct and the JUnit 5 migration is properly implemented. However, line 81 has inconsistent indentation.
♻️ Proposed fix
assertNotNull(result); - assertInstanceOf(MockApolloClientMetricsExporter.class, result); + assertInstanceOf(MockApolloClientMetricsExporter.class, result); }apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.java (2)
33-33: Remove the commented annotation.The
@SpringBootTestannotation already includesSpringExtensionautomatically, making this commented line redundant and confusing.🧹 Proposed cleanup
-//@ExtendWith(SpringExtension.class)
62-76: Remove commented-out code.Version control preserves the history, so there's no need to keep this commented code block. If it needs to be restored later, it can be retrieved from git history.
🧹 Proposed cleanup
@Test public void testEnvironmentVariablesCompatible() throws Exception { -// SystemLambda.withEnvironmentVariable( -// ApolloClientSystemConsts.DEPRECATED_APOLLO_CACHE_DIR_ENVIRONMENT_VARIABLES, -// "test-2/cacheDir") -// .and(ApolloClientSystemConsts.DEPRECATED_APOLLO_ACCESS_KEY_SECRET_ENVIRONMENT_VARIABLES, -// "test-2-secret") -// .and(ApolloClientSystemConsts.DEPRECATED_APOLLO_CONFIG_SERVICE_ENVIRONMENT_VARIABLES, -// "https://test-2-config-service") -// .execute(() -> { -// assertEquals("test-2/cacheDir", -// this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR)); -// assertEquals("test-2-secret", -// this.environment.getProperty(ApolloClientSystemConsts.APOLLO_ACCESS_KEY_SECRET)); -// assertEquals("https://test-2-config-service", -// this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CONFIG_SERVICE)); -// }); Assertions.assertEquals(apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/TestListAppender.java (1)
31-31: Consider thread-safety if appender is used concurrently.The
eventslist usesArrayList, which is not thread-safe. If this appender might be accessed by multiple threads during testing, consider using a thread-safe collection likeCopyOnWriteArrayListor synchronizing access.♻️ Optional thread-safe alternative
-import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; public class TestListAppender extends AbstractAppender { - private final List<LogEvent> events = new ArrayList<>(); + private final List<LogEvent> events = new CopyOnWriteArrayList<>();Also applies to: 38-40
apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/DeferredLoggerTest.java (2)
32-35: Remove commented-out code.Lines 32-35 contain commented-out JUnit 5 annotations. If these are no longer needed, remove them. If they might be needed in the future, document why they're commented rather than deleted.
🧹 Proposed cleanup
-//@ExtendWith(AloneExtension.class) -//@TestInstance(TestInstance.Lifecycle.PER_CLASS) -//@Execution(ExecutionMode.SAME_THREAD) -//@ResourceLock(Resources.SYSTEM_OUT) @ExtendWith(ResetDeferredLoggerExtension.class) public class DeferredLoggerTest {
39-55: Consider alternative approaches to verify log output without Log4j2 core APIs.The test uses Log4j2 core Logger and a custom TestListAppender to capture log events. While this pattern is unique to this test and appears intentional, it creates tight coupling to Log4j2 implementation details.
Before refactoring:
- DeferredLogCacheTest demonstrates that deferred logging behavior can be tested without Log4j2 internals (cache size limits, disable state), but that test doesn't verify actual log output.
- If output verification is essential, consider whether a mock or spy of the underlying SLF4J logger could replace the custom appender approach, or document why Log4j2 core API access is necessary here.
- Verify this approach remains compatible with the Spring Boot 4.0.1 environment and Log4j2 configuration.
| @Test | ||
| public void testEnvironmentVariablesCompatible() throws Exception { | ||
| SystemLambda.withEnvironmentVariable( | ||
| ApolloClientSystemConsts.DEPRECATED_APOLLO_CACHE_DIR_ENVIRONMENT_VARIABLES, | ||
| "test-2/cacheDir") | ||
| .and(ApolloClientSystemConsts.DEPRECATED_APOLLO_ACCESS_KEY_SECRET_ENVIRONMENT_VARIABLES, | ||
| "test-2-secret") | ||
| .and(ApolloClientSystemConsts.DEPRECATED_APOLLO_CONFIG_SERVICE_ENVIRONMENT_VARIABLES, | ||
| "https://test-2-config-service") | ||
| .execute(() -> { | ||
| Assert.assertEquals("test-2/cacheDir", | ||
| this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR)); | ||
| Assert.assertEquals("test-2-secret", | ||
| this.environment.getProperty(ApolloClientSystemConsts.APOLLO_ACCESS_KEY_SECRET)); | ||
| Assert.assertEquals("https://test-2-config-service", | ||
| this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CONFIG_SERVICE)); | ||
| }); | ||
| // SystemLambda.withEnvironmentVariable( | ||
| // ApolloClientSystemConsts.DEPRECATED_APOLLO_CACHE_DIR_ENVIRONMENT_VARIABLES, | ||
| // "test-2/cacheDir") | ||
| // .and(ApolloClientSystemConsts.DEPRECATED_APOLLO_ACCESS_KEY_SECRET_ENVIRONMENT_VARIABLES, | ||
| // "test-2-secret") | ||
| // .and(ApolloClientSystemConsts.DEPRECATED_APOLLO_CONFIG_SERVICE_ENVIRONMENT_VARIABLES, | ||
| // "https://test-2-config-service") | ||
| // .execute(() -> { | ||
| // assertEquals("test-2/cacheDir", | ||
| // this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR)); | ||
| // assertEquals("test-2-secret", | ||
| // this.environment.getProperty(ApolloClientSystemConsts.APOLLO_ACCESS_KEY_SECRET)); | ||
| // assertEquals("https://test-2-config-service", | ||
| // this.environment.getProperty(ApolloClientSystemConsts.APOLLO_CONFIG_SERVICE)); | ||
| // }); | ||
| Assertions.assertEquals( | ||
| "test-2/cacheDir", | ||
| environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR) | ||
| ); | ||
| Assertions.assertEquals( | ||
| "test-2-secret", | ||
| environment.getProperty(ApolloClientSystemConsts.APOLLO_ACCESS_KEY_SECRET) | ||
| ); | ||
| Assertions.assertEquals( | ||
| "https://test-2-config-service", | ||
| environment.getProperty(ApolloClientSystemConsts.APOLLO_CONFIG_SERVICE) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the test file and read the full context
fd "ApolloClientEnvironmentVariablesCompatibleTest.java" -type fRepository: apolloconfig/apollo-java
Length of output: 238
🏁 Script executed:
# Search for DEPRECATED_APOLLO environment variable constants
rg -n "DEPRECATED_APOLLO.*ENVIRONMENT_VARIABLES" --type java -A 2 -B 2Repository: apolloconfig/apollo-java
Length of output: 7683
🏁 Script executed:
# Search for other tests that might validate the compatibility mapping
rg -n "SystemLambda|@DynamicPropertySource" --type java -lRepository: apolloconfig/apollo-java
Length of output: 319
🏁 Script executed:
# Read the full test file to understand the context
wc -l apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.javaRepository: apolloconfig/apollo-java
Length of output: 208
🏁 Script executed:
# Read the test file
cat -n apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.javaRepository: apolloconfig/apollo-java
Length of output: 4512
🏁 Script executed:
# Check if there are other tests that verify deprecated environment variable compatibility
rg -n "DEPRECATED_APOLLO" apollo-client-config-data/src/test --type java -A 3 -B 1Repository: apolloconfig/apollo-java
Length of output: 4797
🏁 Script executed:
# Look at the ApolloClientPropertyCompatibleTestConfiguration to understand the test setup
fd "ApolloClientPropertyCompatibleTestConfiguration" -type fRepository: apolloconfig/apollo-java
Length of output: 238
🏁 Script executed:
# Find the configuration file
fd "ApolloClientPropertyCompatibleTestConfiguration" -type fRepository: apolloconfig/apollo-java
Length of output: 238
🏁 Script executed:
# Read the configuration file
cat -n apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientPropertyCompatibleTestConfiguration.javaRepository: apolloconfig/apollo-java
Length of output: 1207
🏁 Script executed:
# Also check the ApolloClientSystemPropertiesCompatibleTest to see what it tests
cat -n apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientSystemPropertiesCompatibleTest.javaRepository: apolloconfig/apollo-java
Length of output: 3464
🏁 Script executed:
# Check if there are other tests in the directory that might test env variables compatibility
find apollo-client-config-data/src/test -name "*.java" | xargs grep -l "SystemLambda\|environment.*variable" | head -10Repository: apolloconfig/apollo-java
Length of output: 319
🏁 Script executed:
# Check PureApolloConfigTest for any relevant tests
cat -n apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/importer/PureApolloConfigTest.java | head -150Repository: apolloconfig/apollo-java
Length of output: 4628
🏁 Script executed:
# Search for any test method names that might indicate compatibility testing
grep -n "test.*[Cc]ompatib\|test.*[Dd]eprecated" apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data -rRepository: apolloconfig/apollo-java
Length of output: 841
🏁 Script executed:
# Let's verify that SystemLambda is still available in dependencies
find apollo-client-config-data -name "pom.xml" -o -name "build.gradle" | xargs grep -l "systemlambda\|SystemLambda" 2>/dev/null | head -5Repository: apolloconfig/apollo-java
Length of output: 50
🏁 Script executed:
# Check the pom.xml for the test module
cat apollo-client-config-data/pom.xml | grep -A 5 -B 5 "systemlambda"Repository: apolloconfig/apollo-java
Length of output: 50
🏁 Script executed:
# Check the test dependencies in the pom.xml
cat apollo-client-config-data/pom.xml | head -200Repository: apolloconfig/apollo-java
Length of output: 3697
🏁 Script executed:
# Search for systemlambda in all pom files
find . -name "pom.xml" -exec grep -l "systemlambda" {} \;Repository: apolloconfig/apollo-java
Length of output: 50
Critical: Test no longer validates environment variable backward compatibility.
The original test verified that deprecated environment variables (DEPRECATED_APOLLO_CACHE_DIR_ENVIRONMENT_VARIABLES, DEPRECATED_APOLLO_ACCESS_KEY_SECRET_ENVIRONMENT_VARIABLES, DEPRECATED_APOLLO_CONFIG_SERVICE_ENVIRONMENT_VARIABLES) were properly mapped to new property names. The new implementation uses @DynamicPropertySource to set the new property names directly and reads them back, which bypasses testing the compatibility mapping entirely.
Since deprecated environment variables are still actively supported in production code (ConfigUtil.java, ConfigServiceLocator.java, DefaultApplicationProvider.java) with deprecation warnings, this test loss could allow breaking changes to slip through. The test name testEnvironmentVariablesCompatible is now misleading as it doesn't test environment variable compatibility at all.
Restore compatibility testing by:
- Using
SystemLambda(already available in test dependencies) to set deprecated environment variables - Verifying they correctly map to the new property names
Or, if deprecated environment variables are being intentionally removed, rename the test to reflect that it now only tests direct property injection.
🤖 Prompt for AI Agents
In
@apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.java
around lines 60 - 89, The test testEnvironmentVariablesCompatible no longer
verifies deprecated environment variables mapping because it sets new properties
directly; restore the original compatibility check by using
SystemLambda.withEnvironmentVariable to set
DEPRECATED_APOLLO_CACHE_DIR_ENVIRONMENT_VARIABLES,
DEPRECATED_APOLLO_ACCESS_KEY_SECRET_ENVIRONMENT_VARIABLES, and
DEPRECATED_APOLLO_CONFIG_SERVICE_ENVIRONMENT_VARIABLES (from
ApolloClientSystemConsts) and execute assertions that
environment.getProperty(ApolloClientSystemConsts.APOLLO_CACHE_DIR),
APOLLO_ACCESS_KEY_SECRET, and APOLLO_CONFIG_SERVICE return the expected values,
or if deprecation support was intentionally removed, rename
testEnvironmentVariablesCompatible to reflect it only validates direct property
injection; update the test body accordingly and remove the current assertions
that bypass deprecated env var mapping.
| assertEquals(result, someProperties, | ||
| "LocalFileConfigRepository's properties should be the same as fallback repo's when there is no local cache"); |
There was a problem hiding this comment.
Fix incorrect assertion parameter order.
JUnit 5's assertEquals signature is assertEquals(expected, actual, message). The parameters here are reversed: result (actual) comes before someProperties (expected). While the test will still pass or fail correctly due to equality being commutative, failure messages will be confusing and say "expected <actual> but was <expected>".
🔧 Proposed fix
- assertEquals(result, someProperties,
+ assertEquals(someProperties, result,
"LocalFileConfigRepository's properties should be the same as fallback repo's when there is no local cache");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertEquals(result, someProperties, | |
| "LocalFileConfigRepository's properties should be the same as fallback repo's when there is no local cache"); | |
| assertEquals(someProperties, result, | |
| "LocalFileConfigRepository's properties should be the same as fallback repo's when there is no local cache"); |
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepositoryTest.java
around lines 153 - 154, The assertEquals call in LocalFileConfigRepositoryTest
currently uses the wrong parameter order; swap the expected and actual arguments
so the call becomes assertEquals(someProperties, result,
"LocalFileConfigRepository's properties should be the same as fallback repo's
when there is no local cache") — keep the same message and only flip the
variables `result` and `someProperties` in the assertEquals invocation to match
JUnit 5's expected, actual signature.
| private static String someSecret; | ||
|
|
||
| @Before | ||
| @BeforeEach |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if serviceDTO.getHomepageUrl() and configServiceLocator.getConfigServices()
# are actually invoked in all test methods
FILE="apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java"
# First, let's check if the file exists and get basic stats
if [ -f "$FILE" ]; then
echo "=== File found: $FILE ==="
echo "Total lines: $(wc -l < "$FILE")"
else
echo "File not found: $FILE"
exit 1
fi
echo ""
echo "=== Test methods in RemoteConfigLongPollServiceTest ==="
rg -n "@Test" "$FILE" | head -30
echo ""
echo "=== Lines with setUp/tearDown setup (around lines 83-106) ==="
sed -n '80,110p' "$FILE" | cat -n
echo ""
echo "=== All usages of getHomepageUrl ==="
rg -n "getHomepageUrl" "$FILE"
echo ""
echo "=== All usages of getConfigServices ==="
rg -n "getConfigServices" "$FILE"
echo ""
echo "=== Counting test methods ==="
rg -c "@Test" "$FILE"Repository: apolloconfig/apollo-java
Length of output: 1899
Remove unused mock stubs from setUp().
The stubs for serviceDTO.getHomepageUrl() and configServiceLocator.getConfigServices() (lines 92-93) are never invoked in any of the 8 test methods in this class. These unused stubs should be removed entirely from the setUp() method, rather than being marked with lenient(). Unused mocks indicate unnecessary test setup that adds maintenance burden without providing value.
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java
at line 83, Remove the unused lenient stubs from the test setup: in the setUp()
method delete the lenient() stubbing calls for serviceDTO.getHomepageUrl() and
configServiceLocator.getConfigServices(), since neither
serviceDTO.getHomepageUrl() nor configServiceLocator.getConfigServices() are
invoked by any tests in RemoteConfigLongPollServiceTest and the lenient()
markers are redundant; ensure no other tests rely on those mocks before
committing the removal.
| import org.junit.AfterClass; | ||
| import org.junit.Assert; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
| import org.junit.experimental.runners.Enclosed; | ||
| import org.junit.jupiter.api.Assertions; | ||
| import org.junit.jupiter.api.Nested; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.junit.runner.RunWith; |
There was a problem hiding this comment.
Complete the JUnit 4 to JUnit 5 migration.
The imports show an incomplete migration from JUnit 4 to JUnit 5. Lines 29-31 and 36 import JUnit 4 classes (AfterClass, BeforeClass, Enclosed, RunWith) while lines 32-35 import JUnit 5 equivalents. This creates confusion and prevents proper test execution under JUnit 5.
For a complete migration:
- Replace
@BeforeClasswith@BeforeAll(requiresimport org.junit.jupiter.api.BeforeAll) - Replace
@AfterClasswith@AfterAll(requiresimport org.junit.jupiter.api.AfterAll) - Remove
@RunWith(Enclosed.class)from line 53 (JUnit 5 natively supports@Nestedwithout a runner) - Remove unused JUnit 4 imports
🔧 Proposed import fixes
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
-import org.junit.experimental.runners.Enclosed;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.runner.RunWith;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import org.junit.AfterClass; | |
| import org.junit.Assert; | |
| import org.junit.BeforeClass; | |
| import org.junit.Test; | |
| import org.junit.experimental.runners.Enclosed; | |
| import org.junit.jupiter.api.Assertions; | |
| import org.junit.jupiter.api.Nested; | |
| import org.junit.jupiter.api.Test; | |
| import org.junit.jupiter.api.extension.ExtendWith; | |
| import org.junit.runner.RunWith; | |
| import org.junit.jupiter.api.AfterAll; | |
| import org.junit.jupiter.api.BeforeAll; | |
| import org.junit.jupiter.api.Assertions; | |
| import org.junit.jupiter.api.Nested; | |
| import org.junit.jupiter.api.Test; | |
| import org.junit.jupiter.api.extension.ExtendWith; |
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
around lines 29 - 36, The test class still imports JUnit 4 symbols and uses a
JUnit 4 runner; replace org.junit.AfterClass and org.junit.BeforeClass imports
with org.junit.jupiter.api.AfterAll and org.junit.jupiter.api.BeforeAll, remove
org.junit.experimental.runners.Enclosed and org.junit.runner.RunWith imports,
replace any @BeforeClass/@AfterClass annotations on methods with
@BeforeAll/@AfterAll, and remove the @RunWith(Enclosed.class) annotation (nested
tests use @Nested in JUnit 5 natively); also delete any now-unused JUnit 4
imports so only JUnit 5 imports (e.g., @Test, @Nested, @BeforeAll, @AfterAll,
Assertions) remain.
| public class ResetDeferredLoggerExtension implements BeforeEachCallback { | ||
| @Override | ||
| public void beforeEach(ExtensionContext context) throws Exception { | ||
| // 清理 DeferredLogCache | ||
| DeferredLogCache.clear(); | ||
|
|
||
| // 通过反射重置 DeferredLogger.state | ||
| Field stateField = DeferredLogger.class.getDeclaredField("state"); | ||
| stateField.setAccessible(true); | ||
| // 获取 AtomicInteger 对象 | ||
| AtomicInteger state = (AtomicInteger) stateField.get(null); | ||
| // 只修改它的值，不替换对象 | ||
| state.set(-1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider adding a proper reset API to DeferredLogger to avoid reflection.
The extension uses reflection to reset DeferredLogger.state, which is brittle and breaks encapsulation. Since DeferredLogger already exposes getState(), consider adding a package-private or test-visible reset() method to eliminate the need for reflection.
Reflection risks:
- Breaks if the field is renamed, removed, or changes type
- No compile-time safety
- Violates encapsulation principles
Would you like me to generate a proposed reset() method for DeferredLogger.java that would eliminate the need for reflection here?
🛡️ Add error handling for reflection
Until a proper reset API is added, at minimum add error handling:
@Override
public void beforeEach(ExtensionContext context) throws Exception {
// Clear DeferredLogCache
DeferredLogCache.clear();
- // Reset DeferredLogger.state via reflection
- Field stateField = DeferredLogger.class.getDeclaredField("state");
- stateField.setAccessible(true);
- // Get the AtomicInteger object
- AtomicInteger state = (AtomicInteger) stateField.get(null);
- // Only modify its value, do not replace the object
- state.set(-1);
+ try {
+ // Reset DeferredLogger.state via reflection
+ Field stateField = DeferredLogger.class.getDeclaredField("state");
+ stateField.setAccessible(true);
+ // Get the AtomicInteger object
+ AtomicInteger state = (AtomicInteger) stateField.get(null);
+ if (state == null) {
+ throw new IllegalStateException("DeferredLogger.state field is null");
+ }
+ // Only modify its value, do not replace the object
+ state.set(-1);
+ } catch (NoSuchFieldException | IllegalAccessException | ClassCastException e) {
+ throw new IllegalStateException("Failed to reset DeferredLogger.state via reflection. " +
+ "The field may have been renamed or its type changed.", e);
+ }
}🤖 Prompt for AI Agents
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/ResetDeferredLoggerExtension.java
around lines 28 - 41, Replace the reflection-based reset in
ResetDeferredLoggerExtension by adding a package-private/test-visible reset API
on DeferredLogger (e.g., void resetForTests() or reset()) that clears
DeferredLogCache and resets the internal AtomicInteger state to -1 without
replacing the AtomicInteger object; update
ResetDeferredLoggerExtension.beforeEach to call DeferredLogger.reset() (and keep
DeferredLogCache.clear() if still desired) so there is no use of
DeferredLogger.state via reflection and no brittle field access.
| public List<LogEvent> getEvents() { | ||
| return events; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Return unmodifiable view to prevent external modification.
The getEvents() method returns the internal mutable list directly, allowing callers to modify the captured events. Consider returning an unmodifiable view to preserve encapsulation.
♻️ Proposed refactor
+import java.util.Collections;
+
public class TestListAppender extends AbstractAppender {
public List<LogEvent> getEvents() {
- return events;
+ return Collections.unmodifiableList(events);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<LogEvent> getEvents() { | |
| return events; | |
| } | |
| public List<LogEvent> getEvents() { | |
| return Collections.unmodifiableList(events); | |
| } |
🤖 Prompt for AI Agents
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/TestListAppender.java
around lines 42 - 44, getEvents() exposes the internal mutable list 'events'
from TestListAppender, allowing callers to modify captured events; change
getEvents() to return an unmodifiable view (e.g.,
Collections.unmodifiableList(events) or List.copyOf(events) for Java 10+) so
callers cannot alter the internal list, and add the required import if missing.
| @Test | ||
| public void testGetClusterWithError() throws Exception { | ||
| String someCluster = "someCluster"; | ||
|
|
||
| when(statusLine.getStatusCode()).thenReturn(404); | ||
|
|
||
| clusterOpenApiService.getCluster(someAppId, someEnv, someCluster); | ||
| assertThrows(RuntimeException.class,()-> | ||
| clusterOpenApiService.getCluster(someAppId, someEnv, someCluster)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Improve formatting of assertThrows lambda expression.
The migration to assertThrows is functionally correct, but the formatting is awkward with inconsistent indentation and line breaks within the lambda expression.
♻️ Proposed fix for consistent formatting
@Test
public void testGetClusterWithError() throws Exception {
String someCluster = "someCluster";
when(statusLine.getStatusCode()).thenReturn(404);
- assertThrows(RuntimeException.class,()->
- clusterOpenApiService.getCluster(someAppId, someEnv, someCluster));
+ assertThrows(RuntimeException.class, () ->
+ clusterOpenApiService.getCluster(someAppId, someEnv, someCluster));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void testGetClusterWithError() throws Exception { | |
| String someCluster = "someCluster"; | |
| when(statusLine.getStatusCode()).thenReturn(404); | |
| clusterOpenApiService.getCluster(someAppId, someEnv, someCluster); | |
| assertThrows(RuntimeException.class,()-> | |
| clusterOpenApiService.getCluster(someAppId, someEnv, someCluster)); | |
| } | |
| @Test | |
| public void testGetClusterWithError() throws Exception { | |
| String someCluster = "someCluster"; | |
| when(statusLine.getStatusCode()).thenReturn(404); | |
| assertThrows(RuntimeException.class, () -> | |
| clusterOpenApiService.getCluster(someAppId, someEnv, someCluster)); | |
| } |
🤖 Prompt for AI Agents
In
@apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/ClusterOpenApiServiceTest.java
around lines 71 - 78, The assertThrows lambda in testGetClusterWithError is
poorly formatted; reformat the call to assertThrows(RuntimeException.class, ()
-> clusterOpenApiService.getCluster(someAppId, someEnv, someCluster)); so the
lambda is on a single line with proper spacing and indentation inside the
testGetClusterWithError method (keep the
when(statusLine.getStatusCode()).thenReturn(404) line above unchanged).
null-point-exception
force-pushed
the
upgrade-boot4
branch
from
cba94a1 to
9082384
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java (1)
40-55: Replace deprecated Mockito initialization with JUnit 5 extension.The
MockitoAnnotations.initMocks(this)method is deprecated and the class is missing the modern JUnit 5 Mockito integration pattern. Based on the PR's migration to JUnit 5, adopt@ExtendWith(MockitoExtension.class)for consistency with other migrated test files.♻️ Recommended refactor to use MockitoExtension
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class DefaultApolloClientNamespaceApiTest { @Mock private ConfigManager configManager; @Mock private Config config; @InjectMocks private DefaultApolloClientNamespaceApi namespaceApi; @BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); when(configManager.getConfig(anyString())).thenReturn(config); }This eliminates the deprecated API call and aligns with the modern JUnit 5 + Mockito pattern used elsewhere in the PR.
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (1)
81-102: Replace @BeforeClass/@afterclass with @BeforeAll/@afterall.All nested test classes still use JUnit 4 lifecycle annotations:
@BeforeClass(lines 81, 125, 168, 208, 245, 280, 316, 352, 386, 417)@AfterClass(lines 97, 143, 183, 222, 255, 291, 326, 364, 395, 429)In JUnit 5, replace with:
@BeforeAll(requiresimport org.junit.jupiter.api.BeforeAll)@AfterAll(requiresimport org.junit.jupiter.api.AfterAll)Both annotations require the methods to be
static.Also applies to: 125-148, 168-189, 208-227, 245-261, 280-296, 316-332, 352-367, 386-400, 417-435
apollo-client-config-data/pom.xml (1)
49-68: Remove the redundantspring-webfluxdependency.The
spring-webfluxdependency (lines 49-53) is already included transitively byspring-boot-starter-webflux(lines 54-58). Adding both creates unnecessary duplication and potential for version conflicts. Remove the explicitspring-webfluxdeclaration sincespring-boot-starter-webfluxincludes it along with other required supporting libraries (Reactor Netty, Jackson, etc.).The commented-out
optionalandversiontags on the new starters are standard practice when using Spring Boot starters with BOM/parent version management—they can remain as-is or be removed for clarity.
🤖 Fix all issues with AI agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java:
- Around line 92-93: The lenient stubs in setUp() for
serviceDTO.getHomepageUrl() and configServiceLocator.getConfigServices() are
unused; remove the two lines that call
lenient().when(serviceDTO.getHomepageUrl()).thenReturn(someServerUrl) and
lenient().when(configServiceLocator.getConfigServices()).thenReturn(Lists.newArrayList(serviceDTO))
from the setUp() method so the test fixture only defines stubs actually
exercised by the tests.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java:
- Line 66: The test imports the old javax.servlet API which is incompatible with
Spring Boot 4.x; update the import in RemoteConfigRepositoryTest from
javax.servlet.http.HttpServletResponse to
jakarta.servlet.http.HttpServletResponse (and adjust any references/usages in
RemoteConfigRepositoryTest if necessary) so the test compiles and runs against
the Jakarta Servlet API.
- Around line 67-81: Remove the commented-out duplicate of the JUnit 5
annotation: delete the line containing the commented
"//@ExtendWith(MockitoExtension.class)" so only the active
@ExtendWith(MockitoExtension.class) annotation remains applied to the
RemoteConfigRepositoryTest class.
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java:
- Around line 305-310: The annotations around the nested test class are
inconsistent: remove the commented-out duplicate and place the required
annotations directly above the nested class declaration; ensure @Nested,
@SpringBootTest(classes = ConfigurationWithoutConditionalOnProperty.class),
@DirtiesContext and a single @ExtendWith(SpringExtension.class) are all active
and directly precede the class named
TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOffWithYamlFile so
there are no commented or duplicated annotation lines.
- Around line 341-346: Remove the stray commented-out duplicate annotation and
ensure the class TestWithBootstrapDisabledAndDefaultNamespacesAndConditionalOn
has exactly one active @ExtendWith(SpringExtension.class) (not commented),
followed by the other annotations (@Nested, @SpringBootTest, @DirtiesContext) in
a consistent order; edit the annotation block so there are no commented or
duplicate @ExtendWith lines and only the single correct annotation remains above
the class declaration.
- Around line 151-155: The test method in BootstrapConfigTest currently has its
assertions commented out, removing validation for testBean; restore the checks
by uncommenting or re-adding assertions that verify testBean is not null and
that testBean.execute() returns true (i.e., use
Assertions.assertNotNull(testBean) and
Assertions.assertTrue(testBean.execute())) inside the test() method, and ensure
the proper JUnit Assertions import is present so the test actually validates the
bean.
- Around line 375-380: Remove the commented-out and duplicate @ExtendWith and
ensure annotations are correctly ordered on the nested test class: un-comment
and use a single @ExtendWith(SpringExtension.class) (or remove it entirely if
SpringBootTest already provides extension) and place it together with
@SpringBootTest and @DirtiesContext directly above the class declaration for
TestWithBootstrapDisabledAndDefaultNamespacesAndConditionalOff so there are no
commented or duplicate annotation lines.
- Around line 409-414: The annotations for the nested test class are misplaced
and one is commented out; move the annotations (@Nested, @SpringBootTest(classes
= {ConfigurationWithoutConditionalOnProperty.class, TestBean.class}),
@DirtiesContext, and a single @ExtendWith(SpringExtension.class)) so they appear
directly above the class declaration for
TestWithBootstrapEnabledAndEagerLoadEnabled, remove the commented duplicate
//@ExtendWith(SpringExtension.class), and ensure there are no duplicate
@ExtendWith annotations.
- Around line 269-274: Remove the commented-out
@ExtendWith(SpringExtension.class) and relocate the active
@ExtendWith(SpringExtension.class) so it appears directly above the class
declaration for TestWithBootstrapEnabledAndDefaultNamespacesAndConditionalOff
(i.e., place @ExtendWith immediately before the class line, above
@SpringBootTest/@DirtiesContext), ensuring there is only one @ExtendWith
annotation and annotations are in logical order.
- Around line 191-195: The test currently has its assertions commented out in
the test() method, so restore meaningful checks: uncomment or re-add assertions
to verify testBean is instantiated and behaves correctly (e.g.,
assertNotNull(testBean) and assertTrue(testBean.execute())); ensure the test
imports and uses the appropriate Assertions (org.junit.jupiter.api.Assertions)
and that testBean is initialized or injected before assertions run (check setup
methods or @BeforeEach/@BeforeAll) so the assertions pass reliably.
- Around line 104-112: The test method test() in BootstrapConfigTest currently
has all assertions commented out; un-comment and restore meaningful assertions
to validate testBean, config, and someInjectedValue by asserting testBean is not
null and its execute() returns true, asserting config equals the expected
mockedConfig, and asserting someInjectedValue equals someValue (or their current
expected values); ensure any required test setup/mocks for testBean,
mockedConfig, someValue are present so the assertions can run reliably and fail
the test when expectations are not met.
In @apollo-core/pom.xml:
- Around line 73-77: The jakarta.servlet-api dependency in the POM is missing a
test scope and is being included on the compile classpath; update the
<dependency> block for artifactId "jakarta.servlet-api" / groupId
"jakarta.servlet" to include a <scope>test</scope> element so it is only on the
test classpath, matching the other test dependencies.
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/DeferredLoggerTest.java:
- Around line 47-48: Replace the Chinese inline comments in
DeferredLoggerTest.java with clear English equivalents while preserving their
original meaning; specifically update the comment above the logger assignment
for logger = (Logger) LogManager.getLogger("DeferredLoggerTest") and all other
Chinese comments related to test setup and assertions in this class so they are
written in English and describe the intent of the code (e.g., "Get core Log4j2
Logger", "Initialize deferred logger", "Assert log output contains ...", etc.).
In
@apollo-core/src/test/java/com/ctrip/framework/foundation/FoundationTest.java:
- Around line 45-46: The assertNotNull calls use the message as the first
argument (JUnit 5 expects actual then message), so swap the arguments in the two
calls that reference Assertions.assertNotNull so they pass hostAddress and
hostName as the first parameter and the corresponding messages ("No host address
detected." and "No host name resolved.") as the second; update the two
Assertions.assertNotNull usages that reference hostAddress and hostName
accordingly.
In
@apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/AbstractOpenApiServiceTest.java:
- Around line 32-34: Remove the unused JUnit4/Mockito runner imports: delete the
import lines for org.junit.runner.RunWith and
org.mockito.junit.MockitoJUnitRunner from AbstractOpenApiServiceTest.java since
the test now uses JUnit5’s @ExtendWith(MockitoExtension.class); ensure only the
necessary JUnit5/Mockito imports remain (e.g.,
org.junit.jupiter.api.extension.ExtendWith and
org.mockito.junit.jupiter.MockitoExtension) and run the tests to verify no
unused-import warnings remain.
In @pom.xml:
- Around line 173-177: The pom currently declares Apache HttpClient 4.x
(org.apache.httpcomponents:httpclient 4.5.14) which is incompatible with Spring
Boot 4 / Spring Framework 6; replace that dependency with the HttpClient 5
artifact (org.apache.hc.client5:httpclient5) at version 5.1 or newer, updating
the <dependency> entry in the pom accordingly and ensuring any code that uses
legacy org.apache.http.* classes or types (e.g., code building
HttpComponentsClientHttpRequestFactory or RestTemplate integrations) is adjusted
to use the HttpClient 5 APIs or compatible Spring adapters.
🧹 Nitpick comments (33)
apollo-mockserver/src/test/java/com/ctrip/framework/apollo/mockserver/ExtensionLifecycleTest.java (1)
39-39: Consider using static imports consistently.Lines 39 and 44 use
Assertions.assertTrue()while the rest of the test (lines 51-52, 55-56, 59-60) uses the static imported assertions. For consistency, consider using the static importedassertTruethroughout.♻️ Proposed refactor for consistency
@AfterEach public void after() { - Assertions.assertTrue(target.isClosed()); + assertTrue(target.isClosed()); } @Test public void testParameterInjection(ApolloTestingServer server) throws Exception { - Assertions.assertTrue(server.isStarted()); + assertTrue(server.isStarted()); target = server;Also applies to: 44-44
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMessageProducerCompositeTest.java (1)
44-48: Consider modernizing Mockito initialization.
MockitoAnnotations.initMocks(this)has been deprecated. Given the Spring Boot 4.0.1 upgrade context, consider using the modern JUnit 5 approach.♻️ Recommended: Use MockitoExtension (removes boilerplate)
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class ApolloClientMessageProducerCompositeTest { private ApolloClientMessageProducerComposite composite; @Mock private MessageProducer producer1; @Mock private MessageProducer producer2; @BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); List<MessageProducer> producers = Arrays.asList(producer1, producer2); composite = new ApolloClientMessageProducerComposite(producers); }apollo-client/src/test/java/com/ctrip/framework/apollo/ConfigServiceTest.java (1)
66-66: Fix indentation.Both
assertNullstatements have inconsistent indentation with extra leading whitespace compared to surrounding code.🎨 Proposed fix
- assertNull(config.getProperty("unknown", null)); + assertNull(config.getProperty("unknown", null));Also applies to: 89-89
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializerTest.java (1)
44-46: Consider adopting JUnit 5's MockitoExtension.While migrating to JUnit 5, consider also updating the Mockito initialization pattern.
MockitoAnnotations.initMocks(this)is deprecated in favor of the@ExtendWith(MockitoExtension.class)annotation on the test class, which automatically initializes mocks without requiring the explicitinitMocks()call.♻️ Modern JUnit 5 + Mockito pattern
Add the extension annotation to the test class:
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class ConfigMonitorInitializerTest {Then remove the deprecated
initMocks()call:@BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); MockInjector.setInstance(ConfigUtil.class, mockConfigUtil); MockInjector.setInstance(ApolloClientMonitorContext.class, mockMonitorContext); MockInjector.setInstance(ApolloClientMetricsExporterFactory.class, mockExporterFactory); resetConfigMonitorInitializer(); }apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/ReleaseOpenApiServiceTest.java (1)
92-93: Consider improving assertThrows formatting.The
assertThrowscalls are functionally correct but the lambda formatting could be more readable.♻️ Optional formatting improvement
For line 92-93:
- assertThrows(RuntimeException.class,()-> - releaseOpenApiService.publishNamespace(someAppId, someEnv, someCluster, someNamespace, namespaceReleaseDTO)); + assertThrows(RuntimeException.class, () -> + releaseOpenApiService.publishNamespace(someAppId, someEnv, someCluster, someNamespace, namespaceReleaseDTO));Apply similar formatting to lines 114-115 and 142-143.
Also applies to: 114-115, 142-143
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducerTest.java (1)
33-38: Remove unnecessary MockitoAnnotations call.The
@BeforeEachmigration is correct. However,MockitoAnnotations.initMocks(this)is unnecessary since this test class doesn't use any mocks (no@Mockannotations).♻️ Proposed refactor
@BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); - producer = new ApolloClientMonitorMessageProducer(); }apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1)
377-384: Consider renaming the test method for clarity.The test method
test()is not descriptive. Other tests in this file follow a clear naming pattern (e.g.,testCustomizeConnectTimeout,testClientMonitorEnabled). Consider renaming to something liketestPropertyNamesCacheEnabledto match the convention and improve readability.♻️ Proposed refactor
- public void test() { + public void testPropertyNamesCacheEnabled() {apollo-core/src/test/java/com/ctrip/framework/apollo/tracer/TracerTest.java (1)
158-158: LGTM with minor formatting issue.The migration to
assertInstanceOfis correct and provides better error messages than the oldassertTrue(result instanceof NullTransaction)pattern. However, the indentation appears inconsistent—this line uses 6 spaces while the rest of the method body uses 4.🎨 Fix indentation to match code style
verify(someProducer, times(1)).newTransaction(someType, someName); - assertInstanceOf(NullTransaction.class, result); + assertInstanceOf(NullTransaction.class, result);apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/jmx/ApolloClientJmxMBeanRegisterTest.java (1)
35-40: Consider modernizing Mockito setup for JUnit 5.While the current setup works,
MockitoAnnotations.initMocks(this)is deprecated and has no effect here since there are no@Mockannotated fields. Consider adopting the modern JUnit 5 + Mockito approach.♻️ Modernize with MockitoExtension
+import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) public class ApolloClientJmxMBeanRegisterTest { + @Mock private MBeanServer mockMBeanServer; @BeforeEach public void setUp() { - MockitoAnnotations.initMocks(this); - mockMBeanServer = mock(MBeanServer.class); ApolloClientJmxMBeanRegister.setMBeanServer(mockMBeanServer); }apollo-client/src/test/java/com/ctrip/framework/apollo/util/yaml/YamlParserTest.java (1)
19-22: LGTM! Clean JUnit 4 → JUnit 5 migration.The test migration is correct and comprehensive:
- Import statements properly updated to JUnit Jupiter APIs
- Lifecycle annotations correctly changed (@Before/@after → @BeforeEach/@AfterEach)
- Exception testing modernized using
assertThrowsinstead of@Test(expected=...)- Type assertions improved with
assertInstanceOfassertTrueparameter order fixed to match JUnit 5 signatureThe changes align perfectly with the PR objectives and follow standard JUnit 5 migration patterns.
Optional: Minor formatting refinement for assertThrows calls
The line breaks in the
assertThrowscalls (lines 69-73, 75-79, 81-85) are syntactically correct but unconventional. Consider:- assertThrows(DuplicateKeyException.class, () -> - testInvalid("case2.yaml")); + assertThrows(DuplicateKeyException.class, () -> testInvalid("case2.yaml"));Apply similar formatting to lines 75-79 and 81-85.
Also applies to: 34-36, 41-41, 49-52, 54-57, 69-85, 106-106, 144-144
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/JavaConfigAnnotationTest.java (1)
127-135: LGTM! Exception testing correctly migrated to JUnit 5.All exception assertions have been properly migrated from JUnit 4's
@Test(expected=...)pattern to JUnit 5'sassertThrows(). The logic is correct and will properly verify that the expected exceptions are thrown.📝 Optional: Consider reformatting lambda expressions for consistency
The lambda expressions in
assertThrowshave line breaks after the arrow operator. While syntactically correct, a more conventional formatting would improve readability:- assertThrows(BeanCreationException.class,()-> - getBean(TestApolloConfigBean2.class, AppConfig2.class)); + assertThrows(BeanCreationException.class, () -> + getBean(TestApolloConfigBean2.class, AppConfig2.class));This pattern applies to similar calls at lines 196-197, 202-203, 267-268, 276-277, 585-586, and 633-634.
Also applies to: 192-198, 200-204, 261-269, 271-278, 581-587, 630-635
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/DefaultConfigManagerTest.java (1)
60-109: Test logic correctly preserved during migration.All test methods maintain their original behavior with properly migrated assertions. The tests now use JUnit 5's
assertEqualswhile also retaining Hamcrest assertions for instance equality checks, which is perfectly valid.Optional: Consider standardizing assertion style
For consistency, you could optionally replace the Hamcrest
assertThatassertions with JUnit 5'sassertSame:- assertThat( - "Get config multiple times with the same namespace should return the same config instance", - config, equalTo(anotherConfig)); + assertSame(config, anotherConfig, + "Get config multiple times with the same namespace should return the same config instance");However, this is purely stylistic—the current mixed approach works correctly.
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1)
63-63: LGTM! Lifecycle methods correctly migrated.The test lifecycle annotations have been properly updated to JUnit 5 (
@BeforeEachand@AfterEach). The cleanup intearDown()withMockInjector.reset()ensures proper test isolation.💡 Optional: Consider Mockito extension for consistency
While the current manual mock creation works perfectly, you could optionally adopt
@ExtendWith(MockitoExtension.class)with@Mockannotations for consistency with other test files mentioned in the PR. However, given that this test usesMockInjectorfor custom dependency injection, the current approach is equally valid.Also applies to: 88-88
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java (2)
38-50: Consider adding cleanup for thread pool executors.Since you're already updating test lifecycle annotations, consider adding an
@AfterEachmethod to properly shut down the thread pool executors created insetUp(). This prevents resource leaks and improves test isolation.♻️ Suggested cleanup method
+ @AfterEach + public void tearDown() { + if (remoteConfigExecutor != null) { + remoteConfigExecutor.shutdownNow(); + } + if (abstractConfigExecutor != null) { + abstractConfigExecutor.shutdownNow(); + } + if (abstractConfigFileExecutor != null) { + abstractConfigFileExecutor.shutdownNow(); + } + if (metricsExporterExecutor != null) { + metricsExporterExecutor.shutdownNow(); + } + }
52-67: Consider replacing Thread.sleep with proper synchronization.The use of
Thread.sleep(200)on line 60 makes the test timing-dependent and potentially brittle. Consider usingCountDownLatchorawaitTermination()for more reliable synchronization.Also, the comment on line 59 is in Chinese. For international projects, English comments are generally preferred.
♻️ Example using CountDownLatch
@SneakyThrows @Test public void testExportThreadPoolMetrics() { + CountDownLatch latch = new CountDownLatch(2); - remoteConfigExecutor.execute(() -> { - }); - remoteConfigExecutor.execute(() -> { - }); - // 等待任务执行完成 - Thread.sleep(200); + remoteConfigExecutor.execute(latch::countDown); + remoteConfigExecutor.execute(latch::countDown); + // Wait for tasks to complete + latch.await(5, TimeUnit.SECONDS); threadPoolApi.export0(); ApolloThreadPoolInfo info = threadPoolApi.getRemoteConfigRepositoryThreadPoolInfo(); assertEquals(0, info.getQueueSize()); assertEquals(2, info.getCompletedTaskCount()); assertEquals(2, info.getPoolSize()); }apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (1)
439-448: Consider renaming variable for clarity.The variable
names(line 439) containsEnvironmentPostProcessorinstances, not names. Consider renaming topostProcessorsorprocessorsfor better clarity.♻️ Suggested refactor
- List<EnvironmentPostProcessor> names = SpringFactoriesLoader.loadFactories(EnvironmentPostProcessor.class, getClass().getClassLoader()); + List<EnvironmentPostProcessor> postProcessors = SpringFactoriesLoader.loadFactories(EnvironmentPostProcessor.class, getClass().getClassLoader()); boolean containsApollo = false; - for (EnvironmentPostProcessor name : names) { - if (name.getClass().getName().equals("com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer")) { + for (EnvironmentPostProcessor processor : postProcessors) { + if (processor.getClass().getName().equals("com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer")) { containsApollo = true; break; }apollo-client/src/test/java/com/ctrip/framework/apollo/kubernetes/KubernetesManagerTest.java (1)
89-89: Complete the JUnit 5 migration for consistency.These test methods still use Java's
assertkeyword, which is inconsistent with the JUnit 5 migration applied throughout the rest of this file. Java assertions are also disabled by default and provide less helpful error messages than JUnit assertions.♻️ Proposed fix to use JUnit 5 assertions
Line 89:
- assert name.equals(result); + assertEquals(name, result);Line 107:
- assert name.equals(result); + assertEquals(name, result);Also applies to: 107-107
apollo-core/src/test/java/com/ctrip/framework/apollo/core/enums/EnvUtilsTest.java (1)
41-67: Standardize assertThrows formatting for consistency.The assertThrows statements have inconsistent formatting - some are split across lines (lines 43-44, 55-56) with varying indentation, while others are on a single line (lines 61, 66). Additionally, there's missing whitespace after commas in the multi-line versions.
♻️ Proposed fix for consistent formatting
@Test public void testFromInvalidString() throws Exception { - assertThrows(IllegalArgumentException.class,()-> - Env.fromString("someInvalidEnv")); + assertThrows(IllegalArgumentException.class, () -> Env.fromString("someInvalidEnv")); } @Test public void fixTypoInProductionTest() { Env prod = Env.fromString("PROD"); assertEquals(prod, Env.PRO); } @Test public void fromBlankStringTest() { - assertThrows(IllegalArgumentException.class,()-> - Env.fromString("")); + assertThrows(IllegalArgumentException.class, () -> Env.fromString("")); }apollo-core/src/test/java/com/ctrip/framework/foundation/internals/ServiceBootstrapTest.java (7)
32-36: Correct use of assertInstanceOf, but inconsistent indentation.The migration to
assertInstanceOfis the proper JUnit 5 approach for type assertions. However, line 35 has inconsistent indentation (6 spaces instead of 4).♻️ Fix indentation consistency
@Test public void loadFirstSuccessfully() throws Exception { Interface1 service = ServiceBootstrap.loadFirst(Interface1.class); - assertInstanceOf(Interface1Impl.class, service); + assertInstanceOf(Interface1Impl.class, service); }
38-42: Correct assertThrows usage, but formatting inconsistency.The migration to
assertThrowsis correct. However, the lambda formatting is inconsistent with other test methods in this file (lines 46, 51, 56, 72 use inline format with space after->).♻️ Align formatting with other assertThrows calls
@Test public void loadFirstWithNoServiceFileDefined() throws Exception { - assertThrows(IllegalStateException.class,()-> - ServiceBootstrap.loadFirst(Interface2.class)); + assertThrows(IllegalStateException.class, () -> + ServiceBootstrap.loadFirst(Interface2.class)); }
44-47: Correct assertThrows usage, minor formatting issues.The migration to
assertThrowsis correct. Minor formatting issues: inconsistent indentation and missing space after comma.♻️ Fix formatting
@Test public void loadFirstWithServiceFileButNoServiceImpl() throws Exception { - assertThrows(IllegalStateException.class,()-> ServiceBootstrap.loadFirst(Interface3.class)); + assertThrows(IllegalStateException.class, () -> + ServiceBootstrap.loadFirst(Interface3.class)); }
49-52: Correct exception handling migration.The use of
assertThrowswithServiceConfigurationErroris appropriate. Same formatting suggestions apply as previous tests.♻️ Fix formatting
@Test public void loadFirstWithWrongServiceImpl() throws Exception { - assertThrows(ServiceConfigurationError.class,()-> ServiceBootstrap.loadFirst(Interface4.class)); + assertThrows(ServiceConfigurationError.class, () -> + ServiceBootstrap.loadFirst(Interface4.class)); }
54-57: Correct exception handling migration.The migration is correct. Same formatting suggestions apply.
♻️ Fix formatting
@Test public void loadFirstWithServiceImplNotExists() throws Exception { - assertThrows(ServiceConfigurationError.class,()-> ServiceBootstrap.loadFirst(Interface5.class)); + assertThrows(ServiceConfigurationError.class, () -> + ServiceBootstrap.loadFirst(Interface5.class)); }
64-68: Correct assertInstanceOf usage, indentation issue.The migration is correct, but line 67 has the same indentation inconsistency as line 35.
♻️ Fix indentation
@Test public void loadPrimarySuccessfully() { Interface6 service = ServiceBootstrap.loadPrimary(Interface6.class); - assertInstanceOf(Interface6Impl1.class, service); + assertInstanceOf(Interface6Impl1.class, service); }
70-73: Correct assertThrows usage, formatting issue.The migration is correct. Same formatting suggestion applies.
♻️ Fix formatting
@Test public void loadPrimaryWithServiceFileButNoServiceImpl() { - assertThrows(IllegalStateException.class,()-> ServiceBootstrap.loadPrimary(Interface7.class)); + assertThrows(IllegalStateException.class, () -> + ServiceBootstrap.loadPrimary(Interface7.class)); }apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/NamespaceOpenApiServiceTest.java (1)
86-91: Consider improving assertThrows formatting for readability.The exception testing has been correctly migrated to use
assertThrows, but the formatting could be improved for better readability. The current pattern splits the lambda across lines in an unusual way.📝 Suggested formatting improvement
For example, lines 89-90:
- assertThrows(RuntimeException.class,()-> -namespaceOpenApiService.getNamespace(someAppId, someEnv, someCluster, someNamespace, true)); + assertThrows(RuntimeException.class, () -> + namespaceOpenApiService.getNamespace(someAppId, someEnv, someCluster, someNamespace, true) + );This pattern should be applied to all assertThrows calls in this file (lines 96-97, 133-134, 140-141, 176-177, 198-199).
Also applies to: 93-98, 130-135, 137-142, 165-178, 195-200
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java (1)
66-67: Remove the commented-out duplicate annotation.Line 66 contains a commented-out
@ExtendWith(MockitoExtension.class)that duplicates Line 67. Remove the commented line to keep the code clean.🧹 Proposed cleanup
-//@ExtendWith(MockitoExtension.class) @ExtendWith(MockitoExtension.class) public class RemoteConfigLongPollServiceTest {apollo-client-config-data/src/test/java/com/ctrip/framework/apollo/config/data/system/ApolloClientEnvironmentVariablesCompatibleTest.java (1)
33-33: Remove commented-out code.The commented
@ExtendWith(SpringExtension.class)annotation is redundant since Spring Boot's@SpringBootTestautomatically registersSpringExtension. Remove the commented line for cleaner code.♻️ Proposed cleanup
-//@ExtendWith(SpringExtension.class) @SpringBootTest(classes = ApolloClientPropertyCompatibleTestConfiguration.class, webEnvironment = SpringBootTest.WebEnvironment.NONE)apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/DeferredLoggerTest.java (2)
32-36: Remove commented out code.These commented-out JUnit 5 annotations and extensions are no longer needed. Remove them to keep the codebase clean.
♻️ Proposed fix
-//@ExtendWith(AloneExtension.class) -//@TestInstance(TestInstance.Lifecycle.PER_CLASS) -//@Execution(ExecutionMode.SAME_THREAD) -//@ResourceLock(Resources.SYSTEM_OUT) @ExtendWith(ResetDeferredLoggerExtension.class) public class DeferredLoggerTest {
106-110: Remove commented out assertions.These commented-out assertions are no longer relevant with the new logging capture mechanism. Remove them to improve code clarity.
♻️ Proposed fix
slf4jLogger.debug("debugLogger_testDeferredLog"); - // 不再断言 false，因为 MemoryAppender 会立即捕获日志 -// assertFalse(containsMessage("errorLogger_testDeferredLog")); -// assertFalse(containsMessage("inFoLogger_testDeferredLog")); -// assertFalse(containsMessage("warnLogger_testDeferredLog")); -// assertFalse(containsMessage("debugLogger_testDeferredLog")); - - // 回放缓存 + // Replay cached logs DeferredLogCache.replayTo();apollo-mockserver/src/main/java/com/ctrip/framework/apollo/mockserver/EmbeddedApollo.java (1)
26-26: Remove unused import.The
ExternalResourceimport is no longer needed since the class now implements JUnit 5 extension interfaces instead of extendingExternalResource.♻️ Proposed fix
-import org.junit.rules.ExternalResource;apollo-mockserver/src/test/java/com/ctrip/framework/apollo/mockserver/ApolloMockServerSpringIntegrationTest.java (2)
44-44: Minor formatting: Add space after comma.For consistency with Java style conventions, consider adding a space after the comma in the extension array.
♻️ Proposed fix
-@ExtendWith(value = {SpringExtension.class,EmbeddedApollo.class}) +@ExtendWith(value = {SpringExtension.class, EmbeddedApollo.class})
98-99: RefactorassertThrowsfor better readability.The
assertThrowslambda expression is split awkwardly across lines. Consider reformatting for clarity.♻️ Proposed fix
- assertThrows(TimeoutException.class,()-> - testInterestedKeyPrefixesBean.futureData.get(5000, TimeUnit.MILLISECONDS)); + assertThrows(TimeoutException.class, () -> + testInterestedKeyPrefixesBean.futureData.get(5000, TimeUnit.MILLISECONDS));
| lenient().when(serviceDTO.getHomepageUrl()).thenReturn(someServerUrl); | ||
| lenient().when(configServiceLocator.getConfigServices()).thenReturn(Lists.newArrayList(serviceDTO)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused lenient stubs.
The stubs for serviceDTO.getHomepageUrl() and configServiceLocator.getConfigServices() are never invoked in any of the test methods in this class. These should be removed entirely from the setUp() method.
🧹 Proposed cleanup
someServerUrl = "http://someServer";
ServiceDTO serviceDTO = mock(ServiceDTO.class);
- lenient().when(serviceDTO.getHomepageUrl()).thenReturn(someServerUrl);
- lenient().when(configServiceLocator.getConfigServices()).thenReturn(Lists.newArrayList(serviceDTO));
MockInjector.setInstance(ConfigServiceLocator.class, configServiceLocator);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lenient().when(serviceDTO.getHomepageUrl()).thenReturn(someServerUrl); | |
| lenient().when(configServiceLocator.getConfigServices()).thenReturn(Lists.newArrayList(serviceDTO)); | |
| someServerUrl = "http://someServer"; | |
| ServiceDTO serviceDTO = mock(ServiceDTO.class); | |
| MockInjector.setInstance(ConfigServiceLocator.class, configServiceLocator); |
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollServiceTest.java
around lines 92 - 93, The lenient stubs in setUp() for
serviceDTO.getHomepageUrl() and configServiceLocator.getConfigServices() are
unused; remove the two lines that call
lenient().when(serviceDTO.getHomepageUrl()).thenReturn(someServerUrl) and
lenient().when(configServiceLocator.getConfigServices()).thenReturn(Lists.newArrayList(serviceDTO))
from the setUp() method so the test fixture only defines stubs actually
exercised by the tests.
| import java.util.Map; | ||
| import java.util.Properties; | ||
| import java.util.concurrent.TimeUnit; | ||
| import javax.servlet.http.HttpServletResponse; |
There was a problem hiding this comment.
Breaking: javax.servlet is incompatible with Spring Boot 4.x.
Spring Boot 4.x uses Jakarta EE (Jakarta Servlet API), not javax.servlet. This import will fail at runtime or compile time.
🔎 Proposed fix
-import javax.servlet.http.HttpServletResponse;
+import jakarta.servlet.http.HttpServletResponse;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import javax.servlet.http.HttpServletResponse; | |
| import jakarta.servlet.http.HttpServletResponse; |
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java
at line 66, The test imports the old javax.servlet API which is incompatible
with Spring Boot 4.x; update the import in RemoteConfigRepositoryTest from
javax.servlet.http.HttpServletResponse to
jakarta.servlet.http.HttpServletResponse (and adjust any references/usages in
RemoteConfigRepositoryTest if necessary) so the test compiles and runs against
the Jakarta Servlet API.
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.mockito.ArgumentCaptor; | ||
| import org.mockito.Mock; | ||
| import org.mockito.invocation.InvocationOnMock; | ||
| import org.mockito.junit.MockitoJUnitRunner; | ||
| import org.mockito.junit.jupiter.MockitoExtension; | ||
| import org.mockito.stubbing.Answer; | ||
|
|
||
| /** | ||
| * Created by Jason on 4/9/16. | ||
| */ | ||
| @RunWith(MockitoJUnitRunner.class) | ||
| //@ExtendWith(MockitoExtension.class) | ||
| @ExtendWith(MockitoExtension.class) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove commented-out code.
The JUnit 5 migration is correct, but Line 80 contains a commented-out duplicate of the active @ExtendWith(MockitoExtension.class) annotation on Line 81. Remove the commented line.
♻️ Proposed fix
-//@ExtendWith(MockitoExtension.class)
@ExtendWith(MockitoExtension.class)
public class RemoteConfigRepositoryTest {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import org.junit.jupiter.api.AfterEach; | |
| import org.junit.jupiter.api.BeforeEach; | |
| import org.junit.jupiter.api.Test; | |
| import org.junit.jupiter.api.extension.ExtendWith; | |
| import org.mockito.ArgumentCaptor; | |
| import org.mockito.Mock; | |
| import org.mockito.invocation.InvocationOnMock; | |
| import org.mockito.junit.MockitoJUnitRunner; | |
| import org.mockito.junit.jupiter.MockitoExtension; | |
| import org.mockito.stubbing.Answer; | |
| /** | |
| * Created by Jason on 4/9/16. | |
| */ | |
| @RunWith(MockitoJUnitRunner.class) | |
| //@ExtendWith(MockitoExtension.class) | |
| @ExtendWith(MockitoExtension.class) | |
| import org.junit.jupiter.api.AfterEach; | |
| import org.junit.jupiter.api.BeforeEach; | |
| import org.junit.jupiter.api.Test; | |
| import org.junit.jupiter.api.extension.ExtendWith; | |
| import org.mockito.ArgumentCaptor; | |
| import org.mockito.Mock; | |
| import org.mockito.invocation.InvocationOnMock; | |
| import org.mockito.junit.jupiter.MockitoExtension; | |
| import org.mockito.stubbing.Answer; | |
| /** | |
| * Created by Jason on 4/9/16. | |
| */ | |
| @ExtendWith(MockitoExtension.class) |
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/internals/RemoteConfigRepositoryTest.java
around lines 67 - 81, Remove the commented-out duplicate of the JUnit 5
annotation: delete the line containing the commented
"//@ExtendWith(MockitoExtension.class)" so only the active
@ExtendWith(MockitoExtension.class) annotation remains applied to the
RemoteConfigRepositoryTest class.
| @Test | ||
| public void test() throws Exception { | ||
| Assert.assertNotNull(testBean); | ||
| Assert.assertTrue(testBean.execute()); | ||
|
|
||
| Assert.assertEquals(mockedConfig, config); | ||
| // Assertions.assertNotNull(testBean); | ||
| // Assertions.assertTrue(testBean.execute()); | ||
|
|
||
| Assert.assertEquals(someValue, someInjectedValue); | ||
| // assertEquals(mockedConfig, config); | ||
| // | ||
| // assertEquals(someValue, someInjectedValue); | ||
| } |
There was a problem hiding this comment.
Critical: Test assertions are completely disabled.
All assertions in this test method are commented out (lines 106-111), removing validation for testBean, config, and someInjectedValue.
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
around lines 104 - 112, The test method test() in BootstrapConfigTest currently
has all assertions commented out; un-comment and restore meaningful assertions
to validate testBean, config, and someInjectedValue by asserting testBean is not
null and its execute() returns true, asserting config equals the expected
mockedConfig, and asserting someInjectedValue equals someValue (or their current
expected values); ensure any required test setup/mocks for testBean,
mockedConfig, someValue are present so the assertions can run reliably and fail
the test when expectations are not met.
| @Test | ||
| public void test() throws Exception { | ||
| Assert.assertNotNull(testBean); | ||
| Assert.assertTrue(testBean.execute()); | ||
| // Assertions.assertNotNull(testBean); | ||
| // Assertions.assertTrue(testBean.execute()); | ||
| } |
There was a problem hiding this comment.
Critical: Test assertions are completely disabled.
All assertions are commented out (lines 153-154), removing validation for testBean.
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
around lines 151 - 155, The test method in BootstrapConfigTest currently has its
assertions commented out, removing validation for testBean; restore the checks
by uncommenting or re-adding assertions that verify testBean is not null and
that testBean.execute() returns true (i.e., use
Assertions.assertNotNull(testBean) and
Assertions.assertTrue(testBean.execute())) inside the test() method, and ensure
the proper JUnit Assertions import is present so the test actually validates the
bean.
| <dependency> | ||
| <groupId>jakarta.servlet</groupId> | ||
| <artifactId>jakarta.servlet-api</artifactId> | ||
| <version>6.1.0</version> | ||
| </dependency> |
There was a problem hiding this comment.
Add test scope to jakarta.servlet-api dependency.
The jakarta.servlet-api dependency is placed in the test dependencies section but lacks the <scope>test</scope> tag, unlike other test dependencies. This will add it to the compile classpath unnecessarily.
🔧 Proposed fix
<dependency>
<groupId>jakarta.servlet</groupId>
<artifactId>jakarta.servlet-api</artifactId>
<version>6.1.0</version>
+ <scope>test</scope>
</dependency>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <dependency> | |
| <groupId>jakarta.servlet</groupId> | |
| <artifactId>jakarta.servlet-api</artifactId> | |
| <version>6.1.0</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>jakarta.servlet</groupId> | |
| <artifactId>jakarta.servlet-api</artifactId> | |
| <version>6.1.0</version> | |
| <scope>test</scope> | |
| </dependency> |
🤖 Prompt for AI Agents
In @apollo-core/pom.xml around lines 73 - 77, The jakarta.servlet-api dependency
in the POM is missing a test scope and is being included on the compile
classpath; update the <dependency> block for artifactId "jakarta.servlet-api" /
groupId "jakarta.servlet" to include a <scope>test</scope> element so it is only
on the test classpath, matching the other test dependencies.
| // 获取 Log4j2 核心 Logger | ||
| logger = (Logger) LogManager.getLogger("DeferredLoggerTest"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace Chinese comments with English.
Code comments should be in English for international collaboration and maintainability.
♻️ Proposed fix
- // 获取 Log4j2 核心 Logger
+ // Get Log4j2 core Logger
logger = (Logger) LogManager.getLogger("DeferredLoggerTest");
- // 创建 ListAppender 用于捕获日志
+ // Create ListAppender to capture log events
listAppender = new TestListAppender("TestAppender");
listAppender.start();
- // 将 ListAppender 添加到 logger
+ // Attach ListAppender to logger
logger.addAppender(listAppender);
- // 清空 DeferredLogger 状态
+ // Clear DeferredLogger state
DeferredLogger.disable();
DeferredLogCache.clear();And in the test method:
- // 回放缓存
+ // Replay cached logs
DeferredLogCache.replayTo();
- // 断言日志已被输出（MemoryAppender 捕获）
+ // Assert logs have been output (captured by MemoryAppender)
assertTrue(containsMessage("errorLogger_testDeferredLog"));Also applies to: 50-50, 54-54, 57-57, 112-112, 115-115
🤖 Prompt for AI Agents
In
@apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/DeferredLoggerTest.java
around lines 47 - 48, Replace the Chinese inline comments in
DeferredLoggerTest.java with clear English equivalents while preserving their
original meaning; specifically update the comment above the logger assignment
for logger = (Logger) LogManager.getLogger("DeferredLoggerTest") and all other
Chinese comments related to test setup and assertions in this class so they are
written in English and describe the intent of the code (e.g., "Get core Log4j2
Logger", "Initialize deferred logger", "Assert log output contains ...", etc.).
| Assertions.assertNotNull("No host address detected.", hostAddress); | ||
| Assertions.assertNotNull("No host name resolved.", hostName); |
There was a problem hiding this comment.
Fix incorrect parameter order in assertNotNull calls.
The assertNotNull parameter order has changed between JUnit 4 and JUnit 5. In JUnit 5, the actual value comes first, then the message. The current code checks if the string literals are not null (always true), failing to validate hostAddress and hostName.
🐛 Proposed fix
- Assertions.assertNotNull("No host address detected.", hostAddress);
- Assertions.assertNotNull("No host name resolved.", hostName);
+ Assertions.assertNotNull(hostAddress, "No host address detected.");
+ Assertions.assertNotNull(hostName, "No host name resolved.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Assertions.assertNotNull("No host address detected.", hostAddress); | |
| Assertions.assertNotNull("No host name resolved.", hostName); | |
| Assertions.assertNotNull(hostAddress, "No host address detected."); | |
| Assertions.assertNotNull(hostName, "No host name resolved."); |
🤖 Prompt for AI Agents
In @apollo-core/src/test/java/com/ctrip/framework/foundation/FoundationTest.java
around lines 45 - 46, The assertNotNull calls use the message as the first
argument (JUnit 5 expects actual then message), so swap the arguments in the two
calls that reference Assertions.assertNotNull so they pass hostAddress and
hostName as the first parameter and the corresponding messages ("No host address
detected." and "No host name resolved.") as the second; update the two
Assertions.assertNotNull usages that reference hostAddress and hostName
accordingly.
| import org.junit.runner.RunWith; | ||
| import org.mockito.Mock; | ||
| import org.mockito.junit.MockitoJUnitRunner; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused JUnit 4 imports.
The imports org.junit.runner.RunWith and org.mockito.junit.MockitoJUnitRunner are no longer needed after migrating to JUnit 5 with @ExtendWith(MockitoExtension.class).
🧹 Proposed fix to remove unused imports
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.extension.ExtendWith;
-import org.junit.runner.RunWith;
import org.mockito.Mock;
-import org.mockito.junit.MockitoJUnitRunner;
import org.mockito.junit.jupiter.MockitoExtension;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@apollo-openapi/src/test/java/com/ctrip/framework/apollo/openapi/client/service/AbstractOpenApiServiceTest.java
around lines 32 - 34, Remove the unused JUnit4/Mockito runner imports: delete
the import lines for org.junit.runner.RunWith and
org.mockito.junit.MockitoJUnitRunner from AbstractOpenApiServiceTest.java since
the test now uses JUnit5’s @ExtendWith(MockitoExtension.class); ensure only the
necessary JUnit5/Mockito imports remain (e.g.,
org.junit.jupiter.api.extension.ExtendWith and
org.mockito.junit.jupiter.MockitoExtension) and run the tests to verify no
unused-import warnings remain.
| <dependency> | ||
| <groupId>org.apache.httpcomponents</groupId> | ||
| <artifactId>httpclient</artifactId> | ||
| <version>4.5.14</version> | ||
| </dependency> |
There was a problem hiding this comment.
Upgrade to Apache HttpClient 5.x for Spring Boot 4.0.1 compatibility.
Apache HttpClient 4.5.14 is incompatible with Spring Boot 4.0.1. Spring Framework 6+ (used by Spring Boot 4) requires Apache HttpClient 5.x (>= 5.1) for Spring's HttpComponentsClientHttpRequestFactory and RestTemplate integrations. Replace this dependency with org.apache.hc.client5:httpclient5 version 5.1 or newer.
🔄 Proposed fix
<dependency>
- <groupId>org.apache.httpcomponents</groupId>
- <artifactId>httpclient</artifactId>
- <version>4.5.14</version>
+ <groupId>org.apache.hc.client5</groupId>
+ <artifactId>httpclient5</artifactId>
+ <version>5.1</version>
</dependency>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <dependency> | |
| <groupId>org.apache.httpcomponents</groupId> | |
| <artifactId>httpclient</artifactId> | |
| <version>4.5.14</version> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.apache.hc.client5</groupId> | |
| <artifactId>httpclient5</artifactId> | |
| <version>5.1</version> | |
| </dependency> |
🤖 Prompt for AI Agents
In @pom.xml around lines 173 - 177, The pom currently declares Apache HttpClient
4.x (org.apache.httpcomponents:httpclient 4.5.14) which is incompatible with
Spring Boot 4 / Spring Framework 6; replace that dependency with the HttpClient
5 artifact (org.apache.hc.client5:httpclient5) at version 5.1 or newer, updating
the <dependency> entry in the pom accordingly and ensuring any code that uses
legacy org.apache.http.* classes or types (e.g., code building
HttpComponentsClientHttpRequestFactory or RestTemplate integrations) is adjusted
to use the HttpClient 5 APIs or compatible Spring adapters.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (1)
19-50: Remove unused JUnit 4 imports.Lines 29-31 and 38 contain JUnit 4 imports (
org.junit.*) that are no longer needed after the migration to JUnit 5. These should be removed to prevent confusion and ensure clean migration.🧹 Proposed fix to remove unused imports
import com.google.common.collect.Sets; +import java.util.List; -import org.junit.AfterClass; -import org.junit.BeforeClass; -import org.junit.experimental.runners.Enclosed; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.junit.runner.RunWith; import org.mockito.Mockito;
🤖 Fix all issues with AI agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java:
- Around line 109-118: In BootstrapConfigTest.test() (and other test methods
showing commented assertions) remove or restore the commented-out assertions:
either uncomment the assertions (e.g., Assertions.assertNotNull(testBean);
Assertions.assertTrue(testBean.execute()); assertEquals(mockedConfig, config);
assertEquals(someValue, someInjectedValue);) so the test verifies expected
behavior, or delete the commented lines entirely to avoid dead code; update any
necessary imports or setup in the test class (BootstrapConfigTest) so the
uncommented assertions compile and run, and ensure no unused variables remain if
you remove the lines.
- Around line 55-56: Remove the JUnit 4 @RunWith(Enclosed.class) annotation from
the BootstrapConfigTest class declaration (the class named BootstrapConfigTest)
and delete any corresponding JUnit 4 import (org.junit.runner.RunWith and
org.junit.runners.Enclosed); the nested test classes are already annotated with
@Nested so no replacement is needed—ensure the file only uses JUnit 5
annotations and imports.
- Around line 454-468: The test and factory registration use the deprecated
EnvironmentPostProcessor FQCN; update the import in BootstrapConfigTest to use
org.springframework.boot.EnvironmentPostProcessor and ensure any references to
EnvironmentPostProcessor in tests (e.g., the SpringFactoriesLoader.loadFactories
call and the class name check for
"com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer")
remain unchanged, and update the spring.factories entry key from
org.springframework.boot.env.EnvironmentPostProcessor to
org.springframework.boot.EnvironmentPostProcessor so the initializer is
discovered under Spring Boot 4.0.1.
- Around line 130-156: The nested test classes use JUnit 4 annotations
@BeforeClass and @AfterClass on the static methods beforeClass and afterClass;
replace those with JUnit 5 @BeforeAll and @AfterAll and update the imports to
org.junit.jupiter.api.BeforeAll/AfterAll (or annotate the nested class with
@TestInstance(PER_CLASS) if you prefer non-static lifecycle methods); apply this
change consistently for all nested classes that currently use
@BeforeClass/@AfterClass (e.g., the blocks around beforeClass/afterClass at
lines shown) so the lifecycle annotations match @Nested and JUnit 5.
🧹 Nitpick comments (1)
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (1)
281-285: Remove commented-out @ExtendWith annotations.Lines 281, 318, 355, 392, and 425 have commented-out
@ExtendWith(SpringExtension.class)annotations that are immediately followed by active versions. These commented lines should be removed for cleaner code.🧹 Example cleanup
- // @ExtendWith(SpringExtension.class) @Nested @SpringBootTest(classes = ConfigurationWithoutConditionalOnProperty.class) @DirtiesContext @ExtendWith(SpringExtension.class)Also applies to: 318-321, 355-359, 392-395, 425-429
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.javapom.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-23T01:12:46.767Z
Learnt from: nobodyiam
Repo: apolloconfig/apollo-java PR: 70
File: apollo-client/src/test/java/com/ctrip/framework/apollo/spring/XmlConfigPlaceholderTest.java:65-65
Timestamp: 2024-11-23T01:12:46.767Z
Learning: In test classes extending `AbstractSpringIntegrationTest`, the variable `someAppId` is already defined in the parent class and doesn't need to be redefined.
Applied to files:
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
🪛 GitHub Actions: build
pom.xml
[error] 1-1: Fatal error compiling: invalid target release: 17. The Maven compiler is configured to target Java 17, but the build environment does not support it. Ensure the JDK version in use supports target release 17 or adjust the project to a supported Java version. Command: mvn -rf :apollo-core
🔇 Additional comments (8)
pom.xml (7)
361-363: LGTM! Monitor for additional module access requirements.The
--add-opens java.base/java.util=ALL-UNNAMEDconfiguration is appropriate for Java 17+ to allow reflection access during testing. However, additional packages might need to be opened as you encounter issues during testing.Common additional opens that might be needed:
--add-opens java.base/java.lang=ALL-UNNAMED--add-opens java.base/java.lang.reflect=ALL-UNNAMEDMonitor test failures for "InaccessibleObjectException" errors that indicate additional
--add-opensflags are required.
64-64: The GitHub Actions build workflow already includes Java 17 in its test matrix, so no update to.github/workflows/build.ymlis required. The pipeline failure is likely due to a different cause and should be investigated separately.Likely an incorrect or invalid review comment.
71-71: Version 3.5.4 is the current stable release. The upgrade to maven-surefire-plugin 3.x is appropriate for JUnit 5 support, and version 3.5.4 is available on Maven Central. Code change approved.
173-177: This dependency is already used in the apollo-openapi module.The
httpclientdependency is needed byapollo-openapi, which usesCloseableHttpClientacross multiple service classes (ApolloOpenApiClient,AbstractOpenApiService, and all*OpenApiServiceimplementations). Version 4.5.14 is the current stable release of HttpClient 4.x. The dependencyManagement declaration in the parent pom.xml is the correct approach to ensure version consistency across the multi-module project.
68-68: No action needed. junitperf 1.37.0 is compatible with JUnit 5/Jupiter and includes official Jupiter bindings (released Oct 4, 2024 with junit-jupiter 5.9.0 dependency).
105-105: Correct Spring Boot version requirement: SLF4J 2.x is for Spring Boot 3.x, not 4.x.The upgrade to SLF4J 2.0.17 is necessary for Spring Boot 3.x (3.0+) compatibility. Version 2.0.17 is the latest stable release.
Likely an incorrect or invalid review comment.
66-66: Spring Boot 4.0.1 is valid and current.Spring Boot 4.0.1 was released December 18, 2025, and is available in Maven Central. It is the latest stable version. No action needed.
apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java (1)
470-496: LGTM!The static configuration classes and test bean are well-structured and appropriate for the test setup.
| @RunWith(Enclosed.class) | ||
| public class BootstrapConfigTest { |
There was a problem hiding this comment.
Remove JUnit 4 @RunWith annotation.
Line 55 still has @RunWith(Enclosed.class), which is a JUnit 4 annotation. In JUnit 5, the @Nested annotation (already applied to the inner classes) handles nested test execution without requiring @RunWith. This annotation must be removed for proper JUnit 5 migration.
🧹 Proposed fix to remove @RunWith
-@RunWith(Enclosed.class)
public class BootstrapConfigTest {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @RunWith(Enclosed.class) | |
| public class BootstrapConfigTest { | |
| public class BootstrapConfigTest { |
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
around lines 55 - 56, Remove the JUnit 4 @RunWith(Enclosed.class) annotation
from the BootstrapConfigTest class declaration (the class named
BootstrapConfigTest) and delete any corresponding JUnit 4 import
(org.junit.runner.RunWith and org.junit.runners.Enclosed); the nested test
classes are already annotated with @Nested so no replacement is needed—ensure
the file only uses JUnit 5 annotations and imports.
| @Test | ||
| public void test() throws Exception { | ||
| // Assertions.assertNotNull(testBean); | ||
| // Assertions.assertTrue(testBean.execute()); | ||
|
|
||
| Assert.assertEquals(someValue, someInjectedValue); | ||
| // assertEquals(mockedConfig, config); | ||
| // | ||
| // assertEquals(someValue, someInjectedValue); | ||
| } | ||
| } |
There was a problem hiding this comment.
Uncomment or remove incomplete test assertions.
Lines 111-117 contain commented-out test assertions. This pattern also appears in other test methods (lines 160-162, 201-203). Either uncomment these assertions if the tests should verify behavior, or remove them if they're no longer needed.
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
around lines 109 - 118, In BootstrapConfigTest.test() (and other test methods
showing commented assertions) remove or restore the commented-out assertions:
either uncomment the assertions (e.g., Assertions.assertNotNull(testBean);
Assertions.assertTrue(testBean.execute()); assertEquals(mockedConfig, config);
assertEquals(someValue, someInjectedValue);) so the test verifies expected
behavior, or delete the commented lines entirely to avoid dead code; update any
necessary imports or setup in the test class (BootstrapConfigTest) so the
uncommented assertions compile and run, and ensure no unused variables remain if
you remove the lines.
| @BeforeClass | ||
| public static void beforeClass() throws Exception { | ||
| doSetUp(); | ||
|
|
||
| System.setProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_ENABLED, "true"); | ||
| System.setProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_NAMESPACES, | ||
| String.format("%s, %s", ConfigConsts.NAMESPACE_APPLICATION, FX_APOLLO_NAMESPACE)); | ||
| System.setProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_ENABLED, "true"); | ||
| System.setProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_NAMESPACES, | ||
| String.format("%s, %s", ConfigConsts.NAMESPACE_APPLICATION, FX_APOLLO_NAMESPACE)); | ||
|
|
||
| Config config = mock(Config.class); | ||
| Config anotherConfig = mock(Config.class); | ||
| Config config = mock(Config.class); | ||
| Config anotherConfig = mock(Config.class); | ||
|
|
||
| when(config.getPropertyNames()).thenReturn(Sets.newHashSet(TEST_BEAN_CONDITIONAL_ON_KEY)); | ||
| when(config.getProperty(eq(TEST_BEAN_CONDITIONAL_ON_KEY), Mockito.nullable(String.class))).thenReturn(Boolean.TRUE.toString()); | ||
| when(config.getPropertyNames()).thenReturn( | ||
| Sets.newHashSet(TEST_BEAN_CONDITIONAL_ON_KEY)); | ||
| when(config.getProperty(eq(TEST_BEAN_CONDITIONAL_ON_KEY), | ||
| Mockito.nullable(String.class))).thenReturn(Boolean.TRUE.toString()); | ||
|
|
||
| mockConfig(someAppId, ConfigConsts.NAMESPACE_APPLICATION, anotherConfig); | ||
| mockConfig(someAppId, FX_APOLLO_NAMESPACE, config); | ||
| } | ||
| mockConfig(someAppId, ConfigConsts.NAMESPACE_APPLICATION, anotherConfig); | ||
| mockConfig(someAppId, FX_APOLLO_NAMESPACE, config); | ||
| } | ||
|
|
||
| @AfterClass | ||
| public static void afterClass() throws Exception { | ||
| System.clearProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_ENABLED); | ||
| System.clearProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_NAMESPACES); | ||
| @AfterClass | ||
| public static void afterClass() throws Exception { | ||
| System.clearProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_ENABLED); | ||
| System.clearProperty(PropertySourcesConstants.APOLLO_BOOTSTRAP_NAMESPACES); | ||
|
|
||
| doTearDown(); | ||
| } | ||
| doTearDown(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file
fd "BootstrapConfigTest.java" --type fRepository: apolloconfig/apollo-java
Length of output: 154
🏁 Script executed:
# Read the test file to understand the class structure
wc -l apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.javaRepository: apolloconfig/apollo-java
Length of output: 158
🏁 Script executed:
# Read the beginning of the file to understand class structure and JUnit version
head -150 apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java | cat -nRepository: apolloconfig/apollo-java
Length of output: 6740
🏁 Script executed:
# Continue reading to see the afterClass and look for more nested classes
sed -n '150,200p' apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java | cat -nRepository: apolloconfig/apollo-java
Length of output: 2302
🏁 Script executed:
# Check the structure more carefully - look at where @Nested is used and the class declarations
sed -n '61,130p' apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java | cat -nRepository: apolloconfig/apollo-java
Length of output: 2812
🏁 Script executed:
# Get the exact class declarations to understand the nesting structure
grep -n "^ class\|^ @Nested\|^public class" apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.javaRepository: apolloconfig/apollo-java
Length of output: 1115
🏁 Script executed:
# Check which annotations are used in each nested class
for line in 65 124 169 210 249 286 322 360 396 430; do
echo "=== Class starting at line $line ==="
sed -n "${line},$((line+50))p" apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java | grep -E "@Before|@After" | head -4
doneRepository: apolloconfig/apollo-java
Length of output: 989
🏁 Script executed:
# Let me check the Java version requirements in the project
fd "pom.xml" --type f | head -1 | xargs grep -A5 -B5 "source\|target" 2>/dev/null | head -30Repository: apolloconfig/apollo-java
Length of output: 50
🏁 Script executed:
# Check the parent class to understand the structure
sed -n '56,60p' apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java | cat -nRepository: apolloconfig/apollo-java
Length of output: 293
🏁 Script executed:
# Let me verify if static methods in non-static nested classes are actually legal in Java
# by checking the imports more carefully
head -50 apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java | grep -E "import.*junit|import.*Nested"Repository: apolloconfig/apollo-java
Length of output: 521
🏁 Script executed:
# Verify: Can non-static nested classes have static methods in Java?
# Let's create a simple test to confirm
python3 << 'EOF'
# In Java, a non-static nested class (inner class) CAN have static methods
# This is a valid Java construct. The reviewer's claim is incorrect.
# Let me verify the actual issue: mixing JUnit 4 and JUnit 5 annotations
print("Analysis of the lifecycle annotations:")
print("\n1. @Nested is a JUnit 5 (JUnit Jupiter) annotation")
print("2. @BeforeAll/@AfterAll are JUnit 5 lifecycle annotations")
print("3. @BeforeClass/@AfterClass are JUnit 4 lifecycle annotations")
print("\n4. Java allows static methods in non-static (inner) nested classes")
print(" This is NOT an error in Java syntax")
print("\n5. The real issue: Inconsistent use of JUnit versions")
print(" - First nested class uses @BeforeAll/@AfterAll (correct for @Nested)")
print(" - Other nested classes use @BeforeClass/@AfterClass (incorrect for @Nested)")
print("\nConclusion: Original review comment has incorrect Java syntax analysis,")
print("but identifies a real issue with JUnit version mismatch.")
EOFRepository: apolloconfig/apollo-java
Length of output: 154
🏁 Script executed:
# Verify the annotation imports and first nested class usage
sed -n '29,35p' apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java && echo "---" && sed -n '83,84p' apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.javaRepository: apolloconfig/apollo-java
Length of output: 414
Replace JUnit 4 lifecycle annotations with JUnit 5 equivalents in @nested test classes.
Lines 130 and 150 use @BeforeClass and @AfterClass (JUnit 4 annotations), but the nested class is decorated with @Nested (JUnit 5). For JUnit 5 nested test classes, use @BeforeAll and @AfterAll instead, as demonstrated in the first nested test class at lines 83-84.
This pattern inconsistently appears across all nested test classes in this file (lines 124-156, 169-204, 210-243, 249-280, 286-315, 322-354, 360-389, 396-424, 430-452). Most use the incorrect JUnit 4 annotations with the JUnit 5 @Nested decorator.
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
around lines 130 - 156, The nested test classes use JUnit 4 annotations
@BeforeClass and @AfterClass on the static methods beforeClass and afterClass;
replace those with JUnit 5 @BeforeAll and @AfterAll and update the imports to
org.junit.jupiter.api.BeforeAll/AfterAll (or annotate the nested class with
@TestInstance(PER_CLASS) if you prefer non-static lifecycle methods); apply this
change consistently for all nested classes that currently use
@BeforeClass/@AfterClass (e.g., the blocks around beforeClass/afterClass at
lines shown) so the lifecycle annotations match @Nested and JUnit 5.
| @Test | ||
| public void test() { | ||
| List<EnvironmentPostProcessor> names = SpringFactoriesLoader.loadFactories( | ||
| EnvironmentPostProcessor.class, getClass().getClassLoader()); | ||
| boolean containsApollo = false; | ||
| for (EnvironmentPostProcessor name : names) { | ||
| if (name.getClass().getName().equals( | ||
| "com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer")) { | ||
| containsApollo = true; | ||
| break; | ||
| } | ||
| } | ||
| Assertions.assertTrue(containsApollo); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find pom.xml or build files to check Spring Boot version
find . -name "pom.xml" -o -name "build.gradle" | head -20Repository: apolloconfig/apollo-java
Length of output: 337
🏁 Script executed:
# Search for spring.factories file
find . -path "*/META-INF/spring.factories" -o -path "*/META-INF/spring/*" | head -20Repository: apolloconfig/apollo-java
Length of output: 445
🏁 Script executed:
# Check for Apollo's EnvironmentPostProcessor registration
rg "ApolloApplicationContextInitializer" --type java -A 5 -B 5 | head -100Repository: apolloconfig/apollo-java
Length of output: 15147
🏁 Script executed:
# Search for SpringFactoriesLoader usage in the codebase
rg "SpringFactoriesLoader" --type java -B 3 -A 3 | head -100Repository: apolloconfig/apollo-java
Length of output: 1971
🏁 Script executed:
# Check pom.xml for Spring Boot version
head -100 pom.xml | rg "spring-boot|parent" -A 2 -B 2Repository: apolloconfig/apollo-java
Length of output: 312
🏁 Script executed:
# Check the spring.factories files content
cat apollo-client/src/main/resources/META-INF/spring.factoriesRepository: apolloconfig/apollo-java
Length of output: 461
🏁 Script executed:
# Check the AutoConfiguration.imports file
cat apollo-client/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.importsRepository: apolloconfig/apollo-java
Length of output: 130
🌐 Web query:
Spring Boot 4.0.1 SpringFactoriesLoader EnvironmentPostProcessor compatibility spring.factories
💡 Result:
Short answer: Spring Boot 4 changed how EnvironmentPostProcessor implementations are discovered — you must update integrations that rely on spring.factories.
Key points and action items
- The EnvironmentPostProcessor API was moved and refactored in Boot 4 (the interface moved from org.springframework.boot.env to org.springframework.boot and a new EnvironmentPostProcessorsFactory/EnvironmentPostProcessorApplicationListener support discovery). (github.com)
- Spring Boot 4 still supports loading processors from META-INF/spring.factories via EnvironmentPostProcessorsFactory.fromSpringFactories(...), but the package/class names referenced in your spring.factories entries must match the new API/locations (and the old deprecated form has been marked for removal). Update your spring.factories entries to the new FQCNs (or register via the new factory API). (docs.spring.io)
- Practically: update any spring.factories entries that reference the old EnvironmentPostProcessor FQCN to the new one(s) used by Boot 4, or replace your registration with an EnvironmentPostProcessorsFactory (e.g. EnvironmentPostProcessorsFactory.fromSpringFactories(classLoader) or EnvironmentPostProcessorsFactory.of(...)). Test startup to confirm processors are invoked in the expected order. (docs.spring.io)
If you want, tell me the exact spring.factories entries you have and I’ll show the precise updated lines for Spring Boot 4.0.1.
Citations:
- 1: https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-4.0-Migration-Guide?utm_source=openai
- 2: https://docs.spring.io/spring-boot/4.0/api/java/org/springframework/boot/support/EnvironmentPostProcessorsFactory.html?utm_source=openai
- 3: https://docs.spring.io/spring-boot/4.0/api/java/org/springframework/boot/support/EnvironmentPostProcessorsFactory.html?utm_source=openai
🌐 Web query:
Spring Boot 4.0 EnvironmentPostProcessor migration org.springframework.boot.env deprecated new location
💡 Result:
Short answer: In Spring Boot 4.0 the EnvironmentPostProcessor type was moved from org.springframework.boot.env to org.springframework.boot — i.e. use org.springframework.boot.EnvironmentPostProcessor now (update your imports and any META-INF/spring.factories entries). The old org.springframework.boot.env form is deprecated in 4.0 and will be removed later. [1][2][3]
References:
- Spring Boot 4.0 Migration Guide (package changes). [1]
- Spring Boot 4.0 API (deprecated note on old interface). [2]
- Spring Boot 4.0 release notes (replacement FQN). [3]
🏁 Script executed:
# Check if there are any other tests or usages of EnvironmentPostProcessor discovery
rg "loadFactories|EnvironmentPostProcessor" --type java -B 2 -A 2 | head -150Repository: apolloconfig/apollo-java
Length of output: 5328
Update EnvironmentPostProcessor imports and spring.factories for Spring Boot 4.0.1 compatibility.
Spring Boot 4.0 moved the EnvironmentPostProcessor interface from org.springframework.boot.env to org.springframework.boot. The deprecated org.springframework.boot.env.EnvironmentPostProcessor import in this test must be updated to the new location. Additionally, the spring.factories configuration file must update the factory key from org.springframework.boot.env.EnvironmentPostProcessor to org.springframework.boot.EnvironmentPostProcessor to comply with Spring Boot 4.0.1. The deprecated FQCN is marked for removal in future releases.
🤖 Prompt for AI Agents
In
@apollo-client/src/test/java/com/ctrip/framework/apollo/spring/BootstrapConfigTest.java
around lines 454 - 468, The test and factory registration use the deprecated
EnvironmentPostProcessor FQCN; update the import in BootstrapConfigTest to use
org.springframework.boot.EnvironmentPostProcessor and ensure any references to
EnvironmentPostProcessor in tests (e.g., the SpringFactoriesLoader.loadFactories
call and the class name check for
"com.ctrip.framework.apollo.spring.boot.ApolloApplicationContextInitializer")
remain unchanged, and update the spring.factories entry key from
org.springframework.boot.env.EnvironmentPostProcessor to
org.springframework.boot.EnvironmentPostProcessor so the initializer is
discovered under Spring Boot 4.0.1.
1 participant
What's the purpose of this PR
springboot 4.0.1 has released, but apollo-java not work well with it, so
upgrade spring-boot version 2.7.18 -> 4.0.1 to support springboot4
Which issue(s) this PR fixes:
Fixes #
fix some junit4 unit test replace by junit6
fix slf4j log replace by slf4j2
fix junit replace org.junit.Assert. by org.junit.jupiter.api.Assertions
Brief changelog
upgrade spring-boot version 2.7.18 -> 4.0.1
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.