Code Coverage additions for Extensions #17596
Conversation
github-actions
bot
added
Area - Batch Ingestion
Area - Metrics/Event Emitting
Area - Dependencies
Area - MSQ
...re/simple-client-sslcontext/src/test/java/org/apache/druid/https/SSLContextProviderTest.java
Outdated
Show resolved
Hide resolved
...-core/simple-client-sslcontext/src/test/java/org/apache/druid/https/SSLClientConfigTest.java
Outdated
Show resolved
Hide resolved
...-core/multi-stage-query/src/test/java/org/apache/druid/msq/indexing/MSQTuningConfigTest.java
Outdated
Show resolved
Hide resolved
...ions-core/multi-stage-query/src/test/java/org/apache/druid/msq/indexing/MSQTaskListTest.java
Outdated
Show resolved
Hide resolved
ashwintumma23
force-pushed
the
codeCoverageExtensions
branch
from
af7daca to
84435fb
Compare
There was a problem hiding this comment.
Thanks for contributing to Druid, @ashwintumma23 !
I have left some suggestions. Let me know what you think.
As a general principle, while verifying getters, equals and hashCode of POJOs does help with code coverage, it doesn't always add a lot of value by itself. Moreover, these code paths tend to get covered anyway through functional tests where these POJOs are used.
So, I would advise focusing more on functionality when writing future tests.
I look forward to more contributions from you 🙂 .
| public class SSLClientConfigTest | ||
| { | ||
| @Test | ||
| public void testGetters() |
There was a problem hiding this comment.
I don't think this test serves any purpose.
The test is verifying values which the test had itself configured the mock object to return.
There was a problem hiding this comment.
Makes sense. Removing the tests in the next commit.
| public void testPushgatewayStrategy() | ||
| { | ||
| PrometheusEmitterConfig config = new PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.pushgateway, "druid", null, null, "localhost:9091", false, false, 30, null, true, 5000L); | ||
| Assert.assertEquals(PrometheusEmitterConfig.Strategy.pushgateway, config.getStrategy()); | ||
| Assert.assertEquals("druid", config.getNamespace()); | ||
| Assert.assertEquals("localhost:9091", config.getPushGatewayAddress()); | ||
| Assert.assertFalse(config.isAddHostAsLabel()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testPushGatewayStrategyWithoutAddress() | ||
| { | ||
| Assert.assertThrows(IllegalArgumentException.class, () -> { | ||
| new PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.pushgateway, null, null, null, null, false, false, null, null, null, null); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testDefaultFlushPeriodForPushgateway() |
There was a problem hiding this comment.
These 3 can be merged into a single test. They don't seem to add a lot of value by themselves.
There was a problem hiding this comment.
Done. Combined the three tests into one test for PushGatewayStrategy. Pushing in the next commit.
| public void testInvalidFlushPeriod() | ||
| { | ||
| Assert.assertThrows(IllegalArgumentException.class, () -> { | ||
| new PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.pushgateway, null, null, null, "localhost:9091", false, false, 0, null, null, null); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testInvalidExtraLabelName() | ||
| { | ||
| Assert.assertThrows(DruidException.class, () -> { | ||
| Map<String, String> extraLabels = new HashMap<>(); | ||
| extraLabels.put("invalid label", "value"); | ||
| new PrometheusEmitterConfig(null, null, null, null, null, false, false, null, extraLabels, null, null); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testNegativeWaitForShutdownDelay() |
There was a problem hiding this comment.
For the invalid cases, maybe try to verify the exception message as well. You may use AssertionMatcher or DruidExceptionMatcher etc.
There was a problem hiding this comment.
Done, verified the exception message as well. Pushed in the commit.
| public void testGetTaskIds() | ||
| { | ||
| List<String> taskIds = Arrays.asList("task1", "task2", "task3"); | ||
| MSQTaskList msqTaskList = new MSQTaskList(taskIds); | ||
| assertEquals(taskIds, msqTaskList.getTaskIds()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testConstructorWithNullTaskIds() | ||
| { | ||
| Executable executable = () -> new MSQTaskList(null); | ||
| assertThrows(NullPointerException.class, executable); | ||
| } | ||
|
|
There was a problem hiding this comment.
These two tests don't add much value.
There was a problem hiding this comment.
Makes sense. Removing the tests in the next commit.
| public void testExporterStrategy() | ||
| { | ||
| PrometheusEmitterConfig config = new PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.exporter, "druid", null, 8080, null, true, true, null, null, null, null); | ||
| Assert.assertEquals(PrometheusEmitterConfig.Strategy.exporter, config.getStrategy()); | ||
| Assert.assertEquals("druid", config.getNamespace()); | ||
| Assert.assertEquals(8080, config.getPort()); | ||
| Assert.assertTrue(config.isAddHostAsLabel()); | ||
| Assert.assertTrue(config.isAddHostAsLabel()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExporterStrategyWithoutPort() | ||
| { | ||
| Assert.assertThrows(IllegalArgumentException.class, () -> { | ||
| new PrometheusEmitterConfig(PrometheusEmitterConfig.Strategy.exporter, null, null, null, null, false, false, null, null, null, null); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Also combined these two tests into one, so that both ExporterStrategy and PushGatewayStrategy will have one test each.
Description
extensions-coreandextensions-contribdirectoriesRelease note
Key changed/added classes in this PR
extensions-contrib/prometheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfigTest.javaextensions-contrib/prometheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/PrometheusEmitterTest.javaextensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/indexing/MSQTaskListTest.javaextensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/indexing/MSQTuningConfigTest.javaextensions-core/simple-client-sslcontext/pom.xmlextensions-core/simple-client-sslcontext/src/test/java/org/apache/druid/https/SSLClientConfigTest.javaThis PR has: