[BEAM-2558] Migrate checkCombineFn in TestUtils to CombineFnTester by tgroh · Pull Request #3628 · apache/beam

tgroh · 2017-07-24T17:55:29Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

This makes CombineFnTester significantly more discoverable, and usable
without having dependencies on the test JAR.

Update existing tests.

This is purely a move of code with minor renaming, visibility limiting, and
updates to include empty accumulators on both ends of mergeAccumulators.

Worth considering is also ensuring that the output (as returned by extractOutput)
is not the same as any of the merged accumulators (as doing so potentially interacts
poorly with a lifted Combine)

tgroh · 2017-07-24T20:27:08Z

R: @robertwb @kennknowles (either)

robertwb · 2017-07-25T20:56:15Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/CombineFnTester.java

+      List<? extends Iterable<InputT>> shards,
+      Matcher<? super OutputT> matcher) {
+    checkCombineFnShardsWithEmptyAccumulators(fn, shards, matcher);
+    Collections.shuffle(shards); checkCombineFnShardsWithEmptyAccumulators(fn, shards, matcher);


Split into two lines?

robertwb · 2017-07-25T20:57:28Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/CombineFnTester.java

+    Collections.shuffle(shards); checkCombineFnShardsWithEmptyAccumulators(fn, shards, matcher);
+  }
+
+  private static <InputT, AccumT, OutputT> void checkCombineFnShardsWithEmptyAccumulators(


Should we also check without empty accumulators?

robertwb · 2017-07-25T21:00:39Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/CombineFnTester.java

+    checkCombineFnShardsMultipleOrders(fn, shardExponentially(input, 1.4), matcher);
+    checkCombineFnShardsMultipleOrders(fn, shardExponentially(input, 2), matcher);
+    checkCombineFnShardsMultipleOrders(fn, shardExponentially(input, Math.E), matcher);
+  }


This utility was written before streaming. Should we also test that style of combining, e.g.

extractOutput( merge(createAccumulator().addInput(v1), merge(createAccumulator().addInput(v2), merge(createAccumulator().addInput(v3), ...

This makes CombineFnTester significantly more discoverable, and usable without having dependencies on the test JAR. Update existing tests.

robertwb

LGTM, pending tests. Thanks.

coveralls · 2017-07-25T23:22:16Z

Changes Unknown when pulling f5edcc8 on tgroh:combine_fn_tester into ** on apache:master**.

coveralls · 2017-07-25T23:22:54Z

Changes Unknown when pulling f5edcc8 on tgroh:combine_fn_tester into ** on apache:master**.

tgroh · 2017-07-26T17:54:21Z

Added tests.

mingmxu · 2017-07-26T18:43:07Z

retest this please

robertwb · 2017-07-26T19:05:59Z

sdks/java/core/src/test/java/org/apache/beam/sdk/testing/CombineFnTesterTest.java

+public class CombineFnTesterTest {
+  @Test
+  public void checksMergeWithEmptyAccumulators() {
+    final AtomicBoolean sawEmpty = new AtomicBoolean(false);


Why does this need to be atomic?

It needs to be mutable and also final; AtomicBoolean is just an easy way to do that.

robertwb · 2017-07-26T19:10:24Z

sdks/java/core/src/test/java/org/apache/beam/sdk/testing/CombineFnTesterTest.java

+  @Test
+  public void checksWithShards() {
+    final AtomicBoolean sawManyShards = new AtomicBoolean();
+    CombineFn<Integer, int[], Integer> combineFn =


Might be cleaner to just use an Integer for the accumulator as performance doesn't matter here.

robertwb · 2017-07-26T19:13:19Z

sdks/java/core/src/test/java/org/apache/beam/sdk/testing/CombineFnTesterTest.java

+
+          @Override
+          public List<Integer> addInput(List<Integer> accumulator, Integer input) {
+            if (!accumulator.isEmpty() && accumulator.get(accumulator.size() - 1) > input) {


This line is a bit obscure, comment?

robertwb · 2017-07-26T19:19:38Z

sdks/java/core/src/test/java/org/apache/beam/sdk/testing/CombineFnTesterTest.java

+          }
+        };
+    CombineFnTester.testCombineFn(
+        Sum.ofIntegers(), Arrays.asList(1, 1, 2, 2, 3, 3, 4, 4, 5, 5), matcher);


For this kind of test I think it's more succinct to use a matcher that matches nothing (or the wrong thing) and verify that CombineFnTester.testCombine fails. (For writing TesterTesters, my preference is generally to write a Fn that fails (just) the property you are trying to validate, and verify that the tester rejects it. However, this works as well.)

I have done both.

coveralls · 2017-07-26T20:18:03Z

Coverage increased (+0.08%) to 70.478% when pulling 00ccc9f on tgroh:combine_fn_tester into 69a51c9 on apache:master.

robertwb

LGTM

coveralls · 2017-07-26T22:30:30Z

Coverage increased (+0.06%) to 70.464% when pulling 2215f4a on tgroh:combine_fn_tester into b67a30b on apache:master.

tgroh force-pushed the combine_fn_tester branch 2 times, most recently from e350c59 to e930810 Compare July 25, 2017 18:14

robertwb reviewed Jul 25, 2017

View reviewed changes

Migrate checkCombineFn in TestUtils to CombineFnTester

3509c11

This makes CombineFnTester significantly more discoverable, and usable without having dependencies on the test JAR. Update existing tests.

tgroh force-pushed the combine_fn_tester branch from e930810 to 3509c11 Compare July 25, 2017 21:56

robertwb approved these changes Jul 25, 2017

View reviewed changes

mingmxu mentioned this pull request Jul 26, 2017

[BEAM-2676] move BeamSqlRow and BeamSqlRowType to sdk/java/core #3633

Closed

tgroh force-pushed the combine_fn_tester branch from f5edcc8 to 00ccc9f Compare July 26, 2017 17:54

robertwb reviewed Jul 26, 2017

View reviewed changes

fixup! Migrate checkCombineFn in TestUtils to CombineFnTester

2215f4a

tgroh force-pushed the combine_fn_tester branch from 00ccc9f to 2215f4a Compare July 26, 2017 21:06

robertwb approved these changes Jul 26, 2017

View reviewed changes

asfgit closed this in e206b1a Jul 26, 2017

mingmxu mentioned this pull request Jul 26, 2017

sync-up from master branch #3648

Closed

Conversation

tgroh commented Jul 24, 2017

Uh oh!

tgroh commented Jul 24, 2017

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!

robertwb Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 25, 2017

Uh oh!

coveralls commented Jul 25, 2017

Uh oh!

tgroh commented Jul 26, 2017

Uh oh!

mingmxu commented Jul 26, 2017

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!

coveralls commented Jul 26, 2017

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robertwb Jul 25, 2017 •

edited

Loading