Skip to content

Commit

Permalink
[ISSUE #4483] Standardize exception handling in tests (#4484)
Browse files Browse the repository at this point in the history
* Fix EventMeshGrpcProducerTest#testPublishWithException

Although EventMeshGrpcProducerTest#testPublishWithException passes,
it's wrong to do so.
This tests calls the EventMeshGrpcProducer#produce method with a valid
EventMeshMessage instance, so it does not, in fact, throw an error.
The test then doesn't fail on the following line. Since no exception
is ever thrown, the catch block is never reached, and the assertion in
it is never called.

This patch fixes the test by passing a truly invalid message (just a
string), and adds an explicit failure if an exception is not called.

* Standardize exception handling in tests

Following 7508251, the upgrade to
JUnit Jupiter allows us to standardize the way exceptions are handled
in tests and reduce boilerplate code.

- There is no need to catch an exception and explicitly fail the test.
  The test should be allowed to throw the exception and error-out,
  which is technically the correct behavior (the test didn't really
  fail on any logic test).
  - In the cases where the only assertion was explicitly failing the
    test in case an exception was thrown,
    Assertions.assertDoesNotThrow was used instead.
- In order to test cases when an exception must be thrown,
  Assertions.assertThrows can be used. There is no need to catch the
  exception and assert it's not null.
  - It's also worth noting that some instances that used the idiom of
    catching an expected exception did not fail the test in case the
    exception was never thrown, meaning this patch not only improves
    the tests style, but also their correctness.

Fixes #4483
  • Loading branch information
mureinik authored Oct 14, 2023
1 parent d327ff3 commit 1a35984
Show file tree
Hide file tree
Showing 20 changed files with 87 additions and 296 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,8 @@ public class ResetCountDownLatchTest {

@Test
public void testConstructorParameterError() {
try {
new ResetCountDownLatch(-1);
} catch (IllegalArgumentException e) {
Assertions.assertEquals(e.getMessage(), "count must be greater than or equal to 0");
}
IllegalArgumentException e = Assertions.assertThrows(IllegalArgumentException.class, () -> new ResetCountDownLatch(-1));
Assertions.assertEquals(e.getMessage(), "count must be greater than or equal to 0");
ResetCountDownLatch resetCountDownLatch = new ResetCountDownLatch(1);
Assertions.assertEquals(1, resetCountDownLatch.getCount());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,8 @@ public void testLoadPropertiesWhenFileExist() throws Exception {
configService.setRootConfig("classPath://configuration.properties");
properties = configService.getRootConfig();
String path = configService.getRootPath();
try {
PropertiesUtils.loadPropertiesWhenFileExist(properties, path);
Assertions.assertEquals("env-succeed!!!", properties.get("eventMesh.server.env").toString());
Assertions.assertEquals("idc-succeed!!!", properties.get("eventMesh.server.idc").toString());
} catch (Exception e) {
Assertions.fail();
}
PropertiesUtils.loadPropertiesWhenFileExist(properties, path);
Assertions.assertEquals("env-succeed!!!", properties.get("eventMesh.server.env").toString());
Assertions.assertEquals("idc-succeed!!!", properties.get("eventMesh.server.idc").toString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@
import org.apache.eventmesh.webhook.admin.AdminWebHookConfigOperationManager;
import org.apache.eventmesh.webhook.api.WebHookConfigOperation;

import java.io.IOException;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
Expand Down Expand Up @@ -85,13 +83,8 @@ public void testStart() throws Exception {
try (MockedStatic<HttpServer> dummyStatic = Mockito.mockStatic(HttpServer.class)) {
HttpServer server = mock(HttpServer.class);
dummyStatic.when(() -> HttpServer.create(any(), anyInt())).thenReturn(server);
try {
Mockito.doNothing().when(adminController).run(server);
controller.start();
} catch (IOException e) {
Assertions.fail(e.getMessage());
}

Mockito.doNothing().when(adminController).run(server);
Assertions.assertDoesNotThrow(controller::start);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,16 @@ public void init() throws Exception {

@Test
public void testHandler() {
try {
WebHookProcessor processor = new WebHookProcessor();
processor.setWebHookController(controller);
processor.handler(buildMockWebhookRequest());

CloudEvent msgSendToMq = captor.getValue();
Assertions.assertNotNull(msgSendToMq);
Assertions.assertTrue(StringUtils.isNoneBlank(msgSendToMq.getId()));
Assertions.assertEquals("www.github.com", msgSendToMq.getSource().getPath());
Assertions.assertEquals("github.ForkEvent", msgSendToMq.getType());
Assertions.assertEquals(BytesCloudEventData.wrap("\"mock_data\":0".getBytes(StandardCharsets.UTF_8)), msgSendToMq.getData());
} catch (Exception e) {
Assertions.fail(e.getMessage());
}
WebHookProcessor processor = new WebHookProcessor();
processor.setWebHookController(controller);
processor.handler(buildMockWebhookRequest());

CloudEvent msgSendToMq = captor.getValue();
Assertions.assertNotNull(msgSendToMq);
Assertions.assertTrue(StringUtils.isNoneBlank(msgSendToMq.getId()));
Assertions.assertEquals("www.github.com", msgSendToMq.getSource().getPath());
Assertions.assertEquals("github.ForkEvent", msgSendToMq.getType());
Assertions.assertEquals(BytesCloudEventData.wrap("\"mock_data\":0".getBytes(StandardCharsets.UTF_8)), msgSendToMq.getData());
}

private HttpRequest buildMockWebhookRequest() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ public class BannerUtilTest {

@Test
public void testGenerateBanner() {
try {
BannerUtil.generateBanner();
} catch (Exception e) {
Assertions.fail("BannerUtil.generateBanner() test failed");
}
Assertions.assertDoesNotThrow(BannerUtil::generateBanner);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,9 @@ public void testCloneObject() throws IOException, ClassNotFoundException {

@Test
public void testPrintState() {
try {
ScheduledExecutorService serviceRebalanceScheduler = ThreadPoolFactory
.createScheduledExecutor(5, new EventMeshThreadFactory("proxy-rebalance-sch", true));
EventMeshUtil.printState((ThreadPoolExecutor) serviceRebalanceScheduler);
} catch (Exception e) {
Assertions.fail(e.getMessage());
}
ScheduledExecutorService serviceRebalanceScheduler = ThreadPoolFactory
.createScheduledExecutor(5, new EventMeshThreadFactory("proxy-rebalance-sch", true));
Assertions.assertDoesNotThrow(() -> EventMeshUtil.printState((ThreadPoolExecutor) serviceRebalanceScheduler));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,11 @@
public class ValueComparatorTest {

@Test
public void testSerializeOrderedCollection() {
public void testSerializeOrderedCollection() throws IOException {
Map<Map.Entry<String, Integer>, Integer> map = new TreeMap<>(new ValueComparator());
try (OutputStream bos = new ByteArrayOutputStream();
ObjectOutputStream oos = new ObjectOutputStream(bos)) {
oos.writeObject(map);
} catch (IOException e) {
Assertions.fail();
Assertions.assertDoesNotThrow(() -> oos.writeObject(map));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
public class WebhookUtilTest {

@Test
public void testObtainDeliveryAgreement() {
public void testObtainDeliveryAgreement() throws Exception {
// normal case
try (CloseableHttpClient httpClient = mock(CloseableHttpClient.class);
CloseableHttpResponse response = mock(CloseableHttpResponse.class);
Expand All @@ -54,15 +54,8 @@ public void testObtainDeliveryAgreement() {

// abnormal case
Mockito.when(httpClient2.execute(any())).thenThrow(new RuntimeException());
try {
Assertions.assertTrue(WebhookUtil.obtainDeliveryAgreement(httpClient2, "xxx", "*"),
"when throw exception ,default return true");
} catch (RuntimeException e) {
Assertions.fail(e.getMessage());
}

} catch (Exception e) {
Assertions.fail(e.getMessage());
Assertions.assertTrue(WebhookUtil.obtainDeliveryAgreement(httpClient2, "xxx", "*"),
"when throw exception ,default return true");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.eventmesh.client.grpc.producer;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.ArgumentMatchers.argThat;
Expand Down Expand Up @@ -93,11 +94,7 @@ public void setUp() throws Exception {

@Test
public void testPublishWithException() {
try {
producer.publish(defaultEventMeshMessageBuilder().content("mockExceptionContent").build());
} catch (Exception e) {
assertThat(e).isNotNull();
}
assertThrows(IllegalArgumentException.class, () -> producer.publish("Not a supported message"));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,24 +95,14 @@ public void testBuildEventMeshCloudEvent() {
io.cloudevents.CloudEvent event =
CloudEventBuilder.v1().withType("eventmesh").withSource(URI.create("/")).withId(id).build();
EventMeshMessage meshMessage = EventMeshMessage.builder().build();
Exception exception = null;
try {
EventMeshCloudEventBuilder.buildEventMeshCloudEvent(event, clientConfig, EventMeshProtocolType.EVENT_MESH_MESSAGE);
} catch (ClassCastException e) {
exception = e;
}
Assertions.assertNotNull(exception);
Assertions.assertThrows(Exception.class,
() -> EventMeshCloudEventBuilder.buildEventMeshCloudEvent(event, clientConfig, EventMeshProtocolType.EVENT_MESH_MESSAGE));
CloudEvent cloudEvent = EventMeshCloudEventBuilder.buildEventMeshCloudEvent(event, clientConfig, EventMeshProtocolType.CLOUD_EVENTS);
Assertions.assertNotNull(cloudEvent);
Assertions.assertEquals("eventmesh", cloudEvent.getType());
Assertions.assertEquals(id, cloudEvent.getId());
Exception exception1 = null;
try {
EventMeshCloudEventBuilder.buildEventMeshCloudEvent(meshMessage, clientConfig, EventMeshProtocolType.CLOUD_EVENTS);
} catch (ClassCastException e) {
exception1 = e;
}
Assertions.assertNotNull(exception1);
Assertions.assertThrows(Exception.class,
() -> EventMeshCloudEventBuilder.buildEventMeshCloudEvent(meshMessage, clientConfig, EventMeshProtocolType.CLOUD_EVENTS));
EventMeshMessage meshMessage1 = EventMeshMessage.builder().uniqueId(id).build();
CloudEvent cloudEvent1 = EventMeshCloudEventBuilder.buildEventMeshCloudEvent(meshMessage1, clientConfig,
EventMeshProtocolType.EVENT_MESH_MESSAGE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,66 +38,44 @@
public class HttpUtilsTest {

@Test
public void testPostPositive() {
public void testPostPositive() throws IOException {
CloseableHttpClient client = mock(CloseableHttpClient.class);
String uri = "http://example.com";
RequestParam requestParam = new RequestParam(HttpMethod.POST);
IOException exception = null;
try {
String expectedResult = "Success";
when(client.execute(any(HttpPost.class), any(ResponseHandler.class))).thenReturn(expectedResult);
String result = HttpUtils.post(client, uri, requestParam);
Assertions.assertEquals(expectedResult, result);
} catch (IOException e) {
exception = e;
}
Assertions.assertNull(exception);
String expectedResult = "Success";
when(client.execute(any(HttpPost.class), any(ResponseHandler.class))).thenReturn(expectedResult);
String result = HttpUtils.post(client, uri, requestParam);
Assertions.assertEquals(expectedResult, result);
}

@Test
public void testPostNegative() {
public void testPostNegative() throws IOException {
CloseableHttpClient client = mock(CloseableHttpClient.class);
String uri = "http://example.com";
RequestParam requestParam = new RequestParam(HttpMethod.GET);
String expectedResult = "Failure";
try {
when(client.execute(any(HttpPost.class), any(ResponseHandler.class))).thenReturn(expectedResult);
String result = HttpUtils.post(client, uri, requestParam);
Assertions.assertNotEquals(expectedResult, result);
} catch (Exception e) {
Assertions.assertNotNull(e);
}
when(client.execute(any(HttpPost.class), any(ResponseHandler.class))).thenReturn(expectedResult);
Assertions.assertThrows(Exception.class, () -> HttpUtils.post(client, uri, requestParam));
}

@Test
public void testGetPositive() {
public void testGetPositive() throws IOException {
CloseableHttpClient client = mock(CloseableHttpClient.class);
String uri = "http://example.com";
RequestParam requestParam = new RequestParam(HttpMethod.GET);
String expectedResult = "Success";
IOException exception = null;
try {
when(client.execute(any(HttpGet.class), any(ResponseHandler.class))).thenReturn(expectedResult);
String result = HttpUtils.get(client, uri, requestParam);
Assertions.assertEquals(expectedResult, result);
} catch (IOException e) {
exception = e;
}
Assertions.assertNull(exception);
when(client.execute(any(HttpGet.class), any(ResponseHandler.class))).thenReturn(expectedResult);
String result = HttpUtils.get(client, uri, requestParam);
Assertions.assertEquals(expectedResult, result);
}

@Test
public void testGetNegative() {
public void testGetNegative() throws IOException {
CloseableHttpClient client = mock(CloseableHttpClient.class);
String uri = "http://example.com";
RequestParam requestParam = new RequestParam(HttpMethod.POST);
String expectedResult = "Failure";
try {
when(client.execute(any(HttpGet.class), any(ResponseHandler.class))).thenReturn(expectedResult);
String result = HttpUtils.get(client, uri, requestParam);
Assertions.assertNotEquals(expectedResult, result);
} catch (Exception e) {
Assertions.assertNotNull(e);
}
when(client.execute(any(HttpGet.class), any(ResponseHandler.class))).thenReturn(expectedResult);
Assertions.assertThrows(Exception.class, () -> HttpUtils.get(client, uri, requestParam));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,7 @@ public void testHello() {
Assertions.assertNotNull(msg.getBody());
Assertions.assertTrue(msg.getBody() instanceof UserAgent);
// Negative Test Case
user = null;
try {
msg = null;
msg = MessageUtils.hello(user);

} catch (Exception e) {
Assertions.assertNull(msg);
}
Assertions.assertDoesNotThrow(() -> MessageUtils.hello(null));
}

@Test
Expand Down Expand Up @@ -102,16 +95,7 @@ public void testSubscribe() {
Assertions.assertNotNull(msg.getBody());
Assertions.assertTrue(msg.getBody() instanceof Subscription);
// Negative Test Case
topic = null;
subscriptionMode = null;
subscriptionType = null;
try {
msg = null;
msg = MessageUtils.subscribe(topic, subscriptionMode, subscriptionType);

} catch (Exception e) {
Assertions.assertNull(msg);
}
Assertions.assertDoesNotThrow(() -> MessageUtils.subscribe(null, null, null));
}

@Test
Expand Down Expand Up @@ -140,13 +124,7 @@ public void testAsyncMessageAck() {
Assertions.assertNotNull(msg.getBody());
Assertions.assertEquals(msg.getBody(), in.getBody());
// Negative Test Case
in = null;
msg = null;
try {
msg = MessageUtils.asyncMessageAck(in);
} catch (Exception e) {
Assertions.assertNull(msg);
}
Assertions.assertThrows(Exception.class, () -> MessageUtils.asyncMessageAck(null));
}

@Test
Expand Down
Loading

0 comments on commit 1a35984

Please sign in to comment.