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 coverage checks to property tests for TokenMap.intersection #2756

Merged
merged 13 commits into from
Jul 13, 2021

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Jul 9, 2021

Issue Number

ADP-997

Pre-merge checks

  • Perform soak test of test suite to ensure no flakiness has been introduced.
    (✅ 1000 test runs with --match "/Token map properties/Arithmetic/prop_intersection" passed with no failures.)

Overview

PR #2725 added a TokenMap.intersection function and property tests.

This PR:

  • adds coverage checks to the prop_intersection_* series of properties.
  • where necessary, adjusts property inputs in order to attain the required level of coverage.

@jonathanknowles jonathanknowles self-assigned this Jul 9, 2021
Copy link
Contributor

@sevanspowell sevanspowell left a comment

Choose a reason for hiding this comment

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

Looks good to me!

-- generate larger values of 'A', based on simple monoidal concatenation of
-- multiple smaller values.
--
newtype ManyFolded a = ManyFolded
Copy link
Contributor

@sevanspowell sevanspowell Jul 12, 2021

Choose a reason for hiding this comment

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

This is required because the available Arbitrary instance for TokenMap is something like this:

instance Arbitrary TokenMap where
    arbitrary = genTokenMapSmallRange

right?

I think I prefer Hedgehog's use of functions to Quickcheck's typeclass instances, but that's not the question here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found this confusing @jonathanknowles.

Adding in Blind didn't help (couldn't Show be defined with show . mconcat?

Why can't we just use the QuickCheck size parameter? See Coding Standards - QuickCheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @rvl. I agree with you that using the size parameter would be a better solution here.

I've added a commit which does this here: 86f4856

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Thanks @jonathanknowles .
Coverage checks - good.
ManyFolded - unsure.

-- generate larger values of 'A', based on simple monoidal concatenation of
-- multiple smaller values.
--
newtype ManyFolded a = ManyFolded
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this confusing @jonathanknowles.

Adding in Blind didn't help (couldn't Show be defined with show . mconcat?

Why can't we just use the QuickCheck size parameter? See Coding Standards - QuickCheck.

This allows the generation and shrinking of token quantities from a range
that depends on the size parameter.
This allows the generation and shrinking of token names from a range
that depends on the size parameter.
jonathanknowles added a commit that referenced this pull request Jul 13, 2021
In this change, we use sized generators for token maps instead of
relying on the `ManyFolded` combinator.

In response to review feedback:

#2756 (comment)
jonathanknowles added a commit that referenced this pull request Jul 13, 2021
In this change, we use sized generators for token maps instead of
relying on the `ManyFolded` combinator.

In response to review feedback:

#2756 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/token-map-intersection-test-coverage branch from 86f4856 to 4fae8c4 Compare July 13, 2021 06:22
This allows the generation and shrinking of token policy identifiers
from a range that depends on the size parameter.
This allows the generation and shrinking of asset identifiers from a range
that depends on the size parameter.
This allows the generation and shrinking of token maps where the number
of entries, the range of asset identifiers, and the range of token
quantities all depend on the size parameter.
In this change, we use sized generators for token maps instead of
relying on the `ManyFolded` combinator.

In response to review feedback:

#2756 (comment)
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/token-map-intersection-test-coverage branch from 4fae8c4 to 5673f74 Compare July 13, 2021 06:33
@jonathanknowles
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 13, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit dd41d30 into master Jul 13, 2021
@iohk-bors iohk-bors bot deleted the jonathanknowles/token-map-intersection-test-coverage branch July 13, 2021 08:16
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