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 Cardano.Api.Tx generators #2985

Merged
merged 1 commit into from
Oct 28, 2021
Merged

Add Cardano.Api.Tx generators #2985

merged 1 commit into from
Oct 28, 2021

Conversation

sevanspowell
Copy link
Contributor

@sevanspowell sevanspowell commented Oct 21, 2021

  • Copied and extended Tx generators from the cardano-api library. We were unable to import them due to issues with cabal sublibs.

  • Made a first pass at providing coverage tests for as many of the generators as possible.

    • Further note: I've decided to take a "best-effort" approach to providing coverage to generators. Coverage of generators is difficult, particularly for sum types and large product types. If you can see a better way of doing this, please let me know!
  • Best reviewed commit-by-commit

Issue Number

ADP-1147

@sevanspowell sevanspowell self-assigned this Oct 21, 2021
@sevanspowell sevanspowell force-pushed the feature/adp-1147-tx-gen branch 8 times, most recently from be8a161 to a1a28cd Compare October 27, 2021 04:41
@sevanspowell sevanspowell marked this pull request as ready for review October 27, 2021 04:47
@rvl rvl force-pushed the feature/adp-1147-tx-gen branch from 65ec3d4 to bb9ef33 Compare October 27, 2021 07:33
@rvl
Copy link
Contributor

rvl commented Oct 27, 2021

@sevanspowell I have rebased this onto latest master.

@sevanspowell
Copy link
Contributor Author

@sevanspowell I have rebased this onto latest master.

Thank you!

@sevanspowell sevanspowell force-pushed the feature/adp-1147-tx-gen branch 2 times, most recently from 74ee8cb to 27efd39 Compare October 27, 2021 08:21
@rvl
Copy link
Contributor

rvl commented Oct 27, 2021

How come there are two PRs for the same jira ticket with almost identical titles? #2975 and #2985.
I'm confused.

@sevanspowell
Copy link
Contributor Author

How come there are two PRs for the same jira ticket with almost identical titles? #2975 and #2985. I'm confused.

Sorry, that's my fault.

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.

Good thanks.
Please squash the branch though - it's actually harder to review the single commits for things like this.

Comment on lines 138 to 158
describe "genTxInsCollateral" $ do
it "ByronEra" $
property
$ forAll (genTxInsCollateral ByronEra)
$ genTxInCollateralCoverage ByronEra
it "ShelleyEra" $
property
$ forAll (genTxInsCollateral ShelleyEra)
$ genTxInCollateralCoverage ShelleyEra
it "AllegraEra" $
property
$ forAll (genTxInsCollateral AllegraEra)
$ genTxInCollateralCoverage AllegraEra
it "MaryEra" $
property
$ forAll (genTxInsCollateral MaryEra)
$ genTxInCollateralCoverage MaryEra
it "AlonzoEra" $
property
$ forAll (genTxInsCollateral AlonzoEra)
$ genTxInCollateralCoverage AlonzoEra
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be possible to factor such code by using AnyCardanoEra. I put some code in the other PR - #2975 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I tried to to this using existential quantification but then I had issues with the era quantifier escaping it's scope (skolem error). I'll try again with AnyCardanoEra.

@sevanspowell sevanspowell force-pushed the feature/adp-1147-tx-gen branch from 789ee49 to 620350a Compare October 27, 2021 08:54
@sevanspowell
Copy link
Contributor Author

it's actually harder to review the single commits for things like this.

Differing views on this. I will leave multiple commits for the review process and squash before bors. That way those that prefer multiple commits can review commit-by-commit, and those that don't can view the diff or squash locally.

lib/core/test/unit/Cardano/Api/GenSpec.hs Outdated Show resolved Hide resolved
@sevanspowell
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Oct 28, 2021
2985: Add Cardano.Api.Tx generators r=sevanspowell a=sevanspowell

- [ ] Copied and extended Tx generators from the cardano-api library. We were unable to import them due to issues with cabal sublibs.
- [ ] Made a first pass at providing coverage tests for as many of the generators as possible.
    - Further note: I've decided to take a "best-effort" approach to providing coverage to generators. Coverage of generators is difficult, particularly for sum types and large product types. If you can see a better way of doing this, please let me know!

- Best reviewed commit-by-commit
    
### Issue Number

ADP-1147


Co-authored-by: Samuel Evans-Powell <[email protected]>
Co-authored-by: IOHK <[email protected]>
@sevanspowell
Copy link
Contributor Author

bors cancel

- Copied and extended Tx generators from the cardano-api library. We were unable
  to import them due to issues with cabal sublibs.
- Made a first pass at providing coverage tests for as many of the generators as
  possible.
    - NOTE: I've decided to take a "best-effort" approach to providing coverage
    to generators. Coverage of generators is difficult, particularly for sum
    types and large product types. If you can see a better way of doing this,
    please let me know!

Co-authored-by: Rodney Lorrimar <[email protected]>
@sevanspowell sevanspowell force-pushed the feature/adp-1147-tx-gen branch from 8a6fdf5 to db9553e Compare October 28, 2021 04:33
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 28, 2021

Canceled.

@sevanspowell
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 28, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit d29a579 into master Oct 28, 2021
@iohk-bors iohk-bors bot deleted the feature/adp-1147-tx-gen branch October 28, 2021 05:15
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