Maintenance/use more junit5 features #518

jens-kaiser · 2024-11-26T12:51:17Z

Many Junit 5 features are not used in existing tests because the structures from JUnit 4 are still being implemented.
Mocks and other values e.g. can be injected directly into the tests without using the Mockito.mock method or class attributes.

… another tests the existence of the pojo.

codecov · 2024-11-28T17:44:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.16%. Comparing base (7904764) to head (8b81231).
Report is 67 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #518      +/-   ##
============================================
+ Coverage     82.11%   82.16%   +0.04%     
+ Complexity      478      390      -88     
============================================
  Files            86       85       -1     
  Lines          1068      953     -115     
  Branches         70       68       -2     
============================================
- Hits            877      783      -94     
+ Misses          156      144      -12     
+ Partials         35       26       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

...c/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeRestEndpointIntegrationTest.java

...ng-boot/src/test/java/de/codecentric/spring/boot/demo/chaos/monkey/ChaosDemoApplication.java

...spring/boot/chaos/monkey/watcher/outgoing/ChaosMonkeyRestTemplateWatcherIntegrationTest.java

WtfJoke

I finally found the time to check it out in more detail. I've left some comments.
The changes overall looks good, thanks for that 👍

There aren't really blocking points in my opinion, despite the @Disabled test and the comment/review from @denniseffing.

After that feel free to (squash :D) merge (or address some of the further comments) :)

...est/java/de/codecentric/spring/boot/chaos/monkey/ChaosDemoApplicationDefaultProfileTest.java

WtfJoke · 2024-12-06T12:58:21Z

...p-naked/src/test/java/com/example/chaos/monkey/chaosdemo/controller/HelloControllerTest.java

