twitter / summingbird

Streaming MapReduce with Scalding and Storm
https://twitter.com/summingbird
Apache License 2.0
2.14k stars 267 forks source link

Refactor `StormLaws` #727

Closed ttim closed 7 years ago

ttim commented 7 years ago

In this PR I refactored StormLaws class with a way to test same Producer on both Storm and Memory platforms and compare their results. In future this approach can be extended to Scalding platform as well with a very unified way of testing.

Also I've included test for https://github.com/twitter/summingbird/issues/725, where we build Storm topology which fails with ClassCastException in runtime.

codecov-io commented 7 years ago

Codecov Report

Merging #727 into develop will decrease coverage by 0.35%. The diff coverage is 94.82%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #727      +/-   ##
===========================================
- Coverage    71.38%   71.02%   -0.36%     
===========================================
  Files          141      144       +3     
  Lines         3491     3548      +57     
  Branches       195      194       -1     
===========================================
+ Hits          2492     2520      +28     
- Misses         999     1028      +29
Impacted Files Coverage Δ
...a/com/twitter/summingbird/MemoryTestExecutor.scala 100% <100%> (ø)
...n/scala/com/twitter/summingbird/TestPlatform.scala 100% <100%> (ø)
...cala/com/twitter/summingbird/storm/TestStore.scala 70.58% <100%> (ø) :arrow_up:
.../com/twitter/summingbird/PlatformTransformer.scala 90.32% <90.32%> (ø)
...a/com/twitter/summingbird/storm/StormTestRun.scala 60% <0%> (-30%) :arrow_down:
...la/com/twitter/summingbird/storm/BuildSummer.scala 42.22% <0%> (-22.23%) :arrow_down:
...ain/scala/com/twitter/summingbird/TestGraphs.scala 88.53% <0%> (-5.1%) :arrow_down:
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 731da7f...d25bbf8. Read the comment docs.

ttim commented 7 years ago

@johnynek @pankajroark thoughts on this one?

johnynek commented 7 years ago

👍

Nice cleanup.

ttim commented 7 years ago

@johnynek What I really like here is a fact we can introduce shared test suite for Storm and Scalding platforms after introduction of proper memory platform (with notion of time) and Scalding implementation for CreatorCtx.

Should I merge or better to wait for one more review from @pankajroark for example?

ttim commented 7 years ago

@johnynek @pankajroark Hi, I've reworked this change a lot with introduction of TestPlatform which corresponds to previous CreatorCtx, can you take a look? I think it's much more Summingbirdish approach.