[Improve] add unit test #6
Conversation
Signed-off-by: yuluo-yx <[email protected]>
Signed-off-by: yuluo-yx <[email protected]>
…beat into 0804-yuluo/tets-1
Signed-off-by: yuluo-yx <[email protected]>
…beat into 0804-yuluo/tets-1
Signed-off-by: yuluo-yx <[email protected]>
Signed-off-by: yuluo-yx <[email protected]>
Signed-off-by: yuluo-yx <[email protected]>
Signed-off-by: YuLuo <[email protected]>
Signed-off-by: yuluo-yx <[email protected]>
Signed-off-by: yuluo-yx <[email protected]>
|
yep. I think so. I am debug in my local. If has new discover, i will push. tomsun28 @.***>于2024年8月5日 周一12:37写道：
|
|
@tomsun28 I find question: pls see: spring-projects/spring-data-commons#2987 |
👍 it seem this can fix it. spring-projects/spring-data-commons#2987 (comment) add pageModule in jackson |
This would be due to additional class configuration, not sure if there are other side effects, I think it's better to use |
hi, the root cause is the serialization problem of the |
got it, let me try |
|
@tomsun28 hi, This e2e error is probably caused by apache#2486, I think we can run the ci test again after the apache#2486 merge. |
WalkthroughThis update introduces a new global Spring MVC configuration for JSON serialization and deserialization, specifically customizing date and time formats using Jackson. The new configuration replaces the previous Jackson customization that was scoped to the manager module, centralizing and unifying date/time handling across the application. Minor whitespace and formatting adjustments are made in alert controller classes, while the alert-related tests are refactored: one test class is simplified, and a new test class is added for alert silences. Additionally, the application's server port is updated in the configuration file. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SpringMVC (with MVCConfig)
participant Controller
participant Service
Client->>SpringMVC (with MVCConfig): Sends HTTP request with JSON (date/time fields)
SpringMVC (with MVCConfig)->>Controller: Deserializes JSON using custom ObjectMapper
Controller->>Service: Processes request
Service-->>Controller: Returns response data
Controller->>SpringMVC (with MVCConfig): Returns response object
SpringMVC (with MVCConfig)->>Client: Serializes response JSON with custom date/time formats
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
For this pr, the current state is that it may affect all controllers that use Page as a return value, but it does not affect front-end api calls. It only appears in e2e tests and unit tests, so this pr is in pending status, and will be updated again when there is a better solution for serializing Page objects in spring data jpa. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
common/src/main/java/org/apache/hertzbeat/common/config/MVCConfig.java (1)
51-95: Well-structured MVC configuration for date/time handling.The configuration properly sets up serialization and deserialization for Java 8 date/time types with clear format patterns. Adding the custom converter at index 0 ensures it takes precedence over default converters.
Consider adding a comment explaining why the converter is added at index 0:
messageConverter.setObjectMapper(objectMapper); converters.add(0, messageConverter); +// Add at index 0 to ensure our converter takes precedence over default converters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
alerter/src/main/java/org/apache/hertzbeat/alert/controller/AlertDefinesController.java(2 hunks)alerter/src/main/java/org/apache/hertzbeat/alert/controller/AlertSilencesController.java(1 hunks)alerter/src/test/java/org/apache/hertzbeat/alert/controller/AlertDefinesControllerTest.java(2 hunks)alerter/src/test/java/org/apache/hertzbeat/alert/controller/AlertSilencesControllerTest.java(1 hunks)common/src/main/java/org/apache/hertzbeat/common/config/MVCConfig.java(1 hunks)manager/src/main/java/org/apache/hertzbeat/manager/config/JacksonConfig.java(0 hunks)manager/src/main/resources/application.yml(1 hunks)
💤 Files with no reviewable changes (1)
- manager/src/main/java/org/apache/hertzbeat/manager/config/JacksonConfig.java
🔇 Additional comments (7)
alerter/src/main/java/org/apache/hertzbeat/alert/controller/AlertSilencesController.java (1)
59-60: Formatting adjustment looks good.The added blank line improves code readability by separating the method body from its parameter declarations.
alerter/src/main/java/org/apache/hertzbeat/alert/controller/AlertDefinesController.java (2)
64-65: Formatting improvement is good.The added spacing around the return statement improves readability.
83-85: Consistent formatting applied.The indentation of method parameters has been adjusted to maintain consistent formatting throughout the codebase.
alerter/src/test/java/org/apache/hertzbeat/alert/controller/AlertSilencesControllerTest.java (2)
42-67: Test setup looks good.The test class is well structured with proper mocking of dependencies and test data setup.
1-81:❌ Incorrect review comment
Verify serialization of Page objects in tests.
Based on PR discussions about serialization issues with Spring Data's
Pageobjects, verify that the tests properly handle serialization ofPageobjects returned by controllers. Consider adding a test that validates the JSON structure of the response.
🏁 Script executed:
#!/bin/bash # Find configuration classes that might handle Page serialization echo "Looking for configuration classes that might handle Page serialization..." rg -A 10 "ObjectMapper|Jackson" --type java echo "Checking for PageModule or similar Page serialization configuration..." rg -l "PageModule|Page.*serializ" --type javaLength of output: 78410
Ignore Page serialization suggestion for this test.
AlertSilencesControllerTest only covers the DELETE
/api/alert/silencesendpoint—which does not return a Spring Data Page—so there’s no Page serialization to verify here. If you have other controller methods that return Page objects, please add dedicated JSON‑structure tests in those controllers’ test classes instead.Likely an incorrect or invalid review comment.
alerter/src/test/java/org/apache/hertzbeat/alert/controller/AlertDefinesControllerTest.java (1)
77-86: Test implementation looks good.The test for the delete operation correctly sets up the request and verifies the response.
common/src/main/java/org/apache/hertzbeat/common/config/MVCConfig.java (1)
60-61: Good implementation of the PageModule integration.This correctly addresses the serialization issues with
Page<T>objects as mentioned in the PR discussion. By integrating Spring Data'sPageModuleinto the Jackson ObjectMapper, you're ensuring proper serialization/deserialization of pagination results in JSON responses.This is the recommended approach for handling Spring Data's
Pageobjects in REST responses, as discussed in Spring Data Commons issue apache#2987.Also applies to: 89-91
| @@ -13,7 +13,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| server: | |||
| port: 1157 | |||
| port: 11570 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify port configuration change impact.
The server port has been updated from 1157 to 11570. This change should be coordinated with any service discovery configurations, deployment scripts, or documentation that references the previous port number.
🏁 Script executed:
#!/bin/bash
# Check for other references to the old port number in the codebase
echo "Searching for references to the old port 1157 that might need updating..."
rg -l "1157" --glob "!{manager/src/main/resources/application.yml}" -g "!*.{class,jar,war}"Length of output: 10356
Update all port 1157 references to 11570 across the codebase
Multiple occurrences of the old port (1157) were found in configuration files, scripts and documentation. To avoid runtime errors and configuration drift, please update every reference to use the new port (11570). Notable locations include:
- collector/src/main/resources/application.yml
- manager/src/main/resources/define/app-push.yml, app-hertzbeat*.yml
- script/docker-compose/**/conf/application.yml and docker-compose.yaml
- script/assembly/server/bin/startup.sh, startup.bat, shutdown.bat
- e2e/docker-compose.yml and e2e/testsuite.yaml
- README.md, README_CN.md
- Documentation under home/docs, home/blog and home/versioned_docs
You can run rg -l "1157" from the repo root to review all occurrences. Ensure your deployment scripts, service discovery settings and CI/CD workflows are updated to reference port 11570.
| @Test | ||
| void testDeleteAlertDefines() throws Exception { | ||
|
|
||
| doNothing().when(alertSilenceService).deleteAlertSilences(any()); | ||
|
|
||
| mockMvc.perform(delete("/api/alert/silences") | ||
| .param("ids", "1,2,3") | ||
| .accept(MediaType.APPLICATION_JSON)) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test method needs better naming and more test cases.
The test method name testDeleteAlertDefines doesn't match the actual controller method name deleteAlertSilences it's testing. Additionally, consider adding tests for edge cases like empty ID list and for the getAlertSilences method.
-@Test
-void testDeleteAlertDefines() throws Exception {
+@Test
+void testDeleteAlertSilences() throws Exception {
doNothing().when(alertSilenceService).deleteAlertSilences(any());
mockMvc.perform(delete("/api/alert/silences")
.param("ids", "1,2,3")
.accept(MediaType.APPLICATION_JSON))
.andExpect(status().isOk())
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE));
}
+
+@Test
+void testDeleteAlertSilences_withEmptyIdList() throws Exception {
+ mockMvc.perform(delete("/api/alert/silences")
+ .accept(MediaType.APPLICATION_JSON))
+ .andExpect(status().isOk())
+ .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE));
+}
+
+@Test
+void testGetAlertSilences() throws Exception {
+ Page<AlertSilence> mockPage = new PageImpl<>(List.of(alertSilence));
+ when(alertSilenceService.getAlertSilences(any(), any(), any(), any(), anyInt(), anyInt()))
+ .thenReturn(mockPage);
+
+ mockMvc.perform(get("/api/alert/silences")
+ .accept(MediaType.APPLICATION_JSON))
+ .andExpect(status().isOk())
+ .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE))
+ .andExpect(jsonPath("$.data.content[0].id").value(1));
+}📝 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 | |
| void testDeleteAlertDefines() throws Exception { | |
| doNothing().when(alertSilenceService).deleteAlertSilences(any()); | |
| mockMvc.perform(delete("/api/alert/silences") | |
| .param("ids", "1,2,3") | |
| .accept(MediaType.APPLICATION_JSON)) | |
| .andExpect(status().isOk()) | |
| .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)); | |
| } | |
| @Test | |
| void testDeleteAlertSilences() throws Exception { | |
| doNothing().when(alertSilenceService).deleteAlertSilences(any()); | |
| mockMvc.perform(delete("/api/alert/silences") | |
| .param("ids", "1,2,3") | |
| .accept(MediaType.APPLICATION_JSON)) | |
| .andExpect(status().isOk()) | |
| .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)); | |
| } | |
| @Test | |
| void testDeleteAlertSilences_withEmptyIdList() throws Exception { | |
| // no "ids" param | |
| mockMvc.perform(delete("/api/alert/silences") | |
| .accept(MediaType.APPLICATION_JSON)) | |
| .andExpect(status().isOk()) | |
| .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)); | |
| } | |
| @Test | |
| void testGetAlertSilences() throws Exception { | |
| // prepare a mock page of silences | |
| Page<AlertSilence> mockPage = new PageImpl<>(List.of(alertSilence)); | |
| when(alertSilenceService.getAlertSilences(any(), any(), any(), any(), anyInt(), anyInt())) | |
| .thenReturn(mockPage); | |
| mockMvc.perform(get("/api/alert/silences") | |
| .accept(MediaType.APPLICATION_JSON)) | |
| .andExpect(status().isOk()) | |
| .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)) | |
| .andExpect(jsonPath("$.data.content[0].id").value(1)); | |
| } |
| @ExtendWith(MockitoExtension.class) | ||
| @SpringBootTest(classes = Manager.class) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent testing approach.
The test class combines @SpringBootTest with @ExtendWith(MockitoExtension.class) and then manually creates the MockMvc instance using standaloneSetup(). This is an unusual combination that mixes different testing approaches.
Choose one of these approaches:
- For a focused controller test with mocks:
@ExtendWith(MockitoExtension.class)
-@SpringBootTest(classes = Manager.class)
class AlertDefinesControllerTest {
// Keep the standaloneSetup approach
}- For a Spring context-aware test:
-@ExtendWith(MockitoExtension.class)
@SpringBootTest(classes = Manager.class)
+@AutoConfigureMockMvc
class AlertDefinesControllerTest {
+ @Autowired
private MockMvc mockMvc;
@BeforeEach
void setUp() {
- this.mockMvc = standaloneSetup(alertDefinesController).build();
// Other setup code...
}
}Also applies to: 64-64
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
| import org.apache.catalina.Manager; |
There was a problem hiding this comment.
Incorrect import for Manager class.
The import org.apache.catalina.Manager is incorrect for this context. This is a Tomcat-specific interface, not a Spring Boot application class that should be used with @SpringBootTest(classes = Manager.class).
Replace with the correct import for your application's main class, which is likely something like org.apache.hertzbeat.manager.Manager based on the class reference on line 48.
| javaTimeModule.addSerializer(LocalDate.class, new LocalDateSerializer(dateTimeFormatter)); | ||
| javaTimeModule.addSerializer(LocalTime.class, new LocalTimeSerializer(timeFormatter)); | ||
|
|
||
| javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(dateTimeFormatter)); |
There was a problem hiding this comment.
Fix incorrect date format for LocalDateTime deserialization.
You're using the date formatter (dateTimeFormatter) for LocalDateTime deserialization, which only includes the date part (yyyy-MM-dd) without the time component.
Use the defaultDateTimeFormatter instead, which includes both date and time parts:
-javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(dateTimeFormatter));
+javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(defaultDateTimeFormatter));📝 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.
| javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(dateTimeFormatter)); | |
| javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(defaultDateTimeFormatter)); |
Summary by CodeRabbit