Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix property tests #421

Merged
merged 5 commits into from
Mar 2, 2015
Merged

Conversation

MansurAshraf
Copy link
Contributor

Most property tests such as monoidLaw, monadLaw etc were not running properly due to combination of org.scalatest.prop.PropertyChecks with org.scalacheck.Prop.forAll. Scalatest mark test as a failure only when an exception is thrown using various matchers etc, most Algebird laws indicate failure by returning false inside org.scalacheck.Prop.forAll. Due to this mismatch Scalatest was passing all the laws even when they failed as no exception was thrown. I have changed the tests to use org.scalatest.prop.Checkers instead of org.scalatest.prop.PropertyChecks which delegates to Scalatest for test verification keeping the old behavior of false for failure true for success and error for exception

@MansurAshraf
Copy link
Contributor Author

@johnynek any idea why monoid laws for Array[Int] be failing?

[info] - Array Monoid laws *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (CheckProperties.scala:12)
[info]     Falsified after 0 successful property evaluations.
[info]     Location: (CheckProperties.scala:12)
[info]     Occurred when passed generated values (
[info]       arg0 = Array()
[info]     )

@johnynek
Copy link
Collaborator

This was added when the tests were broken:
https://github.com/twitter/algebird/blob/develop/algebird-core/src/main/scala/com/twitter/algebird/Monoid.scala#L127

in fact it helped us notice they are broken.

I guess there is a corner case there.

One idea is to use labels on Props to see what part of the law failed:
https://github.com/rickynils/scalacheck/blob/master/src/main/scala/org/scalacheck/Prop.scala#L138

But the problem is the return types of all these things:
https://github.com/twitter/algebird/blob/develop/algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

are Boolean, not Prop (which should probably be changed, I suppose).

In the mean time, can you load the repl and see what is going wrong?

My guess, since it is only one arg is that isNonZero is broken (because Array equality is reference equality). I guess isNonZero(a: Array[T]) = a.nonEmpty might fix it.

The isNonZero is a pretty busted idea. We should have taken Equiv (or our own typeclass) for that, and these cases where there are many representations of zero (Array(), Array(0), Array(0, 0)) are really problematic.

johnynek added a commit that referenced this pull request Mar 2, 2015
@johnynek johnynek merged commit fb307df into twitter:develop Mar 2, 2015
@johnynek
Copy link
Collaborator

johnynek commented Mar 2, 2015

So glad to have these back running.

@miguno
Copy link

miguno commented Mar 9, 2015

(Just chuckled because it was Arrays again -- for better or worse. :-P)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants