CARDS-2154: cards-utils tests by sofi2002sofi · Pull Request #1418 · data-team-uhn/cards

sofi2002sofi · 2023-03-10T15:12:32Z

No description provided.

sdumitriu · 2023-03-13T21:22:48Z

+ * @version $Id $
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class ResourceToCSVAdapterFactoryTest


More tests:

getAdapterWithNoProcessorsReturnsResourcePath - has an empty list of processors

getAdapterUsesFirstProcessorThatCanProcess - has 3 processors, first cannot process, check second's output is used, verify first's and third's serialize is never invoked

sdumitriu · 2023-03-13T21:23:32Z

+    }
+
+    @Test
+    public void getAdapterForResourceAdaptableObjectReturnsResourcePath()


Suggested change

public void getAdapterForResourceAdaptableObjectReturnsResourcePath()

public void getAdapterForUnsupportedResourceReturnsResourcePath()

sdumitriu · 2023-03-13T21:26:49Z

+        when(processor.isEnabledByDefault(adaptable)).thenReturn(true);
+        when(processor.getName()).thenReturn("name");
+        when(processor.canProcess(adaptable)).thenReturn(true);


These lines are the same in all 3 tests, they should be moved in a mockWorkingProcessor method.

sdumitriu · 2023-03-13T21:34:16Z

+        ResourceMetadata resourceMetadata = mock(ResourceMetadata.class);
+        when(adaptable.getResourceMetadata()).thenReturn(resourceMetadata);


This code seems useless to me; the fact that it is actually needed here suggests that the actual code can be improved. Try moving the adaptTo(Node.class) line earlier in the method, and add a check for null.

Normally, in test-driven development, the tests are supposed to influence the code, not the other way around, so you should feel free to also change the code being tested when you feel that it can be improved, not just write tests that perfectly match the existing code.

sdumitriu · 2023-03-13T21:55:22Z

+ * @version $Id $
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class ResourceToJsonAdapterFactoryTest


More tests:

getAdapterUsesAllSupportedAndEnabledProcessors - receives a list of processors that either support or not the resource, and that are either enabled or disabled by default

getAdapterSortsProcessors - receives a list of processors in the wrong order; to make the order validation easier to check, write a simple class that receives a priority and 2 integers a and b in its constructor, and implements ResourceJsonProcessor.processProperty as return Json.createValue(input == null ? b : ((JsonNumber) input).intValue() * a + b);; instantiate 4 such processors with different prime values for a and b, and serialize a number; make sure the output matches the expected result

getAdapterWithNoProcessorsReturnsEmptyJsonObject - uses an empty list of processors

getAdapterWithNoSupportedProcessorsReturnsEmptyJsonObject - uses one processor that cannot process the resource

getAdapterWithRecursiveReferencesUsesResourcePathForNestedReferences

getAdapterUsesResourceSelectorsToDisableDefaultProcessors

getAdapterUsesResourceSelectorsToEnableProcessors

getAdapterWithBothEnableAndDisableSelectorsPrioritizesEnable

sdumitriu · 2023-03-15T14:10:30Z

+    }
+
+    @Test
+    public void isEnabledByDefaultTest()


Oh, I think each processor needs this test, even if the method is not overridden in a class, just to check that it is indeed false.

sdumitriu · 2023-03-15T14:26:16Z

+        Property property = node.getProperty("jcr:baseVersion");
+        JsonValue json = Json.createValue(FORM_TYPE);
+        JsonValue jsonValue = this.dereferenceProcessor.processProperty(node, property, json, this::serializeNode);
+        assertNotNull(jsonValue);


This needs an assertEquals check as well.

assertEquals(property.getNode().getPath(), ((JsonString) jsonValue).getString());

sdumitriu · 2023-03-15T15:05:29Z

+        assertTrue(jsonObject.containsKey("@path"));
+        assertEquals(Json.createValue(TEST_FORM_PATH), jsonObject.getJsonString("@path"));
+        assertTrue(jsonObject.containsKey("@name"));
+        assertEquals(Json.createValue("f1"), jsonObject.getJsonString("@name"));


Why not the simpler:

assertEquals("f1", jsonObject.getString("@name"));

sdumitriu · 2023-03-15T15:07:53Z

+    }
+
+    @Test
+    public void leaveAddsPathAndNameAndReferencedParameters() throws RepositoryException


I would split this into 2 methods:

leaveAddsPathAndNameParameters

leaveAddsReferencedParameter

Also, I would test the reference for 2 different nodes, the form (for false) and the questionnaire (for true).

sdumitriu · 2023-03-15T15:11:50Z

+ * @version $Id $
+ */
+@RunWith(MockitoJUnitRunner.class)
+public class SimpleProcessorTest


This is missing a positive test: processPropertyReturnsInput.

