DC-1055: More Junit 5 migrations #1677

pshapiro4broad · 2024-05-06T13:30:49Z

This PR migrates more unit tests to JUnit 5

migrate all unit tests except for embedded database tests
where possible, tests were changed to not use a spring context
spring context tests were changed to use @ContextConfiguration to limit the number of autowired components

pshapiro4broad · 2024-05-06T14:50:59Z

src/test/java/bio/terra/common/TestUtils.java

@@ -355,4 +359,31 @@ public static <T> T extractValueFromPrivateObject(Object object, String fieldNam
    return objectMapper.convertValue(
        ReflectionTestUtils.getField(object, fieldName), new TypeReference<>() {});
  }
+
+  public static String loadJson(final String resourcePath) {


These methods were ported over from the JsonLoader component, so they could be used by unit tests that don't have a spring context. Using static methods in all cases might be a better approach generally, as there doesn't seem to be a benefit to using a component for these operations.

pshapiro4broad · 2024-05-06T14:53:44Z

src/test/java/bio/terra/service/dataset/AssetModelValidatorTest.java

-  @Autowired private JsonLoader jsonLoader;
+
+  @BeforeEach
+  void beforeEach() {


The validator tests require similar boilerplate, it would be nice if there was a way to share this code.

I often find that stubs in before each methods trigger strict stubbing errors, but that seems to not be a problem here

The difference here isn't that they're @BeforeEach, it's that using the spring boot test annotations doesn't support strict stubbing, so no strictness tests are done by mockito in this test. As far as I can tell, there's no easy way to enable strict stubbing and also use WebMvcTest. See https://github.com/spring-projects/spring-boot/issues/19383. Although I do see a SO response that could work https://stackoverflow.com/a/75388020/947286

pshapiro4broad · 2024-05-06T14:56:28Z

src/test/java/bio/terra/service/dataset/DatasetSchemaUpdateValidatorTest.java

+    })
+@WebMvcTest
+@Tag(Unit.TAG)
+class DatasetSchemaUpdateValidatorTest {


I renamed the test so it matches the class it's testing, DatasetSchemaUpdateValidator

pshapiro4broad · 2024-05-06T14:57:31Z

src/test/java/bio/terra/service/dataset/DatasetServiceUnitTest.java

      values.add(
          new SynapseDataResultModel()
              .filteredCount(filteredRowCount)
              .totalCount(totalRowCount)
              .rowResult(new HashMap<>()));
+    } else {
+      when(azureSynapsePdao.getTableTotalRowCount(any(), any(), any())).thenReturn(totalRowCount);


This change was required due to strict stubbing

pshapiro4broad · 2024-05-06T14:58:49Z

src/test/java/bio/terra/service/dataset/IngestRequestValidatorTest.java

+    })
+@WebMvcTest
+@Tag(Unit.TAG)
+class IngestRequestValidatorTest {


Renamed the test to match its class, IngestRequestValidator

pshapiro4broad · 2024-05-06T15:01:07Z

src/test/java/bio/terra/service/dataset/ValidateAssetUnitTest.java

-          "Validation of asset model should fail on " + itemChecked,
-          ex.getCauses().get(0),
-          containsString(expectedErrorMessage));
-      throw ex;


The tests here used to depend on InvalidAssetException being rethrown. Now that we're using assertThrows(), we no longer have to do this.

pshapiro4broad · 2024-05-06T15:02:02Z

src/test/java/bio/terra/service/dataset/ValidatorTestUtils.java

