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

Improve clarity of test failures within prop_performSelection. #2960

Merged
merged 11 commits into from
Oct 8, 2021

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Oct 8, 2021

Issue Number

ADP-1037

Summary

This PR uses the report and verify combinators to clarify the output of failures for the BalanceSpec.prop_performSelection property.

Currently, failures of this property are hard to debug, since even if only one assertion fails, then the names of all successful assertions prior to the failed assertion will also be appended to the output. (This is a limitation of assertWith.)

Details

  • The report combinator automatically pretty-prints values that are passed to it, making it much easier to inspect the internal structure of counterexample values.
  • The verify combinator produces clearer output than assertWith, since verify produces output that's specific to the condition that fails, whereas assertWith can only append to the current counterexample text, making it unsuitable for use in situations where there are many things to verify.

Both of these combinators require a non-monadic Property context, so this PR also converts most of prop_performSelection to a non-monadic style. (Only a small portion of this property actually needs to be run within monadicIO.)

Sample output

sample-failure-for-performSelection

@jonathanknowles jonathanknowles changed the title Improve clarity of test failures for prop_performSelection. Improve clarity of test failures within prop_performSelection. Oct 8, 2021
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/adp-1037/test-coin-selection-3 branch from 60b5c41 to c1cddc5 Compare October 8, 2021 03:54
@jonathanknowles jonathanknowles marked this pull request as ready for review October 8, 2021 03:54
@jonathanknowles jonathanknowles self-assigned this Oct 8, 2021
It's not actually necessary to blind this argument, since it's already
blinded by the outer properties that call this property, and those
properties fully determine the value of the argument.
We now can easily verify that each of these properties is monadic.

Later commits will convert each of these properties to a non-monadic
style, enabling us to use the `report` and `verify` combinators to
clarify the output of failures.
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/adp-1037/test-coin-selection-3 branch from bd73303 to 96b6218 Compare October 8, 2021 04:13
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/adp-1037/test-coin-selection-3 branch from 96b6218 to cb2ce80 Compare October 8, 2021 04:35
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/adp-1037/test-coin-selection-3 branch from cb2ce80 to 9409c10 Compare October 8, 2021 04:41
We can't re-use the `report` name here, as this is already imported.

So we use `transformationReport` instead.
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/adp-1037/test-coin-selection-3 branch from 9409c10 to 6c639b8 Compare October 8, 2021 05:14
@jonathanknowles
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Oct 8, 2021
2960: Improve clarity of test failures within `prop_performSelection`. r=jonathanknowles a=jonathanknowles

## Issue Number

ADP-1037

## Summary

This PR uses the [**`report`**](https://github.com/input-output-hk/cardano-wallet/blob/b1ded0657c3d7fafcd7bbac15d9cd07478e34058/lib/test-utils/src/Test/QuickCheck/Extra.hs#L378) and [**`verify`**](https://github.com/input-output-hk/cardano-wallet/blob/b1ded0657c3d7fafcd7bbac15d9cd07478e34058/lib/test-utils/src/Test/QuickCheck/Extra.hs#L386) combinators to clarify the output of failures for the [`BalanceSpec.prop_performSelection`](https://github.com/input-output-hk/cardano-wallet/blob/942f12ae838fbce0ba00ce0109c7e26ba53e4318/lib/core/test/unit/Cardano/Wallet/Primitive/CoinSelection/BalanceSpec.hs#L821) property.

Currently, failures of this property are hard to debug, since even if only one assertion fails, then the names of all successful assertions prior to the failed assertion will also be appended to the output. (This is a limitation of `assertWith`.)

## Details

* The [**`report`**](https://github.com/input-output-hk/cardano-wallet/blob/b1ded0657c3d7fafcd7bbac15d9cd07478e34058/lib/test-utils/src/Test/QuickCheck/Extra.hs#L378) combinator automatically pretty-prints values that are passed to it, making it much easier to inspect the internal structure of counterexample values.
* The [**`verify`**](https://github.com/input-output-hk/cardano-wallet/blob/b1ded0657c3d7fafcd7bbac15d9cd07478e34058/lib/test-utils/src/Test/QuickCheck/Extra.hs#L386) combinator produces clearer output than `assertWith`, since `verify` produces output that's **_specific_** to the condition that fails, whereas `assertWith` can only append to the current counterexample text, making it unsuitable for use in situations where there are many things to verify.

Both of these combinators require a non-monadic `Property` context, so this PR also converts most of `prop_performSelection` to a non-monadic style. (Only a small portion of this property actually needs to be run within `monadicIO`.)

## Sample output

![sample-failure-for-performSelection](https://user-images.githubusercontent.com/206319/136497971-85fe46ce-c2bb-4f31-b6bd-eaab8dc1b7dd.png)



Co-authored-by: Jonathan Knowles <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 8, 2021

Build failed:

There were no actual test failures in this merge run.

This looks like a spurious failure caused by file locks:

/tmp/stack-e1ec14543d09b86d/test-ghc-env: openBinaryFile: resource busy (file is locked)
 
/tmp/stack-e1ec14543d09b86d/test-ghc-env: openBinaryFile: resource busy (file is locked)
 
/tmp/stack-e1ec14543d09b86d/test-ghc-env: openBinaryFile: resource busy (file is locked)
 
/tmp/stack-e1ec14543d09b86d/test-ghc-env: openBinaryFile: resource busy (file is locked)
 
/tmp/stack-e1ec14543d09b86d/test-ghc-env: openBinaryFile: resource busy (file is locked)
 
/tmp/stack-e1ec14543d09b86d/test-ghc-env: openBinaryFile: resource busy (file is locked)
 
/tmp/stack-e1ec14543d09b86d/test-ghc-env: openBinaryFile: resource busy (file is locked)
error: Command exited with code 1!

@jonathanknowles
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 8, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 7128fbc into master Oct 8, 2021
@iohk-bors iohk-bors bot deleted the jonathanknowles/adp-1037/test-coin-selection-3 branch October 8, 2021 07:36
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