Thanks for reviving this, @Zac-HD !

p.s. Looks like tests are failing with HypothesisDeprecationWarning.

p.s. Looks like tests are failing with HypothesisDeprecationWarning.

Ah, bad interaction with function-scoped fixtures. Just adding (scope='module') is enough to fix that, and makes things marginally more efficient to boot.

Edit: no, wait - we're injecting a context manager, why not just use that directly? One moment...

Edit 2: oh, because it's an expensive operation. Back to module-scoped fixture, plus a comment explaining this time...

Oh, looks like a real failure now. Maybe @taldcroft or @mhvk can make sense of it.

Falsifying example: test_conversion_never_loses_precision(
    iers_b=None, scale1='tai', scale2='tcb', jds=(-31738.500000000346, 0.0),
)

OK, I've marked that one test as xfail and everything else is working.

My preference would be to merge this PR as-is if it's otherwise acceptable, and have someone with more astro background than me investigate it in a follow-up issue - 'has an xfail test' seems to me like an improvement on the no-property-tests status quo 😜

Seems a little heavy-handed to xfail the whole test when only one of the inputs failed, but then again this has dragged on long enough, so I can see pros/cons on both sides. 🤔

Please be patient with us while we wait for the subpackage maintainers to review this thoroughly. Thanks again!

Seems a little heavy-handed to xfail the whole test when only one of the inputs failed

That's the thing - many inputs failed, but Hypothesis reports a single minimal reproducing example. It makes debugging much easier, though it can also make errors look less serious than they really are.

@Zac-HD - the failure is almost certainly due to erfa, in an issue that is being addressed (for overly long discussion, see #6970). I think it makes sense to xfail it for now, so we actually notice if things improve in the future. That said, should there be a test with double the tolerance now that does not fail, so we know we do not regress?

Hello @Zac-HD 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-05-25 01:21:54 UTC

OK, I've done all the investigation I can and the tests are all passing now 😁 @mhvk, I think we're ready to merge!

Happy to help out with the Hypothesis parts going forward, but further work really needs more Astropy expertise than I have at the moment.

OK, I tried to directly fix the failing test for numpy 1.16 instead. If that fails, one can go back to the xfail (but now on the right function 😄)

And of course I did it wrong at least twice, but I think I now found that the failing test was just a case already found before, of a UTC JD close to 0, and had nothing to do with the numpy version (that is the risk of hypothesis, I guess, that the different travis runs do not necessarily try the same tests - good in many ways, but tricky here).

Thanks @mhvk! This is definitely getting into territory where the limit is my knowledge of Time code, not Hypothesis 🙂

OK, tests now pass, so I'll merge. Thanks so much @Zac-HD and @aarchiba for getting this going!!!

You're welcome!

I've updated the list of suggested tests in #9017 - the next set should be faster now that we've sorted out all the dependency and config issues, and upgraded Hypothesis a bit 😁

@bsipocz - I think I did the milestone wrong here. This is really a new type of testing that I think might be better done in 4.2. Is it too late to change it? If you have already included it in 4.1 or 4.0.2, then probably not quite worth reverting either.

Re: milestone -- cc @astrofrog and @eteq

I think this is already in 4.1rc1, no harm in including it and certainly not much point removing it I think. It's also already backported to v4.0.x.