@@ -48,6 +50,9 @@ public void setup() {
        when(greetingServiceMock.greet()).thenReturn(responseService);
        responseRepo = "Hello from repo!";
        when(greetingServiceMock.greetFromRepo()).thenReturn(responseRepo);
+        when(greetingServiceMock.greetFromRepoPagingSorting()).thenReturn(responseRepo);
+        when(greetingServiceMock.greetFromRepoJpa()).thenReturn(responseRepo);
+        when(greetingServiceMock.greetFromRepoAnnotation()).thenReturn(responseRepo);


We could also move this to the parametrized test below, because its only used there. However no hard feelings in keeping it here :D

WtfJoke · 2024-12-06T13:01:10Z

...spring/boot/chaos/monkey/watcher/outgoing/ChaosMonkeyRestTemplateWatcherIntegrationTest.java

    @SpringBootTest(properties = {"chaos.monkey.enabled=true", "chaos.monkey.watcher.rest-template=true", "chaos.monkey.assaults.latency-active=true",
            "chaos.monkey.test.rest-template.time-out=20"}, classes = {ChaosDemoApplication.class})
    @ActiveProfiles("chaos-monkey")
-    static class LatencyAssaultIntegrationTest {
+    @Nested
+    @Disabled


I guess that was added by accident? However it seems that the Exception has changed from SSLException to io.netty.handler.timeout.ReadTimeoutException

EDIT: This seems to work:

assertThatThrownBy(() -> demoRestTemplateService.callWithRestTemplate()).isInstanceOf(ReadTimeoutException.class);

WtfJoke · 2024-12-06T13:01:59Z

...pring/boot/chaos/monkey/watcher/advice/advisor/ChaosMonkeyAnnotationPointcutAdvisorTest.java

+    @Mock
+    private Advice advice;
+    @Mock
+    private ChaosMonkeyBaseClassFilter baseClassFilter;


Good changes 👍

WtfJoke · 2024-12-06T13:18:09Z

...-toggles/src/test/java/com/example/chaos/monkey/toggledemo/component/HelloComponentTest.java

+
+import static org.junit.jupiter.api.Assertions.*;
+
+class HelloComponentTest {


Which value adds this test? 😅 if its only to get codecov happy I would simply drop it

But if codecov drops down, you have to merge it anyway

yeah codecov cant stop us 👿 😄 we merge it!

if you fix that, im happy to merge it (or you can on your own if you like) :D

WtfJoke · 2024-12-06T13:22:29Z

...emo-app/src/test/java/com/example/chaos/monkey/chaosdemo/controller/HelloControllerTest.java

@@ -35,7 +37,7 @@ public class HelloControllerTest {
    @Autowired
    private MockMvc mockMvc;

-    @MockBean
+    @MockitoBean


Can you please do this change for the remaining @MockBean as well if possible? 🙃🙏

WtfJoke · 2024-12-06T13:23:30Z

.../src/test/java/de/codecentric/spring/boot/chaos/monkey/assaults/LatencyAssaultRangeTest.java

-        final int latencyRangeStart = 1000;
-        final int latencyRangeEnd = 5000;
+    @Mock
+    ApplicationEventPublisher applicationEventPublisher;


Personal taste: I would add private to every member variable here

WtfJoke · 2024-12-06T13:32:30Z

.../codecentric/spring/boot/chaos/monkey/configuration/ChaosMonkeyRequestScopeSettingsTest.java


-        assertThat(settings.getChaosMonkeyProperties()).isNotNull();


Not really sure, why this test existed before, however dont we need these notnull checks still in a test (allArgsTest?)?

WtfJoke · 2024-12-06T13:44:16Z

...test/java/de/codecentric/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRestEndpointTest.java

-        BDDAssertions.then(watcherProperties.isRestTemplate()).isTrue();
-        BDDAssertions.then(watcherProperties.isWebClient()).isTrue();
-        BDDAssertions.then(watcherProperties.isActuatorHealth()).isTrue();
+        assertAll(() -> assertEquals(HttpStatus.OK, response.getStatusCode()), () -> assertTrue(watcherProperties.isController()),


… test class

WtfJoke

LGTM 🐼

jens-kaiser added 10 commits November 26, 2024 13:05

clean up code

02c1497

use @mock annotation

f7705c4

replace times(0) by never and times(1) by default verify call

2b21a64

use parameter injection

55d0791

test on configuration properties

53ad10a

clean up code

c0e5afd

it makes no sense to test in one test the attributes of a pojo and in…

9ce386b

… another tests the existence of the pojo.

brain overload, BDDAssertions is only used in a single test

bf71f68

user parameter injection

6198877

don't mock POJOs

80bb3d6

jens-kaiser changed the title ~~Maintenance/user more junit5 features~~ Maintenance/use more junit5 features Nov 26, 2024

jens-kaiser added 3 commits November 26, 2024 15:58

remove useless tests

ee2ea13

apply spotless rules

8fc2d70

deactivate the unused test instead of blocking it incorrectly

8574068

jens-kaiser requested a review from F43nd1r November 28, 2024 13:53

apply spotless rules

833242d

F43nd1r reviewed Nov 29, 2024

View reviewed changes

...c/spring/boot/chaos/monkey/endpoints/ChaosMonkeyRequestScopeRestEndpointIntegrationTest.java Outdated Show resolved Hide resolved

jens-kaiser added 7 commits November 30, 2024 12:41

use @SPY instead of @mock to test AssaultProperties too.

0909bed

don't mix assertNotNull with assertThat

2d33998

mock the ApplicationEventPublisher and not the MetricEventPublisher

dacd15f

add some tests

c82fa2d

apply spotless

44219e8

add unit tests

8c02d4e

add unit tests

d27a845

denniseffing self-requested a review December 6, 2024 08:48

denniseffing reviewed Dec 6, 2024

View reviewed changes

...ng-boot/src/test/java/de/codecentric/spring/boot/demo/chaos/monkey/ChaosDemoApplication.java Outdated Show resolved Hide resolved

...spring/boot/chaos/monkey/watcher/outgoing/ChaosMonkeyRestTemplateWatcherIntegrationTest.java Show resolved Hide resolved

WtfJoke reviewed Dec 6, 2024

View reviewed changes

remove internal ConfigurationBean from test Application, remove empty…

945cd9f

… test class

jens-kaiser self-assigned this Dec 6, 2024

jens-kaiser enabled auto-merge (squash) December 6, 2024 14:35

jens-kaiser removed their assignment Dec 6, 2024

jens-kaiser requested a review from F43nd1r December 6, 2024 14:36

fix unit test

8b81231

WtfJoke approved these changes Dec 6, 2024

View reviewed changes

jens-kaiser merged commit d25b0e3 into codecentric:main Dec 6, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintenance/use more junit5 features #518

Maintenance/use more junit5 features #518

jens-kaiser commented Nov 26, 2024

codecov bot commented Nov 28, 2024 •

edited

Loading

WtfJoke left a comment

WtfJoke Dec 6, 2024

WtfJoke Dec 6, 2024 •

edited

Loading

WtfJoke Dec 6, 2024

WtfJoke Dec 6, 2024

jens-kaiser Dec 6, 2024

WtfJoke Dec 6, 2024

jens-kaiser Dec 6, 2024

WtfJoke Dec 6, 2024 •

edited

Loading

WtfJoke Dec 6, 2024

WtfJoke Dec 6, 2024

WtfJoke Dec 6, 2024 •

edited

Loading

WtfJoke Dec 6, 2024

WtfJoke left a comment •

edited

Loading


		import static org.junit.jupiter.api.Assertions.*;

		class HelloComponentTest {


		assertThat(settings.getChaosMonkeyProperties()).isNotNull();

Maintenance/use more junit5 features #518

Maintenance/use more junit5 features #518

Conversation

jens-kaiser commented Nov 26, 2024

codecov bot commented Nov 28, 2024 • edited Loading

Codecov Report

WtfJoke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WtfJoke left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2024 •

edited

Loading

WtfJoke Dec 6, 2024 •

edited

Loading

WtfJoke Dec 6, 2024 •

edited

Loading

WtfJoke Dec 6, 2024 •

edited

Loading

WtfJoke left a comment •

edited

Loading