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

Report on which sub-property failed #74

Closed
brentleyjones opened this issue Aug 9, 2015 · 12 comments
Closed

Report on which sub-property failed #74

brentleyjones opened this issue Aug 9, 2015 · 12 comments

Comments

@brentleyjones
Copy link
Contributor

I may be doing something wrong, but when I use sub properties and one of them fails, all I get in the output are the inputs used and the name of the parent property. For example:

property("equtatable") <- forAll {
    (a: Int, b: Int) in

    let reflexive = EquatableTestUtilities.checkReflexive { Constant(a) } <?> "reflexive"
    let symmetric = EquatableTestUtilities.checkSymmetric { Constant(a) } <?> "symmetric"
    let transitive = EquatableTestUtilities.checkTransitive { Constant(a) } <?> "transitive"
    let notEquate = (a != b) ==> {
        EquatableTestUtilities.checkNotEquate(
            { Constant(b) },
            { Constant(b) }
        )
    }() <?> "not-equate"

    return reflexive ^&&^ symmetric ^&&^ transitive ^&&^ notEquate
}

will just say that "equatable" failed, instead of also telling me that "not-equate" failed:

*** Failed! Proposition: equtatable
Falsifiable (after 1 test):
0
1
*** Passed 0 tests

If I get it to go past 1 test, it looks even weirder, saying 100% of the sub-properties passed?:

*** Failed! Proposition: equtatable
Falsifiable (after 2 tests and 2 shrinks):
-2
0
*** Passed 1 tests
(100%, not-equate, transitive, symmetric, reflexive)

Basically, I feel it would be really valuable to know which sub-properties have failed. Sometimes custom arbitrary types are expensive to compute and it's easier to do many sub-properties off the same random values.

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

^&&^ is for precisely the case you're trying to avoid here: treating multiple sub-properties as one contiguous entity.

What you're seeing is also normal. What SwiftCheck is telling you is

*** Failed! Proposition: equtatable // The proposition failed
Falsifiable (after 2 tests and 2 shrinks): // With these arguments
-2
0
*** Passed 1 tests // But hey, you passed one test
(100%, not-equate, transitive, symmetric, reflexive) // And all of your sub-properties did too.

SwiftCheck also offers a variety of combinators to make custom behavior (like what I think you're asking for) possible. But I think some combination of verbose, conjunction, and disjunction should suffice for your problem.

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

I do see a bug in the pluralizer, tho. That will be fixed shortly.

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

As for the matter of expensive objects in tests, it may be possible SwiftCheck is causing over-retains if you happen to capture things inside the testing block (which gets bandied around quite a bit of tree-mutations before actually being invoked). The natural solution to this is to use a smart-pointer-esque value type.

Otherwise I can think of 2 other ways to help with this:

  1. (if expensive means the type is recursive) using Actions rather than direct instances of Arbitrary has been very helpful.

  2. (if expensive means expensive) You may consider using once with some outer test block that returns the actual test inside the block. Like

forAll { (x : ExpensiveFoo) in
   return forAll { (a : Int, b : Int)
        return x.doTestedThings(a, b)
   }
}.once

Or give forAll an explicit generator:

forAll(Gen.pure(ExpensiveFoo(createExpensivelyWithThing: thing))) { (x) in
   return forAll { (a : Int, b : Int)
        return x.doTestedThings(a, b)
   }
}

@brentleyjones
Copy link
Contributor Author

Thank you for taking the time to respond to this.

I see in the comments now about how conjunction will mask lower failures. Going directly to disjunction though isn't very useful, since I want a failure if any of them fail the whole. Based on your comment it seems that to get that I have to make a convoluted combination of them (which I currently can't picture, sorry about that).

Also, I'm only testing values right now, so it's not over-retains. It's simply lots of Dictionaries 😄. once is a good tip, but in this case I want to have a normal spread of randomness on the values, I just want to be able to do many different tests with those values, and if a single one of those tests fail I want to whole to fail.

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

Try verbose along with your sub-properties then for initially failing tests. SwiftCheck should give you a TMI view of things that will allow you to patch your test cases one by one until you can peel off the band-aid and go back to regular old conjunctions.

Vanilla QuickCheck isn't even designed to do what you ask, though ScalaCheck is (because they have a very different architecture from us). Though we have had similar thoughts at one point in time (see #41).

@brentleyjones
Copy link
Contributor Author

Okay. That's a good band-aid, but given randomness, it might take a bit for it to happen again. In that case, I would instead take the inputs given and manually try each of the sub-properties until I saw which one was failing... which seems to fly in the face of the FIRST principle for tests, and really isn't something I would expect contributors to my project to try to figure out if they get a failing test case.

Anyway, it does seem that I wasn't really doing anything wrong, this just isn't supported currently, in the way I want to use it. I would understand if no change will be made for it, it just seemed counter-intuitive that it didn't report which sub-property was failing (I feel in the conjunction case, regardless of what it is, if you gave it a label that means it's important enough to be reported on in the failure case).

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

That seems a bit hyperbolic. Manual, here, looks like this

return reflexive.verbose ^&&^ symmetric.verbose ^&&^ transitive.verbose ^&&^ notEquate.verbose

or even

return reflexive.whenFail { "Reflexive fail" } ^&&^ symmetric.whenFail { "symmetric fail" } ^&&^ transitive.whenFail { "transitive fail" } ^&&^ notEquate.whenFail { "not equate fail" }

Then you just take off the verboses for passing cases. Point is QuickCheck (and by extension, SwiftCheck) care about the test case as a piece of data, as a whole whose parts are simply parts contributing. You have all the tools you need to examine the sub-parts of a failing test already, it's just a matter of using the right combination to produce a passing test.

@brentleyjones
Copy link
Contributor Author

I personally think that when a test fails I should be able to tell why it failed, without having to modify it. I can agree to disagree here though.

So, I did exactly what you prescribed, and I think I ran into a different issue:

Failed:
*** Failed! Failed:
Proposition: equtatable
Falsifiable (after 1 test and 2 shrinks):
0
1
*** Passed 0 tests

It doesn't seem to be outputting the label of the sub-property.

Edit: Here is an example of it getting further:

Passed:
Passed:
Passed:
Passed:
Failed:
*** Failed! Proposition: equtatable
Falsifiable (after 2 tests):
0
-1
*** Passed 1 tests
(100%, not-equate, transitive, symmetric, reflexive)

(The 100% at the end still feels so weird to me.)

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

I personally think that when a test fails I should be able to tell why it failed, without having to modify it

SwiftCheck is rather bare-bones about it because you have access to quite a bit of the testing mechanism to produce custom output. Or perhaps the original authors were lazy, who knows? 😛

If you can find a way to make this work while still maintaining modularity, I'll merge it.

It doesn't seem to be outputting the label of the sub-property.

There is no sub-property here. verbose applies to the entity it is attached to, not the whole test (you can try .verbose on the whole shebang to see that).

@brentleyjones
Copy link
Contributor Author

There is no sub-property here. verbose applies to the entity it is attached to, not the whole test (you can try .verbose on the whole shebang to see that).

Does that mean:

Passed:
Passed:
Passed:
Passed:
Failed:

isn't bugged? Each entity had a label (via <?>) and .verbose.


For the time being whenFail with print works really well, thank you for that tip. I'll see where that leads me and maybe I can come up with something that is merge worthy if this continues to bug me 😉.

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

Hmph, that may be something that can be fixed. That's expected output from QuickCheck, but you're right: we have a chance to do it better here. I'll see what I can do.

@CodaFi
Copy link
Member

CodaFi commented Aug 9, 2015

With the merge of #76 this can be closed. If you have anything further to add, or I managed to not fix anything, feel free to reopen it or another issue.

@CodaFi CodaFi closed this as completed Aug 9, 2015
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

No branches or pull requests

2 participants