Added Junit Tests #3008

sonja-ct · 2020-07-02T10:11:06Z

Added Junit tests for EventStoreDAO class
Added Junit tests for EventStoreFactoryImpl class
Added Junit tests for EventStoreRecordImpl class
Added Junit tests for EventStoreServiceImpl class

Signed-off-by: Sonja sonja.matic@comtrade.com

Related Issue
/

Description of the solution adopted
/

Screenshots
/

Any side note on the changes made
/

codecov · 2020-07-02T11:09:13Z

Codecov Report

Merging #3008 into develop will increase coverage by 0.41%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             develop    #3008      +/-   ##
=============================================
+ Coverage      57.86%   58.28%   +0.41%     
- Complexity      2596     2598       +2     
=============================================
  Files           1070     1070              
  Lines          22863    22863              
  Branches        2023     2023              
=============================================
+ Hits           13230    13326      +96     
+ Misses          8693     8603      -90     
+ Partials         940      934       -6

Impacted Files	Coverage Δ	Complexity Δ
.../DeviceManagementOperationRegistryServiceImpl.java	`69.64% <0.00%> (ø)`	`14.00% <0.00%> (ø%)`
...nnection/internal/DeviceConnectionServiceImpl.java	`69.66% <0.00%> (+2.24%)`	`17.00% <0.00%> (+1.00%)`
...pse/kapua/commons/service/internal/ServiceDAO.java	`74.21% <0.00%> (+3.45%)`	`0.00% <0.00%> (ø%)`
...lugin/authentication/AdminAuthenticationLogic.java	`76.92% <0.00%> (+7.69%)`	`0.00% <0.00%> (ø%)`
...pua/broker/core/listener/ErrorMessageListener.java	`70.83% <0.00%> (+8.33%)`	`0.00% <0.00%> (ø%)`
...rtz/persistence/KapuaQuartzConnectionProvider.java	`100.00% <0.00%> (+12.50%)`	`5.00% <0.00%> (+1.00%)`
...ce/event/store/internal/EventStoreServiceImpl.java	`17.64% <0.00%> (+17.64%)`	`0.00% <0.00%> (ø%)`
...ns/service/event/store/internal/EventStoreDAO.java	`77.77% <0.00%> (+77.77%)`	`0.00% <0.00%> (ø%)`
...ce/event/store/internal/EventStoreFactoryImpl.java	`100.00% <0.00%> (+87.50%)`	`0.00% <0.00%> (ø%)`
...ice/event/store/internal/EventStoreRecordImpl.java	`100.00% <0.00%> (+94.44%)`	`0.00% <0.00%> (ø%)`
... and 3 more

LeoNerdoG

Hey @Coduz,
I have checked this PR and it looks OK - can you please recheck and merge when possible?
Tests have passed and codeCov has no complaints.

Coduz

Hi @sonja-ct ,

