[
https://issues.apache.org/jira/browse/BEAM-4059?focusedWorklogId=98508&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-98508
]
ASF GitHub Bot logged work on BEAM-4059:
----------------------------------------
Author: ASF GitHub Bot
Created on: 04/May/18 17:08
Start Date: 04/May/18 17:08
Worklog Time Spent: 10m
Work Description: swegner commented on a change in pull request #5193:
[BEAM-4059] Reduce number of ValidatesRunner tests and reorganize them for
better parallelization
URL: https://github.com/apache/beam/pull/5193#discussion_r186146909
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/metrics/MetricsTest.java
##########
@@ -69,216 +74,286 @@ private static MetricQueryResults
queryTestMetrics(PipelineResult result) {
.build());
}
- @Rule
- public final transient TestPipeline pipeline = TestPipeline.create();
-
- @Rule
- public final transient ExpectedException thrown = ExpectedException.none();
-
- @After
- public void tearDown() {
- MetricsEnvironment.setCurrentContainer(null);
- }
-
- @Test
- public void testDistributionWithoutContainer() {
- assertNull(MetricsEnvironment.getCurrentContainer());
- // Should not fail even though there is no metrics container.
- Metrics.distribution(NS, NAME).update(5L);
- }
-
- @Test
- public void testCounterWithoutContainer() {
- assertNull(MetricsEnvironment.getCurrentContainer());
- // Should not fail even though there is no metrics container.
- Counter counter = Metrics.counter(NS, NAME);
- counter.inc();
- counter.inc(5L);
- counter.dec();
- counter.dec(5L);
- }
-
- @Test
- public void testCounterWithEmptyName() {
- thrown.expect(IllegalArgumentException.class);
- Metrics.counter(NS, "");
- }
-
- @Test
- public void testCounterWithEmptyNamespace() {
- thrown.expect(IllegalArgumentException.class);
- Metrics.counter("", NAME);
+ /** Shared test helpers and setup/teardown. */
+ public abstract static class SharedTestBase {
+ @Rule
+ public final transient ExpectedException thrown = ExpectedException.none();
+
+ @Rule
+ public final transient TestPipeline pipeline = TestPipeline.create();
+
+ @After
+ public void tearDown() {
+ MetricsEnvironment.setCurrentContainer(null);
+ }
+
+ protected PipelineResult runPipelineWithMetrics() {
+ final Counter count = Metrics.counter(MetricsTest.class, "count");
+ final TupleTag<Integer> output1 = new TupleTag<Integer>(){};
+ final TupleTag<Integer> output2 = new TupleTag<Integer>(){};
+ pipeline
+ .apply(Create.of(5, 8, 13))
+ .apply("MyStep1", ParDo.of(new DoFn<Integer, Integer>() {
+ Distribution bundleDist = Metrics.distribution(MetricsTest.class,
"bundle");
+
+ @StartBundle
+ public void startBundle() {
+ bundleDist.update(10L);
+ }
+
+ @SuppressWarnings("unused")
+ @ProcessElement
+ public void processElement(ProcessContext c) {
+ Distribution values = Metrics.distribution(MetricsTest.class,
"input");
+ count.inc();
+ values.update(c.element());
+
+ c.output(c.element());
+ c.output(c.element());
+ }
+
+ @DoFn.FinishBundle
+ public void finishBundle() {
+ bundleDist.update(40L);
+ }
+ }))
+ .apply("MyStep2", ParDo
+ .of(new DoFn<Integer, Integer>() {
+ @SuppressWarnings("unused")
+ @ProcessElement
+ public void processElement(ProcessContext c) {
+ Distribution values =
Metrics.distribution(MetricsTest.class, "input");
+ Gauge gauge = Metrics.gauge(MetricsTest.class, "my-gauge");
+ Integer element = c.element();
+ count.inc();
+ values.update(element);
+ gauge.set(12L);
+ c.output(element);
+ c.output(output2, element);
+ }
+ })
+ .withOutputTags(output1, TupleTagList.of(output2)));
+ PipelineResult result = pipeline.run();
+
+ result.waitUntilFinish();
+ return result;
+ }
}
- @Test
- public void testDistributionWithEmptyName() {
- thrown.expect(IllegalArgumentException.class);
- Metrics.distribution(NS, "");
+ /** Tests validating basic metric scenarios. */
+ @RunWith(JUnit4.class)
+ public static class BasicTests extends SharedTestBase {
+ @Test
+ public void testDistributionWithoutContainer() {
+ assertNull(MetricsEnvironment.getCurrentContainer());
+ // Should not fail even though there is no metrics container.
+ Metrics.distribution(NS, NAME).update(5L);
+ }
+ @Test
+ public void testCounterWithoutContainer() {
+ assertNull(MetricsEnvironment.getCurrentContainer());
+ // Should not fail even though there is no metrics container.
+ Counter counter = Metrics.counter(NS, NAME);
+ counter.inc();
+ counter.inc(5L);
+ counter.dec();
+ counter.dec(5L);
+ }
+
+ @Test
+ public void testCounterWithEmptyName() {
+ thrown.expect(IllegalArgumentException.class);
+ Metrics.counter(NS, "");
+ }
+
+ @Test
+ public void testCounterWithEmptyNamespace() {
+ thrown.expect(IllegalArgumentException.class);
+ Metrics.counter("", NAME);
+ }
+
+ @Test
+ public void testDistributionWithEmptyName() {
+ thrown.expect(IllegalArgumentException.class);
+ Metrics.distribution(NS, "");
+ }
+
+ @Test
+ public void testDistributionWithEmptyNamespace() {
+ thrown.expect(IllegalArgumentException.class);
+ Metrics.distribution("", NAME);
+ }
+
+ @Test
+ public void testDistributionToCell() {
+ MetricsContainer mockContainer = Mockito.mock(MetricsContainer.class);
+ Distribution mockDistribution = Mockito.mock(Distribution.class);
+
when(mockContainer.getDistribution(METRIC_NAME)).thenReturn(mockDistribution);
+
+ Distribution distribution = Metrics.distribution(NS, NAME);
+
+ MetricsEnvironment.setCurrentContainer(mockContainer);
+ distribution.update(5L);
+
+ verify(mockDistribution).update(5L);
+
+ distribution.update(36L);
+ distribution.update(1L);
+ verify(mockDistribution).update(36L);
+ verify(mockDistribution).update(1L);
+ }
+
+ @Test
+ public void testCounterToCell() {
+ MetricsContainer mockContainer = Mockito.mock(MetricsContainer.class);
+ Counter mockCounter = Mockito.mock(Counter.class);
+ when(mockContainer.getCounter(METRIC_NAME)).thenReturn(mockCounter);
+
+ Counter counter = Metrics.counter(NS, NAME);
+
+ MetricsEnvironment.setCurrentContainer(mockContainer);
+ counter.inc();
+ verify(mockCounter).inc(1);
+
+ counter.inc(47L);
+ verify(mockCounter).inc(47);
+
+ counter.dec(5L);
+ verify(mockCounter).inc(-5);
+ }
}
- @Test
- public void testDistributionWithEmptyNamespace() {
- thrown.expect(IllegalArgumentException.class);
- Metrics.distribution("", NAME);
+ /** Tests for committed metrics. */
+ @RunWith(JUnit4.class)
+ public static class CommittedMetricTests extends SharedTestBase {
+ @Category({ValidatesRunner.class, UsesCommittedMetrics.class,
UsesCounterMetrics.class,
+ UsesDistributionMetrics.class, UsesGaugeMetrics.class})
+ @Test
+ public void testAllCommittedMetrics() {
+ PipelineResult result = runPipelineWithMetrics();
+ MetricQueryResults metrics = queryTestMetrics(result);
+
+ assertAllMetrics(metrics, true);
+ }
+
+ @Category({ValidatesRunner.class, UsesCommittedMetrics.class,
UsesCounterMetrics.class})
+ @Test
+ public void testCommittedCounterMetrics() {
+ PipelineResult result = runPipelineWithMetrics();
+ MetricQueryResults metrics = queryTestMetrics(result);
+ assertCounterMetrics(metrics, true);
+ }
+
+ @Category({ValidatesRunner.class, UsesCommittedMetrics.class,
UsesDistributionMetrics.class})
+ @Test
+ public void testCommittedDistributionMetrics() {
+ PipelineResult result = runPipelineWithMetrics();
+ MetricQueryResults metrics = queryTestMetrics(result);
+ assertDistributionMetrics(metrics, true);
+ }
+
+ @Category({ValidatesRunner.class, UsesCommittedMetrics.class,
UsesGaugeMetrics.class})
+ @Test
+ public void testCommittedGaugeMetrics() {
+ PipelineResult result = runPipelineWithMetrics();
+ MetricQueryResults metrics = queryTestMetrics(result);
+ assertGaugeMetrics(metrics, true);
+ }
+
+ @Test
+ @Category({NeedsRunner.class, UsesAttemptedMetrics.class,
UsesCounterMetrics.class})
Review comment:
For `MetricsTest`, I tried to reduce what I felt was redundant validation of
the runner-provided pieces. I kept a few `@ValidatesRunner` tests exercise
different parts of the `MetricsContainerStepMap`; in particular:
* `testAllAttemptedMetrics()`
* `testAllCommittedMetrics()`
* `testAttempted[Counter|Distribution|Gauge]Metrics()`
* `testCommitted[Counter|Distribution|Gauge]Metrics()`
This test, `testBoundedSourceMetrics()` is validating that metrics from
`BoundedSources` get added into the container. I don't believe this exercises
any new runner-behavior not already covered by the other tests. Which is why I
converted it.
Let me know if you disagree. If there is runner-behavior that needs to be
validated we shouldn't be shy about keeping it as `@ValidatesRunner`. I think
it's better to have long test runs than gaps in our validation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 98508)
Time Spent: 6h (was: 5h 50m)
> Make sure Dataflow ValidatesRunner tests pass in Gradle
> -------------------------------------------------------
>
> Key: BEAM-4059
> URL: https://issues.apache.org/jira/browse/BEAM-4059
> Project: Beam
> Issue Type: Sub-task
> Components: build-system
> Reporter: Pablo Estrada
> Assignee: Scott Wegner
> Priority: Major
> Time Spent: 6h
> Remaining Estimate: 0h
>
> See
> https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Gradle/
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