veronikaslc · 2023-06-16T21:49:24Z

ResourceToJsonAdapterFactoryTest: Lines 267-208 not covered from ResourceToJsonAdapterFactory
BareProcessorTest not covered cases of lines 141 and 206-209 of BareProcessor
DereferenceProcessorTest: not covered line 127 of DereferenceProcessor
SimpleProcessorTest: not covered line 101 of SimpleProcessor

veronikaslc · 2023-06-16T21:50:10Z

jacoco.zip
jacoco:report html file attached

Modernize the revived tests and complete the module coverage to 100%: - migrate the tests to the current platform APIs: jakarta.json, SlingJakartaHttpServletRequest/Response, the jakartaResponse binding, Mockito 5, sling-mock 4 - drop the Mockito runner in favor of plain mocks with reflection injection, since concurrently running runners collide on Mockito's global listener registry under the parallel surefire configuration - raise the sling-mock resource resolver factory timeout, the default 500ms is flaky when several Oak-backed contexts start in parallel - add tests for the previously uncovered classes: SelectorServlet, DateUtils, SelectorDetails, BaseFilterFactory, DataFilter and the serialization SPI default methods, DefaultDataFilters(Parser), DefaultOptionsProcessor, ExcludeDefaultPropertiesProcessor, ImportableProcessor, ReferencedProcessor, PreventVersionOverrideServletFilter - fix a latent NPE: SelectorDetails built through the vararg options constructor with fewer than two option strings left the options array null, crashing SelectorServlet's getOptions().length - remove an unreachable branch in DateUtils.parseDateTime, parseBest either returns one of the two queried types or throws - remove the module's coverage exemption, restoring the default 1.00 required instruction coverage ratio

+    }
+
+    @Test
+    public void getAdapterUsesFirstProcessorThatCanProcess() throws IllegalAccessException


+    }
+
+    @Test
+    public void getAdapterForResourceAdaptableObjectReturnsSerializedAdapter() throws IllegalAccessException


+    }
+
+    @Test
+    public void getAdapterUsesFirstProcessorThatCanProcess() throws IllegalAccessException


+    }
+
+    @Test
+    public void getAdapterForResourceAdaptableObjectReturnsSerializedAdapter() throws IllegalAccessException


marta- added the test-coverage label Mar 14, 2023

sdumitriu requested changes Mar 15, 2023

View reviewed changes

sofi2002sofi requested a review from sdumitriu March 22, 2023 16:14

sofi2002sofi force-pushed the CARDS-2154-utils-test branch from 3bb94da to fa83ebe Compare July 5, 2023 07:47

marta- assigned veronikaslc Nov 13, 2025

veronikaslc force-pushed the CARDS-2154-utils-test branch from fa83ebe to d08ad09 Compare December 5, 2025 05:45

veronikaslc marked this pull request as ready for review December 5, 2025 05:48

veronikaslc force-pushed the CARDS-2154-utils-test branch from d08ad09 to 4c9df54 Compare December 6, 2025 06:36

sofi2002sofi and others added 2 commits July 27, 2026 19:06

CARDS-2154: Increase test coverage of cards-utils

cd965fb

sdumitriu force-pushed the CARDS-2154-utils-test branch from 4c9df54 to b09a627 Compare July 27, 2026 19:40

github-code-quality Bot found potential problems Jul 27, 2026

View reviewed changes

Navigation Menu

Uh oh!

CARDS-2154: cards-utils tests - #1418

CARDS-2154: cards-utils tests#1418
sofi2002sofi wants to merge 2 commits into
devfrom
CARDS-2154-utils-test

sofi2002sofi commented Mar 10, 2023

Uh oh!

sdumitriu Mar 13, 2023

Uh oh!

sdumitriu Mar 13, 2023

Uh oh!

sdumitriu Mar 13, 2023

Uh oh!

sdumitriu Mar 13, 2023

Uh oh!

sdumitriu Mar 13, 2023

Uh oh!

sdumitriu Mar 15, 2023

Uh oh!

sdumitriu Mar 15, 2023

Uh oh!

sdumitriu Mar 15, 2023

Uh oh!

sdumitriu Mar 15, 2023

Uh oh!

sdumitriu Mar 15, 2023

Uh oh!

veronikaslc commented Jun 16, 2023

Uh oh!

veronikaslc commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	public void getAdapterForResourceAdaptableObjectReturnsResourcePath()
	public void getAdapterForUnsupportedResourceReturnsResourcePath()

		ResourceMetadata resourceMetadata = mock(ResourceMetadata.class);
		when(adaptable.getResourceMetadata()).thenReturn(resourceMetadata);

Uh oh!

Conversation

sofi2002sofi commented Mar 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

veronikaslc commented Jun 16, 2023

Uh oh!

veronikaslc commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants