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

Issue 3353: Investigate antisymmetry failure for UpperBounded for Duration #3470

Merged
merged 4 commits into from
Nov 25, 2020
Merged

Issue 3353: Investigate antisymmetry failure for UpperBounded for Duration #3470

merged 4 commits into from
Nov 25, 2020

Conversation

barambani
Copy link
Contributor

This is an attempt to fix #3353. What I observed is that the failure occurs here

def antiSymmetryEq(x: A, y: A, f: A => A): IsEq[Boolean] =
  (!E.eqv(x, y) || E.eqv(f(x), f(y))) <-> true

when x and y are equivalent (such as -4680 hours and -195 days) but f(x) and f(y) are not. The problem should be in the way Cogen for Duration and FiniteDuration works here.

The proposed solution is to use Duration and FiniterDuration's Cogen that ships with ScalaCheck as I assume are correct and tested. It uses the nanoseconds representation for the seed perturbation, that seems reasonable here, in the sense that a function applied to the same duration should give the same result even if it's represented in different units.
I added also a couple of tests that trigger the problem with the current master. To make sure the change actually fixes the case I have been running the tests with

final val IncreasedMinSuccessful: PosInt = if (Platform.isJs) 10 else 100 * 1000 * 1000

without a single failure.

Anyway I'm not sure what's the logic behind the current Cogen's implementation and the semantics of those numbers, so I'm more than happy to close this if it's not a good enough solution.

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2020

Codecov Report

Merging #3470 into master will increase coverage by 0.23%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3470      +/-   ##
==========================================
+ Coverage   91.10%   91.33%   +0.23%     
==========================================
  Files         385      386       +1     
  Lines        8565     8591      +26     
  Branches      260      260              
==========================================
+ Hits         7803     7847      +44     
+ Misses        762      744      -18     

@travisbrown
Copy link
Contributor

Thanks for looking into this. Did you happen to dig up why Cats is currently using custom instances for these types? Dropping them doesn't seem unreasonable to me, but it'd be nice to make sure we know the context and reason someone added them in the first place.

@barambani
Copy link
Contributor Author

barambani commented Jun 15, 2020

That's absolutely right. I could track the addition to this change #1878 by @non that is after the addition of Cogen[Duration] to scalacheck (typelevel/scalacheck#284). Being the scalacheck version already available at the time, there must have been a reason for that implementation in Cats.
What do you think about leaving this open until it's clarified ? Happy to close it if you prefer otherwise though.

@travisbrown
Copy link
Contributor

@barambani Thanks for finding that! Let's leave it open for now.

@barambani barambani requested a review from non June 18, 2020 22:31
Copy link
Contributor

@larsrh larsrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that these implicits are only present in test code, I think it's fairly straightforward to drop them.

@barambani
Copy link
Contributor Author

Thanks for checking 😄 . The risk is we are not exploring enough the space though. The intermittent failure can be because of an error in those numbers but it might also be the case that they are triggering a problem somewhere else. The ideal would be to have some advise from the author ( @non ) but I'm happy to move this on anyway.

@djspiewak
Copy link
Member

I'd rather err on the side of non-flaky builds, so I'm going to merge this.

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.

Investigate antisymmetry failure for UpperBounded for Duration
5 participants