ttim commented 7 years ago

This PR bumps scalacheck and scalatest version and fix all tests which started to fail.

Fixed failures:

scalacheck generates better arbitrary functions now, this cause fail of "match scala for leftJoin jobs", "match scala for leftJoin repeated tuple leftJoin jobs" and "match scala for leftJoin with store (no dependency between the two) jobs", also it uncovered that memory law about leftJoin and sumByKey doesn't make sense
oneOf generator fails in case of empty input sequence now
some tests were failing on empty input elements, because they assumed input time interval is empty

johnynek commented 7 years ago

This looks fine to me.

MemoryPlatform is pretty much a toy, and for cases where there are real races in time, it makes sense that the law might fail.

ttim commented 7 years ago

@johnynek note that I changed BatcherLaws in the end. I think this change is ok because a) it shouldn't affect users, b) it makes MillisecondBatcher more consistent - you expect to have exactly durationMillis seconds in each batch, which wasn't true before (if durationMillis is 2, then -1 batch will contains only -1 millisecond).

codecov-io commented 7 years ago

Codecov Report

Merging #722 into develop will increase coverage by 0.44%. The diff coverage is 88.46%.

@@             Coverage Diff             @@
##           develop     #722      +/-   ##
===========================================
+ Coverage    70.96%   71.41%   +0.44%     
===========================================
  Files          141      141              
  Lines         3475     3491      +16     
  Branches       201      195       -6     
===========================================
+ Hits          2466     2493      +27     
+ Misses        1009      998      -11

Impacted Files	Coverage Δ
.../scala/com/twitter/summingbird/memory/Memory.scala	`94.73% <ø> (ø)`	:arrow_up:
...la/com/twitter/summingbird/scalding/TestUtil.scala	`49.12% <100%> (ø)`	:arrow_up:
...twitter/summingbird/batch/MillisecondBatcher.scala	`100% <100%> (ø)`	:arrow_up:
...ain/scala/com/twitter/summingbird/TestGraphs.scala	`93.63% <100%> (+7.73%)`	:arrow_up:
...com/twitter/summingbird/scalding/TestService.scala	`97.5% <100%> (+0.83%)`	:arrow_up:
.../scala/com/twitter/summingbird/batch/BatchID.scala	`82.22% <66.66%> (-1.5%)`	:arrow_down:
.../com/twitter/summingbird/TestGraphGenerators.scala	`96.25% <75%> (-2.4%)`	:arrow_down:
.../twitter/summingbird/memory/ConcurrentMemory.scala	`97.14% <0%> (+1.42%)`	:arrow_up:
...cala/com/twitter/summingbird/batch/TimeStamp.scala	`59.45% <0%> (+2.7%)`	:arrow_up:

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 bf6aa37...fc15619. Read the comment docs.

ttim commented 7 years ago

@johnynek Do you mind to look one more time? Thanks!

johnynek commented 7 years ago

[info] ! BatchID.range, toInterval and toIterable should be equivalent: Exception raised on property evaluation.
[info] > ARG_0: BatchID.9223372036854775807
[info] > ARG_1: 0
[info] > Exception: java.util.NoSuchElementException: Either.right.value on Left

Can you run the batcher tests locally and get them green? Might save time since travis is so slow.

ttim commented 7 years ago

@johnynek I did run it locally and it passed. I guess test you mentioned is flaky, I'm gonna investigate it as well.

Main reason I still didn't merge is this:

The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

Looks like either we started to print a little bit more logs then before (I saw size of logs exactly around 4mb in local runs) or travis changed this limit recently. What I found is that we already try to print much less logs on travis (see last commit in regards to log4j), but it doesn't change anything. I'll merge this branch as soon as I fix this issue.

ttim commented 7 years ago

@johnynek I tried to rerun BatcherLaws locally a lot of times, and I haven't seen this error. Where did you find it?

johnynek commented 7 years ago

on the link from travis. See the red x next to pr?

ttim commented 7 years ago

@johnynek you're right, it's a legit failure: range, toInterval and toIterable should be equivalent fails in case of BatchId == MAX_LONG - 1 & diff == 0 =) It's also very rare that's why I didn't see it locally (even I ran it for 10s of times).

I'm tired of slow travis with non legit exceptions, that's why I missed this one, thanks for pointing out!

I'll fix it.

ttim commented 7 years ago

@johnynek Actually this failure is pretty interesting. Root cause for that is - BatchID.range can handle intervals like [MAX_LONG, MAX_LONG], while it's impossible to handle this interval through Interval.leftClosedRightOpen instances. I'm wondering can we use intervals closed from both sides?

johnynek commented 7 years ago

these tests are so frustrating.

Looks like travis killed 2.11:

/home/travis/.travis/job_stages: line 54:  2273 Killed                  ./sbt -Dlog4j.configuration=$LOG4J ++$TRAVIS_SCALA_VERSION clean coverage test coverageReport mimaReportBinaryIssues

pankajroark commented 7 years ago

Agree, the tests are very frustrating. I'll try to spend some time on them when I get a chance.

pankajroark commented 7 years ago

lgtm

twitter / summingbird

Bump scalacheck and scalatest versions #722

Codecov Report