Skip to content

[Improve] add unit test #6

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ public ResponseEntity<Message<Page<AlertDefine>>> getAlertDefines(
@Parameter(description = "List current page", example = "0") @RequestParam(defaultValue = "0") int pageIndex,
@Parameter(description = "Number of list pages", example = "8") @RequestParam(defaultValue = "8") int pageSize) {
Page<AlertDefine> alertDefinePage = alertDefineService.getAlertDefines(ids, search, priority, sort, order, pageIndex, pageSize);
return ResponseEntity.ok(Message.success(alertDefinePage));

return ResponseEntity.ok(Message.success(alertDefinePage));
}

@DeleteMapping
Expand All @@ -79,9 +80,9 @@ public ResponseEntity<Message<Void>> deleteAlertDefines(
@GetMapping("/export")
@Operation(summary = "export alertDefine config", description = "export alarm definition configuration")
public void export(
@Parameter(description = "AlertDefine ID List", example = "656937901") @RequestParam List<Long> ids,
@Parameter(description = "Export Type:JSON,EXCEL,YAML") @RequestParam(defaultValue = "JSON") String type,
HttpServletResponse res) throws Exception {
@Parameter(description = "AlertDefine ID List", example = "656937901") @RequestParam List<Long> ids,
@Parameter(description = "Export Type:JSON,EXCEL,YAML") @RequestParam(defaultValue = "JSON") String type,
HttpServletResponse res) throws Exception {
alertDefineService.export(ids, type, res);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public ResponseEntity<Message<Page<AlertSilence>>> getAlertSilences(
@Parameter(description = "Sort mode: asc: ascending, desc: descending", example = "desc") @RequestParam(defaultValue = "desc") String order,
@Parameter(description = "List current page", example = "0") @RequestParam(defaultValue = "0") int pageIndex,
@Parameter(description = "Number of list pages", example = "8") @RequestParam(defaultValue = "8") int pageSize) {

Page<AlertSilence> alertSilencePage = alertSilenceService.getAlertSilences(ids, search, sort, order, pageIndex, pageSize);
return ResponseEntity.ok(Message.success(alertSilencePage));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,9 @@

package org.apache.hertzbeat.alert.controller;

import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.apache.catalina.Manager;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

import org.apache.hertzbeat.alert.service.AlertDefineService;
import org.apache.hertzbeat.common.constants.CommonConstants;
import org.apache.hertzbeat.common.entity.alerter.AlertDefine;
Expand All @@ -36,116 +29,60 @@
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.data.domain.PageImpl;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Sort;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.http.MediaType;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
import org.springframework.test.web.servlet.setup.MockMvcBuilders;

import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup;

/**
* Test case for {@link AlertDefinesController}
* Test whether the data mocked at the mock is correct, and test whether the format of the returned data is correct
*/

@ExtendWith(MockitoExtension.class)
@SpringBootTest(classes = Manager.class)
Comment on lines 47 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. For a focused controller test with mocks:
@ExtendWith(MockitoExtension.class)
-@SpringBootTest(classes = Manager.class)
class AlertDefinesControllerTest {
    // Keep the standaloneSetup approach
}
  1. 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

class AlertDefinesControllerTest {

private MockMvc mockMvc;

@InjectMocks
private AlertDefinesController alertDefinesController;
private MockMvc mockMvc;

@Mock
AlertDefineService alertDefineService;
@InjectMocks
private AlertDefinesController alertDefinesController;

// Parameters to avoid default values interference, default values have been replaced
List<Long> ids = Stream.of(6565463543L, 6565463544L).collect(Collectors.toList());
Byte priority = Byte.parseByte("1");
String sort = "gmtCreate";
String order = "asc";
Integer pageIndex = 1;
Integer pageSize = 7;

// Parameter collection
Map<String, Object> content = new HashMap<>();
@Mock
private AlertDefineService alertDefineService;

// Object for mock
PageRequest pageRequest;
private AlertDefine alertDefine;

// Since the specification is used in dynamic proxy, it cannot be mocked
// Missing debugging parameters are ids, priority
// The missing part has been manually output for testing
@BeforeEach
void setUp() {

@BeforeEach
void setUp() {
this.mockMvc = MockMvcBuilders.standaloneSetup(alertDefinesController).build();
content.put("ids", ids);
content.put("priority", priority);
content.put("sort", sort);
content.put("order", order);
content.put("pageIndex", pageIndex);
content.put("pageSize", pageSize);
Sort sortExp = Sort.by(new Sort.Order(Sort.Direction.fromString(content.get("order").toString()), content.get("sort").toString()));
pageRequest = PageRequest.of((Integer) content.get("pageIndex"), (Integer) content.get("pageSize"), sortExp);
}
this.mockMvc = standaloneSetup(alertDefinesController).build();

// @Test
// todo: fix this test
void getAlertDefines() throws Exception {
alertDefine = AlertDefine.builder()
.id(9L)
.app("linux")
.metric("disk")
.field("usage")
.expr("x")
.times(1)
.tags(new LinkedList<>())
.build();
}

// Test the correctness of the mock
// Although objects cannot be mocked, stubs can be stored using class files
// Mockito.when(alertDefineService.getAlertDefines(Mockito.any(Specification.class), Mockito.argThat(new ArgumentMatcher<PageRequest>() {
// @Override
// public boolean matches(PageRequest pageRequestMidden) {
// // There are three methods in the source code that need to be compared, namely getPageNumber(), getPageSize(), getSort()
// if(pageRequestMidden.getPageSize() == pageRequest.getPageSize() &&
// pageRequestMidden.getPageNumber() == pageRequest.getPageNumber() &&
// pageRequestMidden.getSort().equals(pageRequest.getSort())) {
// return true;
// }
// return false;
// }
// }))).thenReturn(new PageImpl<AlertDefine>(new ArrayList<AlertDefine>()));
AlertDefine define = AlertDefine.builder().id(9L).app("linux").metric("disk").field("usage").expr("x").times(1).tags(new LinkedList<>()).build();
Mockito.when(alertDefineService.getAlertDefines(null, null, null, "id", "desc", 1, 10)).thenReturn(new PageImpl<>(Collections.singletonList(define)));
@Test
void deleteAlertDefines() throws Exception {

mockMvc.perform(MockMvcRequestBuilders.get(
"/api/alert/defines")
.param("ids", ids.toString().substring(1, ids.toString().length() - 1))
.param("priority", priority.toString())
.param("sort", sort)
.param("order", order)
.param("pageIndex", pageIndex.toString())
.param("pageSize", pageSize.toString()))
.andExpect(status().isOk())
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE))
.andExpect(jsonPath("$.data.content").value(new ArrayList<>()))
.andExpect(jsonPath("$.data.pageable").value("INSTANCE"))
.andExpect(jsonPath("$.data.totalPages").value(1))
.andExpect(jsonPath("$.data.totalElements").value(0))
.andExpect(jsonPath("$.data.last").value(true))
.andExpect(jsonPath("$.data.number").value(0))
.andExpect(jsonPath("$.data.size").value(0))
.andExpect(jsonPath("$.data.first").value(true))
.andExpect(jsonPath("$.data.numberOfElements").value(0))
.andExpect(jsonPath("$.data.empty").value(true))
.andExpect(jsonPath("$.data.sort.empty").value(true))
.andExpect(jsonPath("$.data.sort.sorted").value(false))
.andExpect(jsonPath("$.data.sort.unsorted").value(true))
.andReturn();
}
this.mockMvc.perform(MockMvcRequestBuilders.delete("/api/alert/defines")
.contentType(MediaType.APPLICATION_JSON)
.content(JsonUtil.toJson(Collections.singletonList(1))))
.andExpect(status().isOk())
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE))
.andReturn();
}

