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

Rewrite generators to use the QC size parameter #2829

Merged
merged 13 commits into from
Aug 17, 2021

Conversation

jonathanknowles
Copy link
Member

Issue Number

Follow-up from #2768
Supports work for #2819

Comments

This PR adjusts generators for the following types to use the QC size parameter:

  • Address
  • Hash "Tx"
  • TxIndex (Word32)
  • TxIn
  • TxOut
  • UTxO
  • UTxOIndex

Some minor adjustments to coverage conditions were necessary (but surprisingly few).

This PR also:

  • Adds the module UTxO.Gen, so that we can generate values of UTxO without having to generate indices (which add extra overhead), and redefines the UTxOIndex generators in terms of these generators.
  • Adds the functions genSized2 and genSized2With, which restore size linearity to generators of compound values (those defined in terms of other generators).

@jonathanknowles jonathanknowles self-assigned this Aug 17, 2021
AssetId
<$> resize sizeSquareRoot genTokenPolicyId
<*> resize sizeSquareRoot genTokenName
genAssetId = genSized2With AssetId genTokenPolicyId genTokenName
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha nice!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

If we ever have to make a generator for values composed of more than two parts, we can easily make a similar genSized3/genSizedWith3 function.

Though I'm hoping we won't need such a thing. 😄

@jonathanknowles jonathanknowles force-pushed the jonathanknowles/sized-generators-shrinkers branch from 118243f to 9465479 Compare August 17, 2021 06:32
@sevanspowell sevanspowell self-requested a review August 17, 2021 06:35
@jonathanknowles
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 17, 2021
2829: Rewrite generators to use the QC size parameter r=jonathanknowles a=jonathanknowles

### Issue Number

Follow-up from #2768
Supports work for #2819 

### Comments

This PR adjusts generators for the following types to use the QC size parameter:
- `Address`
- `Hash "Tx"`
- `TxIndex` (`Word32`)
- `TxIn`
- `TxOut`
- `UTxO`
- `UTxOIndex`

Some minor adjustments to coverage conditions were necessary (but surprisingly few).

This PR also:
- Adds the module `UTxO.Gen`, so that we can generate values of `UTxO` without having to generate indices (which add extra overhead), and redefines the `UTxOIndex` generators in terms of these generators.
- Adds the functions `genSized2` and `genSized2With`, which restore size linearity to generators of compound values (those defined in terms of other generators).

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

iohk-bors bot commented Aug 17, 2021

Build failed:

Couldn't determine why this build failed. It seems that it ended shortly after reaching this point:

[13 of 92] Compiling Cardano.Wallet.Primitive.Types.TokenQuantity ( src/Cardano/Wallet/Primitive/Types/TokenQuantity.hs, dist/build/Cardano/Wallet/Primitive/Types/TokenQuantity.o, dist/build/Cardano/Wallet/Primitive/Types/TokenQuantity.dyn_o )
[14 of 92] Compiling Cardano.Wallet.Primitive.Types.TokenMap ( src/Cardano/Wallet/Primitive/Types/TokenMap.hs, dist/build/Cardano/Wallet/Primitive/Types/TokenMap.o, dist/build/Cardano/Wallet/Primitive/Types/TokenMap.dyn_o )

#2703

This is consistent with the convention for many other types. For example:

- unCoin
- unNegativeCoin
- unNodePort
- unPretty
- unTokenQuantity

The `un` prefix is arguably more consistent with the meaning, which is
to *unwrap* a `UTxO` rather than to *get* a `UTxO`.
These combinators make it possible to restore size linearity to
generators composed of other generators that depend on the size
parameter.
This commit:

  - creates a new module `UTxO.Gen` which has generators and shrinkers for
    ordinary UTxO sets (without indices).

  - redefines the functions in `UTxOIndex.Gen` to reuse those in `UTxO.Gen`.

We can now use the generators within `UTxO.Gen` to generate plain UTxO sets
without having to generate indices (which carry more overhead).
This commit makes some small adjustments to coverage checks in light of
recent changes to generators.
It's expected that these tests can fail after changes to generators.

The new values are still within acceptable tolerances.
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/sized-generators-shrinkers branch from 40f89d1 to 1750489 Compare August 17, 2021 08:06
@jonathanknowles
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 17, 2021
2829: Rewrite generators to use the QC size parameter r=jonathanknowles a=jonathanknowles

### Issue Number

Follow-up from #2768
Supports work for #2819 

### Comments

This PR adjusts generators for the following types to use the QC size parameter:
- `Address`
- `Hash "Tx"`
- `TxIndex` (`Word32`)
- `TxIn`
- `TxOut`
- `UTxO`
- `UTxOIndex`

Some minor adjustments to coverage conditions were necessary (but surprisingly few).

This PR also:
- Adds the module `UTxO.Gen`, so that we can generate values of `UTxO` without having to generate indices (which add extra overhead), and redefines the `UTxOIndex` generators in terms of these generators.
- Adds the functions `genSized2` and `genSized2With`, which restore size linearity to generators of compound values (those defined in terms of other generators).

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

iohk-bors bot commented Aug 17, 2021

Build failed:

Failure:

Failures:
   src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:391:59:
   1) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_ESTIMATE_09 - Invalid amount, 1.5
        uncaught exception: IOException of type ResourceVanished
        fd:40: hFlush: resource vanished (Broken pipe)

   To rerun use: --match "/CLI Specifications/SHELLEY_CLI_TRANSACTIONS/TRANS_ESTIMATE_09 - Invalid amount/1.5/"

Randomized with seed 745043718

Finished in 948.0664 seconds, used 1077.7424 seconds of CPU time
859 examples, 1 failure, 42 pending

#2855

Although what is interesting here, is that the TransactionsCLI spec precedes WalletsCLI where wallets were leaked (fixed by #2854).

@jonathanknowles
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 17, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 71d7a13 into master Aug 17, 2021
@iohk-bors iohk-bors bot deleted the jonathanknowles/sized-generators-shrinkers branch August 17, 2021 10:08
-- >>> <*> scaleToRoot 3 genB
-- >>> <*> scaleToRoot 3 genC
--
scaleToRoot :: Int -> Gen a -> Gen a
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice idea which might be needed very occasionally, but in general, I don't believe we need to fret too much over the size parameter. It's not meant to be so precise - for example, instance Arbitrary (a,b,c,d,e) in QuickCheck doesn't do any such scaling. From the developer perspective, it's fairly obvious that if you have a big ADT then its total size and range will be bigger than that of a small ADT.

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.

3 participants