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

Fix shrinker for ProtocolParameters. #3205

Merged

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented Mar 29, 2022

Currently, the shrinker for ProtocolParameters always returns the empty list:

> :set -XTypeApplications
> import Cardano.Wallet.Primitive.Types (ProtocolParameters)
> import Cardano.Wallet.DB.Arbitrary
> import Test.QuickCheck
> generate (arbitrary @ProtocolParameters)
ProtocolParameters {decentralizationLevel = ...}
> a = it
> shrink a
[]

By using genericRoundRobinShrink from Test.QuickCheck.Extra, we can get actual shrinking:

> generate (arbitrary @ProtocolParameters)
ProtocolParameters {decentralizationLevel = ...}
> a = it
> length $ shrink a
68

@jonathanknowles jonathanknowles self-assigned this Mar 29, 2022
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/protocol-parameters-shrinker branch from 4d2f641 to 020ab5d Compare March 29, 2022 13:16
Currently, the shrinker always returns the empty list:

```hs
> :set -XTypeApplications
> import Cardano.Wallet.Primitive.Types (ProtocolParameters)
> import Cardano.Wallet.DB.Arbitrary
> import Test.QuickCheck
> generate (arbitrary @ProtocolParameters)
ProtocolParameters {decentralizationLevel = ...}
> a = it
> shrink a
[]
```

By using `genericRoundRobinShrink`, we can get actual shrinking:

```hs
> generate (arbitrary @ProtocolParameters)
ProtocolParameters {decentralizationLevel = ...}
> a = it
> length $ shrink a
68
```
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/protocol-parameters-shrinker branch from b863a2f to 172457a Compare March 29, 2022 13:23
@sevanspowell
Copy link
Contributor

sevanspowell commented Mar 30, 2022

Ah interesting, the default shrink implementation for Arbitrary is simply to return an empty list.

When an empty list is used applicatively like this, it acts as an annihilator:

> [(+1)] <*> []
[]

Some elements of the ProtocolParameters use the default instance for shrinking, hence the empty list when shrinking ProtocolParameters.

We should probably audit our shrinkers for this.

This is a case where Hedgehog provides much more sensible defaults, in that it provides default shrinkers that actually shrink.

@sevanspowell sevanspowell self-requested a review March 30, 2022 03:03
@Unisay
Copy link
Contributor

Unisay commented Mar 30, 2022

Yeah this applicative annihilation is a real gotcha!
(and the best shrinker is that one that you don't have to write)

@jonathanknowles
Copy link
Member Author

bors r+

iohk-bors bot added a commit that referenced this pull request Mar 30, 2022
3205: Fix shrinker for `ProtocolParameters`. r=jonathanknowles a=jonathanknowles

Currently, the shrinker for `ProtocolParameters` always returns the empty list:

```hs
> :set -XTypeApplications
> import Cardano.Wallet.Primitive.Types (ProtocolParameters)
> import Cardano.Wallet.DB.Arbitrary
> import Test.QuickCheck
> generate (arbitrary `@ProtocolParameters)`
ProtocolParameters {decentralizationLevel = ...}
> a = it
> shrink a
[]
```

By using `genericRoundRobinShrink` from `Test.QuickCheck.Extra`, we can get actual shrinking:

```hs
> generate (arbitrary `@ProtocolParameters)`
ProtocolParameters {decentralizationLevel = ...}
> a = it
> length $ shrink a
68
```

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

iohk-bors bot commented Mar 30, 2022

Build failed:

Final output of log:

  Sign transaction
    signTransaction adds reward account witness when necessary (AnyCardanoEra ByronEra)
      +++ OK, passed 100 tests (100% feature not supported in ByronEra.).
    signTransaction adds reward account witness when necessary (AnyCardanoEra ShelleyEra) (30749ms)
      +++ OK, passed 100 tests.
    signTransaction adds reward account witness when necessary (AnyCardanoEra AllegraEra) (31215ms)
      +++ OK, passed 100 tests.
    signTransaction adds reward account witness when necessary (AnyCardanoEra MaryEra) (30973ms)
      +++ OK, passed 100 tests.
    signTransaction adds reward account witness when necessary (AnyCardanoEra AlonzoEra) (30539ms)
      +++ OK, passed 100 tests.
    signTransaction adds extra key witness when necessary (AnyCardanoEra ByronEra) (1ms)
      +++ OK, passed 100 tests (100% feature not supported in ByronEra.).
    signTransaction adds extra key witness when necessary (AnyCardanoEra ShelleyEra)
      +++ OK, passed 100 tests (100% feature not supported in ShelleyEra.).
    signTransaction adds extra key witness when necessary (AnyCardanoEra AllegraEra)
      +++ OK, passed 100 tests (100% feature not supported in AllegraEra.).
    signTransaction adds extra key witness when necessary (AnyCardanoEra MaryEra)
      +++ OK, passed 100 tests (100% feature not supported in MaryEra.).
    signTransaction adds extra key witness when necessary (AnyCardanoEra AlonzoEra) (72958ms)
      +++ OK, passed 100 tests.
    signTransaction adds tx in witnesses when necessary (AnyCardanoEra ByronEra) (1ms)
      +++ OK, passed 100 tests.

Looks like another instance of:

https://input-output.atlassian.net/browse/ADP-1485

@jonathanknowles
Copy link
Member Author

Going to merge manually, since:

@jonathanknowles jonathanknowles merged commit 57635e1 into master Mar 30, 2022
@jonathanknowles jonathanknowles deleted the jonathanknowles/protocol-parameters-shrinker branch March 30, 2022 14:23
WilliamKingNoel-Bot pushed a commit that referenced this pull request Mar 30, 2022
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