@Test
void deleteAlertDefines() throws Exception {
this.mockMvc.perform(MockMvcRequestBuilders.delete("/api/alert/defines")
.contentType(MediaType.APPLICATION_JSON)
.content(JsonUtil.toJson(ids)))
.andExpect(status().isOk())
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE))
.andReturn();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hertzbeat.alert.controller;

import org.apache.hertzbeat.alert.service.AlertSilenceService;
import org.apache.hertzbeat.common.constants.CommonConstants;
import org.apache.hertzbeat.common.entity.alerter.AlertSilence;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.springframework.http.MediaType;
import org.springframework.test.web.servlet.MockMvc;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doNothing;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup;

/**
* test case for {@link AlertSilencesController}
*/

@ExtendWith(MockitoExtension.class)
class AlertSilencesControllerTest {

private MockMvc mockMvc;

@Mock
private AlertSilenceService alertSilenceService;

private AlertSilence alertSilence;

@InjectMocks
private AlertSilencesController alertSilencesController;

@BeforeEach
void setUp() {

this.mockMvc = standaloneSetup(alertSilencesController).build();

alertSilence = AlertSilence
.builder()
.id(1L)
.type((byte) 1)
.name("Test Silence")
.build();
}

@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));
}
Comment on lines +69 to +79
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
@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));
}


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.hertzbeat.common.config;

