kfaraz commented on code in PR #16420:
URL: https://github.com/apache/druid/pull/16420#discussion_r1596228368
##########
server/src/test/java/org/apache/druid/server/lookup/cache/LookupLoadingSpecTest.java:
##########
@@ -59,4 +66,67 @@ public void testLoadingOnlyRequiredLookupsWithNullList()
DruidException exception = Assert.assertThrows(DruidException.class, () ->
LookupLoadingSpec.loadOnly(null));
Assert.assertEquals("Expected non-null set of lookups to load.",
exception.getMessage());
}
+
+ @MethodSource("provideParamsForTestCreateFromContext")
+ @ParameterizedTest
+ public void testGetLookupLoadingSpecFromContext(Map<String, Object> context,
LookupLoadingSpec defaultSpec, LookupLoadingSpec expectedSpec)
Review Comment:
This test doesn't really seem like a good candidate for parameterization.
Druid tests typically tend to use parameterization when the parameters
themselves do not play a role in the assertions. Rather, they tweak some
external factor and the assertions must work despite of that factor. For
example, using different input formats (csv/json) when verifying that Druid is
able to ingest a sample dataset successfully.
In some cases, it might be okay to include the expected value in the
parameters but doing that here severely compromises the readability of the test.
Simply put, we should not have to look at the parameter provider method to
figure out what the test is even verifying.
---
You could break up this method as appropriate, and each assertion would
typically look like:
```java
Assert.assertEquals(
LookupLoadingSpec.NONE,
LookupLoadingSpec.createFromContext(
ImmutableMap.of(LookupLoadingSpec.KEY_1, value1,
LookupLoadingSpec.KEY_2, value2),
LookupLoadingSpec.NONE
)
);
```
This makes reading through the test much easier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
