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

Add within assertion helper #65

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

Add within assertion helper #65

wants to merge 1 commit into from

Conversation

jbn
Copy link

@jbn jbn commented Oct 14, 2015

Given the preponderance of statistical and scientific Julia code, I think this pair of assertion helpers are very useful. I use them for unit-testing statistical expectations of stochastic functions. For example,

using Distributions

lower, upper = quantile(Binomial(100, 0.5 * 0.5), [0.01, 0.99])
@fact complicated_function(...) --> within_inclusive(lower, upper)

Seeing the expected lower and upper bound in the failing test output makes my life easier.

@jbn jbn force-pushed the master branch 4 times, most recently from d1547b3 to f3166ea Compare October 14, 2015 12:45
@jakebolewski
Copy link
Contributor

This seems overkill. Why not complicated_function() in 1:<range> --> true?

@jbn
Copy link
Author

jbn commented Oct 14, 2015

I think that has two problems.

1) The error messages are noisy.

@fact 9.0 in 10.0:12.0 --> true
Failure :: (line:507) :: fact was false
    Expression: $(Expr(:in, 9.0, :(10.0:12.0))) --> true
      Expected: true
      Occurred: false

versus

@fact 9.0 --> within(10.0, 12.0)
Failure :: (line:507) :: fact was false
   Expression: 9.0 --> within(10.0,12.0)
     Expected: 9.0 --> within(10.0)

The printed expectation violation is wrong, but I wanted to do as little surgery to FactCheck's special cases as possible. And, the Expression print is very clear.

2) It's doubly confusing with inclusive bounds.

@fact 1 in (1-1):(2+1) --> true

I think I'd tend to agree with you in that special cases don't belong in FactCheck. But, I think this style of test comes up a lot.

Future Work...

I think someting like an AssertionHelper abstract type is a good idea, in general. Derived-types would implement methods for equality checking and better error messages. This allows for well-designed semantics without the need for FactCheck commits.

@jakebolewski
Copy link
Contributor

I would support within(::Range) helper method. Range is already inclusive so within_inclusive is redundant.

within could be a generalization of in collection containment helper function which could give you the better output that you want above.

@jbn
Copy link
Author

jbn commented Oct 14, 2015

Hrm. I made a mistake with the example. The non-inclusive within is not captured by a range. But, that's an edge case, even for distribution testing. And, I think it could be captured by not(within(l,u)) but I haven't looked at how not works yet. Either way, it's less of an issue.

I think you are right -- a within(::Range) method is cleaner. I'll try it out tonight, leaving this unclosed until then.

@jbn jbn changed the title Add within and within_inclusive assertion helpers Add within assertion helper Oct 14, 2015
@jbn
Copy link
Author

jbn commented Oct 14, 2015

Reimplemented, as per discussion with @jakebolewski. It does feel cleaner than my original implementation. The failure presentation is mostly better. It shows lower:step:upper for FloatRange. But, that doesn't run afoul of the principle of least astonishment. Non-inclusive is a simple not(within(x:y)).

@jbn
Copy link
Author

jbn commented Oct 14, 2015

Actually, no. That's not right either. It doesn't work for FloatRanges. The step field is honored by in. This won't work for the most common use case of continuous distribution confidence intervals.
Let me know what alternative implementation -- other than my original one of within(lower, upper) -- will pass muster for a merge.

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.

2 participants