import java.text.SimpleDateFormat;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.format.DateTimeFormatter;
import java.util.List;
import java.util.TimeZone;

import com.fasterxml.jackson.annotation.JsonAutoDetect;
import com.fasterxml.jackson.annotation.PropertyAccessor;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import com.fasterxml.jackson.datatype.jsr310.deser.LocalDateDeserializer;
import com.fasterxml.jackson.datatype.jsr310.deser.LocalDateTimeDeserializer;
import com.fasterxml.jackson.datatype.jsr310.deser.LocalTimeDeserializer;
import com.fasterxml.jackson.datatype.jsr310.ser.LocalDateSerializer;
import com.fasterxml.jackson.datatype.jsr310.ser.LocalDateTimeSerializer;
import com.fasterxml.jackson.datatype.jsr310.ser.LocalTimeSerializer;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.context.annotation.Configuration;
import org.springframework.data.web.config.SpringDataJacksonConfiguration;
import org.springframework.http.converter.HttpMessageConverter;
import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter;
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;

/**
* MVC Configuration.
*/

@Configuration
public class MVCConfig implements WebMvcConfigurer {

public static final String DEFAULT_DATE_TIME_FORMAT = "yyyy-MM-dd'T'HH:mm:ss";

public static final String DEFAULT_DATE_FORMAT = "yyyy-MM-dd";

public static final String DEFAULT_TIME_FORMAT = "HH:mm:ss";

@Autowired
private SpringDataJacksonConfiguration.PageModule pageModule;

@Override
public void extendMessageConverters(List<HttpMessageConverter<?>> converters) {

MappingJackson2HttpMessageConverter messageConverter = new MappingJackson2HttpMessageConverter();

ObjectMapper objectMapper = new ObjectMapper();
objectMapper.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.ANY);
objectMapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);

final SimpleDateFormat simpleDateFormat = new SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT);
simpleDateFormat.setTimeZone(TimeZone.getDefault());

JavaTimeModule javaTimeModule = new JavaTimeModule();

DateTimeFormatter defaultDateTimeFormatter = DateTimeFormatter.ofPattern(DEFAULT_DATE_TIME_FORMAT);
DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(DEFAULT_DATE_FORMAT);
DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern(DEFAULT_TIME_FORMAT);

javaTimeModule.addSerializer(LocalDateTime.class, new LocalDateTimeSerializer(defaultDateTimeFormatter));
javaTimeModule.addSerializer(LocalDate.class, new LocalDateSerializer(dateTimeFormatter));
javaTimeModule.addSerializer(LocalTime.class, new LocalTimeSerializer(timeFormatter));

javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(dateTimeFormatter));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(dateTimeFormatter));
javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(defaultDateTimeFormatter));

javaTimeModule.addDeserializer(LocalDate.class, new LocalDateDeserializer(dateTimeFormatter));
javaTimeModule.addDeserializer(LocalTime.class, new LocalTimeDeserializer(timeFormatter));

objectMapper.registerModule(javaTimeModule)
.registerModule(pageModule)
.setDateFormat(simpleDateFormat);

messageConverter.setObjectMapper(objectMapper);
converters.add(0, messageConverter);
}
}
Loading
Loading