I've made some comments. I've commented only first occurrence of many things, but I would like to have that fixed on all places (i.e: all places where:

EventStoreRecordImpl eventStoreRecordImpl = new EventStoreRecordImpl(scopeId);
assertNotNull("Null not expected.", eventStoreRecordImpl);

is done.

There is also a question on why EntityManager has been mocked.

Thanks,

Alberto

Coduz · 2020-07-08T14:47:53Z

...st/java/org/eclipse/kapua/commons/service/event/store/internal/EventStoreRecordImplTest.java

+
+        for (KapuaId scopeId : scopeIdList) {
+            EventStoreRecordImpl eventStoreRecordImpl = new EventStoreRecordImpl(scopeId);
+            assertNotNull("Null not expected.", eventStoreRecordImpl);


This test is always true.
Is not possible to have a null object after a new, and if new EventStoreRecordImpl(scopeId) goes on exception, this assertNotNull is not executed.

@sonja-ct I suggest that you check here if the created object is of right instance (for example we are expecting "X" instance and if in future this changes so the method returns instance of an object "Y" we will have a test that will check this.
assertNotNull is generally used only when you ahve two outcomes, Null and some not null value. It can also be a part of some assertion list (several assertions that check various things). I agree with @Coduz on this one.

But a result of a new is always a notNull object, unless Exception, which is not covered anyway by this assertNotNull. This is the same thing as writing assertTrue(true).
If new returns a null, well Java must be a bit stoned or something 😂

I wouldn't also mind to check for the right instance type.
Doing:

EventStoreRecordImpl eventStoreRecordImpl = new EventStoreRecordImpl(scopeId); assertTrue(eventStoreRecordImpl instence of EventStoreRecordImpl)

is useless for the same reasons.
Also this:

EventStoreRecordImpl eventStoreRecordImpl = new EventStoreRecordImpl(scopeId); assertTrue(eventStoreRecordImpl instence of EventStoreRecord)

is useless. In both cases, if that isn't true ClassCastException would be thrown when assigning the new result to the local variable.

Coduz · 2020-07-08T15:02:54Z

...st/java/org/eclipse/kapua/commons/service/event/store/internal/EventStoreRecordImplTest.java

+        String contextId = "ContextId";
+        KapuaId[] scopeIdList = {null, new KapuaIdStatic(BigInteger.ONE), new KapuaIdStatic(BigInteger.TEN), new KapuaIdStatic(BigInteger.ZERO)};
+
+        for (KapuaId scopeId : scopeIdList) {
+            EventStoreRecordImpl eventStoreRecordImpl1 = new EventStoreRecordImpl(scopeId);
+            assertNull("Null expected.", eventStoreRecordImpl1.getContextId());
+            eventStoreRecordImpl1.setContextId(contextId);
+            assertEquals("Expected and actual values should be the same.", contextId, eventStoreRecordImpl1.getContextId());
+            eventStoreRecordImpl1.setContextId(null);
+            assertNull("Null expected.", eventStoreRecordImpl1.getContextId());


I think that testing for different values of scopeId is not meaningful.
We want to test only once test and get.

I agree. We can test only with "null" and BigInteger.ONE for example.

Coduz · 2020-07-08T15:03:56Z

...st/java/org/eclipse/kapua/commons/service/event/store/internal/EventStoreRecordImplTest.java

+        EventStoreRecordImpl eventStoreRecordImpl3 = new EventStoreRecordImpl(eventStoreRecord);
+        assertEquals("Expected and actual values should be the same.", "contextId", eventStoreRecordImpl3.getContextId());
+        eventStoreRecordImpl3.setContextId(contextId);
+        assertEquals("Expected and actual values should be the same.", contextId, eventStoreRecordImpl3.getContextId());
+        eventStoreRecordImpl3.setContextId(null);
+        assertNull("Null expected.", eventStoreRecordImpl3.getContextId());


The clone method has been already been tested in eventStoreRecordImplTest3, if I'm correct.
So also this is not needed.

@Coduz here we are checking if the contextId is null and in the "eventStoreRecordImplTest3" we check if the expected and actual values are the same, so these are two different cases. If I am seeing right.

Coduz · 2020-07-08T15:06:59Z

qa/integration/src/test/java/org/eclipse/kapua/integration/misc/EventStoreDAOTest.java

+
+    @Test
+    public void eventStoreDAOTest() throws Exception {
+        Constructor<EventStoreDAO> eventStoreDAO = EventStoreDAO.class.getDeclaredConstructor();


EventStoreDAO and other EntityDAO are static and testing the instantiation is not needed.

Coduz · 2020-07-08T15:07:28Z

qa/integration/src/test/java/org/eclipse/kapua/integration/misc/EventStoreDAOTest.java

+import java.util.List;
+
+@Category(JUnitTests.class)
+public class EventStoreDAOTest extends Assert {


Why mock DAO methods when there is an actual implementation available and working?

@Coduz we should always mock as much methods as possible to isolate unit test as much as possible.
We don't want actual implementation of other methods to imterfere with our Unit tests.

LeoNerdoG

Hey @Coduz I have finished my review.
Please look at the comments, if you have something to add, please let us know. Otherwise @sonja-ct you can start fixing the tests or rather the comments that have not been addressed yet.

LeoNerdoG · 2020-07-10T05:57:19Z

...st/java/org/eclipse/kapua/commons/service/event/store/internal/EventStoreRecordImplTest.java

+
+        for (KapuaId scopeId : scopeIdList) {
+            EventStoreRecordImpl eventStoreRecordImpl = new EventStoreRecordImpl(scopeId);
+            assertNotNull("Null not expected.", eventStoreRecordImpl);


@sonja-ct I suggest that you check here if the created object is of right instance (for example we are expecting "X" instance and if in future this changes so the method returns instance of an object "Y" we will have a test that will check this.
assertNotNull is generally used only when you ahve two outcomes, Null and some not null value. It can also be a part of some assertion list (several assertions that check various things). I agree with @Coduz on this one.

LeoNerdoG · 2020-07-10T05:57:52Z

...st/java/org/eclipse/kapua/commons/service/event/store/internal/EventStoreRecordImplTest.java

+        String contextId = "ContextId";
+        KapuaId[] scopeIdList = {null, new KapuaIdStatic(BigInteger.ONE), new KapuaIdStatic(BigInteger.TEN), new KapuaIdStatic(BigInteger.ZERO)};
+
+        for (KapuaId scopeId : scopeIdList) {
+            EventStoreRecordImpl eventStoreRecordImpl1 = new EventStoreRecordImpl(scopeId);
+            assertNull("Null expected.", eventStoreRecordImpl1.getContextId());
+            eventStoreRecordImpl1.setContextId(contextId);
+            assertEquals("Expected and actual values should be the same.", contextId, eventStoreRecordImpl1.getContextId());
+            eventStoreRecordImpl1.setContextId(null);
+            assertNull("Null expected.", eventStoreRecordImpl1.getContextId());


I agree. We can test only with "null" and BigInteger.ONE for example.

LeoNerdoG · 2020-07-10T06:00:04Z

qa/integration/src/test/java/org/eclipse/kapua/integration/misc/EventStoreDAOTest.java

+import java.util.List;
+
+@Category(JUnitTests.class)
+public class EventStoreDAOTest extends Assert {


@Coduz we should always mock as much methods as possible to isolate unit test as much as possible.
We don't want actual implementation of other methods to imterfere with our Unit tests.

LeoNerdoG · 2020-07-10T06:01:49Z

...st/java/org/eclipse/kapua/commons/service/event/store/internal/EventStoreRecordImplTest.java

+        EventStoreRecordImpl eventStoreRecordImpl3 = new EventStoreRecordImpl(eventStoreRecord);
+        assertEquals("Expected and actual values should be the same.", "contextId", eventStoreRecordImpl3.getContextId());
+        eventStoreRecordImpl3.setContextId(contextId);
+        assertEquals("Expected and actual values should be the same.", contextId, eventStoreRecordImpl3.getContextId());
+        eventStoreRecordImpl3.setContextId(null);
+        assertNull("Null expected.", eventStoreRecordImpl3.getContextId());


@Coduz here we are checking if the contextId is null and in the "eventStoreRecordImplTest3" we check if the expected and actual values are the same, so these are two different cases. If I am seeing right.

sonja-ct · 2020-07-29T10:19:42Z

Hey @Coduz,
I made all the requested changes, if you can take a look, I hope everything is ok now.
Thanks, Sonja

Added Junit tests for EventStoreDAO class Added Junit tests for EventStoreFactoryImpl class Added Junit tests for EventStoreRecordImpl class Added Junit tests for EventStoreServiceImpl class Signed-off-by: Sonja <sonja.matic@comtrade.com>

LeoNerdoG approved these changes Jul 2, 2020

View reviewed changes

Coduz requested changes Jul 8, 2020

View reviewed changes

LeoNerdoG suggested changes Jul 10, 2020

View reviewed changes

This was referenced Jul 24, 2020

Added Junit Tests for LocalCache Class #3014

Merged

Added Junit Tests #3015

Merged

sonja-ct force-pushed the tests2 branch 2 times, most recently from 3c80046 to 30e5884 Compare July 29, 2020 08:00

Added Junit Tests

6f2597e

Added Junit tests for EventStoreDAO class Added Junit tests for EventStoreFactoryImpl class Added Junit tests for EventStoreRecordImpl class Added Junit tests for EventStoreServiceImpl class Signed-off-by: Sonja <sonja.matic@comtrade.com>

sonja-ct force-pushed the tests2 branch from 30e5884 to 6f2597e Compare September 2, 2020 06:24

Coduz approved these changes Sep 2, 2020

View reviewed changes

Coduz merged commit d328efb into eclipse:develop Sep 2, 2020

sonja-ct deleted the tests2 branch September 3, 2020 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Junit Tests #3008

Added Junit Tests #3008

sonja-ct commented Jul 2, 2020 •

edited

Loading

codecov bot commented Jul 2, 2020 •

edited

Loading

LeoNerdoG left a comment

Coduz left a comment

Coduz Jul 8, 2020

LeoNerdoG Jul 10, 2020

Coduz Jul 24, 2020

Coduz Jul 24, 2020 •

edited

Loading

Coduz Jul 8, 2020

LeoNerdoG Jul 10, 2020

Coduz Jul 24, 2020

Coduz Jul 8, 2020

LeoNerdoG Jul 10, 2020

Coduz Jul 8, 2020

Coduz Jul 8, 2020

LeoNerdoG Jul 10, 2020

LeoNerdoG left a comment

LeoNerdoG Jul 10, 2020

LeoNerdoG Jul 10, 2020

LeoNerdoG Jul 10, 2020

LeoNerdoG Jul 10, 2020

sonja-ct commented Jul 29, 2020

Added Junit Tests #3008

Added Junit Tests #3008

Conversation

sonja-ct commented Jul 2, 2020 • edited Loading

codecov bot commented Jul 2, 2020 • edited Loading

Codecov Report

LeoNerdoG left a comment

Choose a reason for hiding this comment

Coduz left a 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

Coduz Jul 24, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeoNerdoG left a 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

sonja-ct commented Jul 29, 2020

sonja-ct commented Jul 2, 2020 •

edited

Loading

codecov bot commented Jul 2, 2020 •

edited

Loading

Coduz Jul 24, 2020 •

edited

Loading