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 functions {gen,shrink}Tx to Primitive.Tx.Gen #2843

Merged
merged 12 commits into from
Aug 25, 2021

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Aug 25, 2021

Issue Number

ADP-1084

Comments

This PR adds a reusable generator and shrinker for transactions (of type Tx) to the Primitive.Tx.Gen module.

It uses these functions to simplify or eliminate Arbitrary instances in the following modules:

  • Cardano.Wallet.Primitive.ModelSpec
  • Cardano.Wallet.Primitive.TypesSpec
  • Cardano.WalletSpec

This enables the `mockHash` function to be used from core modules.
This function is analogous to `liftShrink2`, but can be applied
to 6-tuples.

This commit could have also added `liftShrink{3,4,5}`, but these are not
currently used. They can be added in future, if needed.
These functions make it possible to generate (and shrink) key-value maps
with custom generators (and shrinkers) for keys and values.
This change applies individual scaling factors to transaction generators
in order to speed up test execution times, without compromising coverage.
@jonathanknowles jonathanknowles self-assigned this Aug 25, 2021
forAllShrink (listOf genTxInputs) (shrinkList shrinkTxInputs)
$ \pendingTxInputs ->
inner utxo pendingTxInputs
forAllShrink (scale (`div` 2) $ listOf genTx) (shrinkList shrinkTx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we scale (div 2) here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like I think I understand the size paramater, but I'm not sure why we need to divide it by two here.

Copy link
Contributor Author

@jonathanknowles jonathanknowles Aug 25, 2021

Choose a reason for hiding this comment

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

By using scale (div 2), we halve the value of the size parameter passed to all the inner generators of genTx, meaning that average size of transactions will be halved (even though the actual size may vary).

In testing, I found that this didn't affect the coverage conditions we care about, while making the tests run faster.

Copy link
Contributor Author

@jonathanknowles jonathanknowles Aug 25, 2021

Choose a reason for hiding this comment

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

In general, users of genTx can choose the size of transactions to generate by manipulating the size parameter in this way.

It's the same principle for most of the other generators that we provide in Gen modules. (Since most of these now generate "sized" values, according to the size parameter.)

If you find that you can scale down the size parameter and keep the coverage you want (which should be explicitly tested), then I think it makes sense to do so, as we can reduce the overall test execution time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also have used scale (mod n) here, for some value of n, which would place a fixed upper limit on the size of transactions (while still allowing the size to vary).

@jonathanknowles
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 25, 2021
2843: Add functions `{gen,shrink}Tx` to `Primitive.Tx.Gen` r=jonathanknowles a=jonathanknowles

### Issue Number

ADP-1084

### Comments

This PR adds a reusable generator and shrinker for transactions (of type `Tx`) to the `Primitive.Tx.Gen` module.

It uses these functions to simplify or eliminate `Arbitrary` instances in the following modules:
- `Cardano.Wallet.Primitive.ModelSpec`
- `Cardano.Wallet.Primitive.TypesSpec`
- `Cardano.WalletSpec`

Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: IOHK <[email protected]>
txWithoutIdToTx :: TxWithoutId -> Tx
txWithoutIdToTx t@TxWithoutId {..} = Tx {..}
where
txId = mockHash $ txWithoutIdToTuple t
Copy link
Contributor Author

@jonathanknowles jonathanknowles Aug 25, 2021

Choose a reason for hiding this comment

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

We probably don't need to convert to a tuple here, as TxWithoutId already has a Show instance:

Suggested change
txId = mockHash $ txWithoutIdToTuple t
txId = mockHash t

Will fix this in follow-up PR.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 25, 2021

Build failed:

This seems to have been caused by weeder errors:
https://buildkite.com/input-output-hk/cardano-wallet/builds/16323#0734f07e-719c-4396-ade0-38837d3c9b3c

#expected

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/gen-shrink-tx branch from 7fb845d to 0551124 Compare August 25, 2021 03:28
@jonathanknowles
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 25, 2021
2843: Add functions `{gen,shrink}Tx` to `Primitive.Tx.Gen` r=jonathanknowles a=jonathanknowles

### Issue Number

ADP-1084

### Comments

This PR adds a reusable generator and shrinker for transactions (of type `Tx`) to the `Primitive.Tx.Gen` module.

It uses these functions to simplify or eliminate `Arbitrary` instances in the following modules:
- `Cardano.Wallet.Primitive.ModelSpec`
- `Cardano.Wallet.Primitive.TypesSpec`
- `Cardano.WalletSpec`

2844: Lower coverage requirement for `prop_selectRandom_one_withAdaOnly`. r=jonathanknowles a=jonathanknowles

This PR lowers a coverage check requirement in `prop_selectRandom_one_withAdaOnly`.

It should fix the following failure, which was seen in overnight testing (2 out of 100 runs):

```hs
Failures:

  test/unit/Cardano/Wallet/Primitive/Types/UTxOIndexSpec.hs:144:9:
  1) Cardano.Wallet.Primitive.Types.UTxOIndex, Indexed UTxO set properties, Index Selection, prop_selectRandom_one_withAdaOnly
       Insufficient coverage (after 1600 tests):
         85.18% selected an entry

         Only 85.18% selected an entry, but expected 90.00%

  To rerun use: --match "/Cardano.Wallet.Primitive.Types.UTxOIndex/Indexed UTxO set properties/Index Selection/prop_selectRandom_one_withAdaOnly/"
```

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

iohk-bors bot commented Aug 25, 2021

Build failed (retrying...):

  src/Test/Integration/Scenario/CLI/Shelley/Wallets.hs:754:18: 
  1) CLI Specifications, SHELLEY_CLI_WALLETS, WALLETS_UTXO_02 - Utxo statistics works properly
       Waited longer than 90s to resolve action: "Wallet balance is as expected".
[cardano-wallet.network:Warning:2382] [2021-08-25 04:38:48.66 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)
[cardano-wallet.network:Warning:52208] [2021-08-25 04:38:48.66 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)
[cardano-wallet.network:Warning:52641] [2021-08-25 04:38:48.67 UTC] Connection lost with the node. Network.Socket.recvBuf: resource vanished (Connection reset by peer)
[cardano-wallet.network:Warning:2380] [2021-08-25 04:38:48.67 UTC] Connection lost with the node. writev: resource vanished (Broken pipe)
       expected: Quantity 1562000000
        but got: Quantity 0

#2429

iohk-bors bot added a commit that referenced this pull request Aug 25, 2021
2843: Add functions `{gen,shrink}Tx` to `Primitive.Tx.Gen` r=jonathanknowles a=jonathanknowles

### Issue Number

ADP-1084

### Comments

This PR adds a reusable generator and shrinker for transactions (of type `Tx`) to the `Primitive.Tx.Gen` module.

It uses these functions to simplify or eliminate `Arbitrary` instances in the following modules:
- `Cardano.Wallet.Primitive.ModelSpec`
- `Cardano.Wallet.Primitive.TypesSpec`
- `Cardano.WalletSpec`

Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: IOHK <[email protected]>
@jonathanknowles
Copy link
Contributor Author

bors r+

@cardano-foundation cardano-foundation deleted a comment from iohk-bors bot Aug 25, 2021
@cardano-foundation cardano-foundation deleted a comment from iohk-bors bot Aug 25, 2021
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 25, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 0880799 into master Aug 25, 2021
@iohk-bors iohk-bors bot deleted the jonathanknowles/gen-shrink-tx branch August 25, 2021 05:49
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