@@ -12,7 +12,7 @@

 public class ValidatorTestUtils {

-  public static void checkValidationErrorModel(ErrorModel errorModel, String[] messageCodes) {
+  public static void checkValidationErrorModel(ErrorModel errorModel, String... messageCodes) {


This change allows callers to pass in the list of strings directly instead of creating an array to pass in.

pshapiro4broad · 2024-05-06T15:08:21Z

src/test/java/bio/terra/service/filedata/azure/AzureSynapsePdaoUnitTest.java

  private SnapshotRequestAssetModel requestAssetModel;
  private AssetSpecification assetSpec;
  private WalkRelationship walkRelationship;

-  @Autowired AssetUtils assetUtils;
-  @SpyBean AzureSynapsePdao azureSynapsePdaoSpy;


A few tests need to spy() this class so they can intercept calls to executeSynapseQuery(). Instead of spying generally, a spy is created in those specific tests that need it. Adding directly mockable interface layer for query execution might be better approach but would require changes to the class being tested.

pshapiro4broad · 2024-05-06T15:09:09Z

src/test/java/bio/terra/service/filedata/azure/AzureSynapsePdaoUnitTest.java

+                     DATA_SOURCE = 'snapshotDataSourceName1',
+                     FORMAT = 'parquet') AS rows;"""
+            .formatted(IngestUtils.formatSnapshotTableName(snapshotId, PDAO_ROW_ID_TABLE), tableId);
+    assertThat(queryCaptor.getValue(), equalToCompressingWhiteSpace(expected));


Sonar had a warning that this test has no assert, so I addd one that validates the generated query.

pshapiro4broad · 2024-05-06T15:15:03Z

src/test/java/bio/terra/service/filedata/flight/ingest/IngestFilePrimaryDataStepTest.java

    assertThat(
        "Step failed due to InvalidUserProjectException and should be retried",
-        StepStatus.STEP_RESULT_FAILURE_RETRY,
-        equalTo(result.getStepStatus()));
+        result.getStepStatus(),


Many calls here had swapped expected and actual parameters.

sonarqubecloud · 2024-05-06T15:20:22Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

rushtong

Looks reasonable to me, thanks for the cleanup 👍🏽

rushtong · 2024-05-06T16:54:51Z

src/test/java/bio/terra/service/dataset/AssetModelValidatorTest.java

-  @Autowired private JsonLoader jsonLoader;
+
+  @BeforeEach
+  void beforeEach() {


I often find that stubs in before each methods trigger strict stubbing errors, but that seems to not be a problem here

rjohanek · 2024-05-07T13:17:32Z

src/test/java/bio/terra/service/dataset/DatasetSchemaUpdateValidatorTest.java


  @Autowired private MockMvc mvc;

+  @MockBean private JobService jobService;


in general, why do these changes require adding so many mockBean annotations? How were the tests working before without relying on these mocked services and validators before? Or were they just being relied on in a different file?

This was required by the shift from using SpringRunner to ContextConfiguration. Before, the test was creating a spring application context such that all beans that weren't marked as MockBean in the test would be injected with real objects. This meant that the test was creating actual components for things like IamService, DatasetService, etc. This works but it means that a lot of code that's not being tested gets executed, and the test behavior could change due to code not being tested here.

Using ContextConfiguration flips things around so that only the classes declared in its classes are real objects. Every other object must be mocked, or it it will trigger spring failure when it tries to create the context for the test.

Normally this is a good thing in that the test likely needs mocked objects anyway so it can verify or stub services that the test needs. The validator tests are a special case though. In this case, the tests require a fully mocked copy of DatasetsApiController, which ends up pulling in all the components that the controller needs.

Although now that I think about it, maybe this test could instead mock the controller itself, or use the controller's base class (DatasetsApi) instead. 🤔

rjohanek · 2024-05-07T13:36:41Z

src/test/java/bio/terra/service/filedata/flight/ingest/IngestFilePrimaryDataStepTest.java


-  @MockBean private ConfigurationService configService;
+  @Mock private Dataset dataset;


why switch from mockBean to mock? Is this to avoid SpringRunner?

Sort of to avoid, yes. Normally a spring runner test requires @MockBean to both mock an api and provide a bean that replaces the real spring bean (component) from being created and injected. When writing a test that uses a spring context, @MockBean is the only way to replace a bean with a mock at object creation time.

The odd thing here is that the code wasn't using spring injection at all so this wasn't needed. It was already creating the step explicitly using new IngestFilePrimaryDataStep() and passing the mocked objects to the constructor. So this test was creating a spring context but wasn't using Autowired so using MockBean here wasn't having any effect anyway.

rjohanek

Looks good! I just have a couple questions just for my own understanding!

pshapiro4broad added 21 commits May 4, 2024 19:21

junit 5

2bb4870

junit 5

1dd95d1

junit 5

caf9901

junit 5

707dd34

junit 5

444b02c

junit 5

b34e58c

junit 5

23bcd7f

junit 5

86d9f14

junit 5

9ca073a

junit 5

b4e4e46

junit 5

5209ee4

junit 5

4f09141

junit 5

7d952c9

junit 5

610ecb2

junit 5

162c54d

junit 5

a303fd6

junit 5

3bcb40a

junit 5

84ae1cc

junit 5

1d42239

junit 5

1529da4

use configuration to provide a more accurate objectmapper

6f9f646

pshapiro4broad commented May 6, 2024

View reviewed changes

remove comment

4e4adaf

pshapiro4broad commented May 6, 2024

View reviewed changes

typo

e356302

pshapiro4broad commented May 6, 2024

View reviewed changes

pshapiro4broad changed the title ~~More Junit 5 migrations~~ DC-1055: More Junit 5 migrations May 6, 2024

pshapiro4broad marked this pull request as ready for review May 6, 2024 15:18

pshapiro4broad requested review from a team as code owners May 6, 2024 15:18

pshapiro4broad requested review from rushtong and okotsopoulos and removed request for a team May 6, 2024 15:18

rushtong approved these changes May 6, 2024

View reviewed changes

rjohanek reviewed May 7, 2024

View reviewed changes

rjohanek approved these changes May 7, 2024

View reviewed changes

pshapiro4broad merged commit b9ff77b into develop May 7, 2024
13 checks passed

pshapiro4broad deleted the ps/unit-tests branch May 7, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DC-1055: More Junit 5 migrations #1677

DC-1055: More Junit 5 migrations #1677

pshapiro4broad commented May 6, 2024 •

edited

Loading

pshapiro4broad May 6, 2024 •

edited

Loading

pshapiro4broad May 6, 2024

rushtong May 6, 2024

pshapiro4broad May 6, 2024 •

edited

Loading

pshapiro4broad May 6, 2024

pshapiro4broad May 6, 2024

pshapiro4broad May 6, 2024

pshapiro4broad May 6, 2024

pshapiro4broad May 6, 2024

pshapiro4broad May 6, 2024

pshapiro4broad May 6, 2024

pshapiro4broad May 6, 2024

sonarqubecloud bot commented May 6, 2024

rushtong left a comment

rushtong May 6, 2024

rjohanek May 7, 2024

pshapiro4broad May 7, 2024

rjohanek May 7, 2024

pshapiro4broad May 7, 2024 •

edited

Loading

rjohanek left a comment


		@Autowired private MockMvc mvc;

		@MockBean private JobService jobService;


		@MockBean private ConfigurationService configService;
		@Mock private Dataset dataset;

DC-1055: More Junit 5 migrations #1677

DC-1055: More Junit 5 migrations #1677

Conversation

pshapiro4broad commented May 6, 2024 • edited Loading

pshapiro4broad May 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

pshapiro4broad May 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 6, 2024

Quality Gate passed

rushtong 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

pshapiro4broad May 7, 2024 • edited Loading

Choose a reason for hiding this comment

rjohanek left a comment

Choose a reason for hiding this comment

pshapiro4broad commented May 6, 2024 •

edited

Loading

pshapiro4broad May 6, 2024 •

edited

Loading

pshapiro4broad May 6, 2024 •

edited

Loading

pshapiro4broad May 7, 2024 •

edited

Loading