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

Generate interesting Days and UniversalTimes #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mossprescott
Copy link

Previously the range of generated Day values was size days before and after 1864-5-8, i.e. about two months in the late nineteenth century. That's both too narrow (only covering part of the year) and doesn't intersect any of the dates which are likely to be of interest to most users of these types.

This makes the range size years, centered on 2000-1-1. With the typical size, that covers most of the era of modern computing and about a decade into the future.

Note: the Day instance is also used to generate LocalDates, UTCTimes, etc., so they also benefit.

Previously the range of generated `Day` values was `size` days before and after 1864-5-8, i.e. about two months in the late nineteenth century. That's both too narrow (only covering part of the year) and doesn't intersect any of the dates which are likely to be of interest to most users of these types.

This makes the range `size` _years_, centered on 2000-1-1. With the typical size, that covers most of the era of modern computing and about a decade into the future.

Note: the Day instance is also used to generate LocalDates, UTCTimes, etc., so they also benefit.
@mossprescott
Copy link
Author

mossprescott commented Dec 29, 2017

cc @rukav

@mossprescott
Copy link
Author

Didn’t want to pester anyone over the holidays, but I wonder if anyone has had a chance to look at this PR. @phadej?

@@ -352,7 +352,10 @@ instance CoArbitrary OldTime.CalendarTime where
-------------------------------------------------------------------------------

instance Arbitrary Time.Day where
arbitrary = Time.ModifiedJulianDay <$> (2000 +) <$> arbitrary
arbitrary = sized $ \n -> do
let limit = (toInteger n)*365
Copy link
Collaborator

@phadej phadej Jan 12, 2018

Choose a reason for hiding this comment

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

n can be big, say 10000, is it ok here? (even 1000 feels fishy...)

Copy link
Author

Choose a reason for hiding this comment

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

There doesn't seem to be any inherent problem with such large values:

λ ModifiedJulianDay $ -10000*365
-8135-07-08
λ ModifiedJulianDay $ 10000*365
11852-03-28
λ UTCTime (ModifiedJulianDay $ -10000*365) 0
-8135-07-08 00:00:00 UTC

@phadej
Copy link
Collaborator

phadej commented Jan 12, 2018

I have to test this on some codebases, if it breaks them, than I'll have a major bump in version.

@phadej
Copy link
Collaborator

phadej commented Feb 12, 2018

As I merged #16 this conflicts now. I'll merge this manually before the release though.

@adamgundry
Copy link

I'd suggest that it is helpful to generate close-together dates (e.g. within a month or two) more frequently, even if you occasionally generate dates across a wide range. Code that does fiddly calculations involving several dates is more likely to hit bugs when the dates are close together.

You also run the risk of exposing "uninteresting" bugs involving dates absurdly far into the past or future.

One could do something like this:

-- | Generate days roughly within a couple of decades of the year 2000, and
-- frequently within a single month (to increase the chance of generating nearby
-- dates that might expose bugs).
instance QC.Arbitrary Day where
  arbitrary = QC.oneof [ QC.choose (read "2000-02-01", read "2000-03-01")
                       , QC.choose (read "1980-01-01", read "2020-01-01")
                       ]
  shrink d = case compare d (read "2000-01-01") of
    LT -> [addDays 1 d]
    EQ -> []
    GT -> [addDays (-1) d]

@Warbo
Copy link

Warbo commented Sep 18, 2024

I just noticed this PR, after previously reporting the same thing. I think the implementation from @mossprescott is pretty reasonable. In particular it will scale up and down based on the size, and the common case of maxSize = 100 will include values before/after the millenium bug, the UNIX epoch and the 2038 epochalypse. That default range won't reach "day 0" of the underlying ModifiedJulianDay, but larger ranges will so that's not too bad. I like the simple shrinking behaviour too.

@adamgundry I agree that close-together values can be important, but that feels like something the size parameter should deal with rather than hard-coding ranges (though I do like your choice of February 2000, as it's an edge case for leap-year calculations). I prefer this PR's shrink to yours, since shrinking towards day 51,544 would likely cause confusion (yes, shrinking to 1858 may be confusing, but that's caused by the time package itself, and explained in its Haddocks; quickcheck-instances shouldn't add even more hurdles to interpreting such counterexamples).

You're also right that "absurdly far" dates aren't too helpful; although again this feels like something the size parameter should control. I think a range like 1980-2020 would be a few decades too small, e.g. it would miss important bug-triggers like the UNIX epoch (1970-01-01) and the 2038 "epochalypse". The PR's use of size = years-from-2000 isn't too much bigger than that, and seems sensible as a general default. Also note that smaller ranges would become less useful over time; for example, we're now 10% outside your suggested range, and this PR hasn't even been merged yet 😉

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.

4 participants