Speed up TransactionSpec.signTransaction.
#3211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit attempts to tidy up parts of `TransactionSpec` where the style has diverged greatly from our agreed-upon team coding style. In particular, this commit tries to restore: - 80 column lines - 4 column indents It doesn't fix everything, but at least it's now easily viewable on an 80-column-wide editor window.
This also slightly improves the readability of the test suite output, so that the name of each property is written as a section header, rather than being repeated for all eras.
…rsals. With rough benchmarking, this makes tests run around 15% faster.
These properties take a long time to run (over 10 minutes in total), so we can get by (for the moment) by testing fewer examples. We remove some calls to `checkCoverage` where there are no corresponding calls to `cover`, as this interferes with the action of `withMaxSuccess`.
sevanspowell
approved these changes
Mar 31, 2022
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Mar 31, 2022
3211: Speed up `TransactionSpec.signTransaction`. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1485 ## Summary This PR: - adjusts `checkSubsetOf` to use set-based lookups instead of repeated list traversals. - adjusts `signTransaction` properties to only test 10 examples per property, rather than 100. This results in a substantial reduction in total time required to run the `signTransaction` tests: ``` Before: 530 ± 10 seconds (over 4 runs) After: 48 ± 2 seconds (over 4 runs) ``` ## Additional Improvements This PR also: - introduces function `spec_forAllEras` to eliminate some repetition in the `signTransaction` spec . - makes the output of `signTransaction` property tests more readable, so that the name of each property is written once as a section header, rather than being repeated for every single era. (The individual eras are still as subsections.) ```hs Cardano.Wallet.Shelley.Transaction Sign transaction signTransaction adds reward account witness when necessary ByronEra +++ OK, passed 10 tests (100% feature not supported in ByronEra). ShelleyEra (2144ms) +++ OK, passed 10 tests. AllegraEra (2143ms) +++ OK, passed 10 tests. MaryEra (2142ms) +++ OK, passed 10 tests. AlonzoEra (2142ms) +++ OK, passed 10 tests. signTransaction adds extra key witnesses when necessary ByronEra +++ OK, passed 10 tests (100% feature not supported in ByronEra). ShelleyEra +++ OK, passed 10 tests (100% feature not supported in ShelleyEra). AllegraEra +++ OK, passed 10 tests (100% feature not supported in AllegraEra). MaryEra +++ OK, passed 10 tests (100% feature not supported in MaryEra). AlonzoEra (686ms) +++ OK, passed 10 tests. ``` Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
|
Build failed: Test failure: Failures:
test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:2225:13:
1) Cardano.Wallet.Shelley.Transaction.balanceTransaction, when passed unresolved inputs, may fail
*** Gave up! Passed only 99 tests; 7 discarded tests (14% unknown txins).
To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/balanceTransaction/when passed unresolved inputs/may fail/"
Randomized with seed 1053383965This exact failure is also reproducible in |
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Mar 31, 2022
3211: Speed up `TransactionSpec.signTransaction`. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1485 ## Summary This PR: - adjusts `checkSubsetOf` to use set-based lookups instead of repeated list traversals. - adjusts `signTransaction` properties to only test 10 examples per property, rather than 100. This results in a substantial reduction in total time required to run the `signTransaction` tests: ``` Before: 530 ± 10 seconds (over 4 runs) After: 48 ± 2 seconds (over 4 runs) ``` ## Additional Improvements This PR also: - introduces function `spec_forAllEras` to eliminate some repetition in the `signTransaction` spec . - makes the output of `signTransaction` property tests more readable, so that the name of each property is written once as a section header, rather than being repeated for every single era. (The individual eras are still as subsections.) ```hs Cardano.Wallet.Shelley.Transaction Sign transaction signTransaction adds reward account witness when necessary ByronEra +++ OK, passed 10 tests (100% feature not supported in ByronEra). ShelleyEra (2144ms) +++ OK, passed 10 tests. AllegraEra (2143ms) +++ OK, passed 10 tests. MaryEra (2142ms) +++ OK, passed 10 tests. AlonzoEra (2142ms) +++ OK, passed 10 tests. signTransaction adds extra key witnesses when necessary ByronEra +++ OK, passed 10 tests (100% feature not supported in ByronEra). ShelleyEra +++ OK, passed 10 tests (100% feature not supported in ShelleyEra). AllegraEra +++ OK, passed 10 tests (100% feature not supported in AllegraEra). MaryEra +++ OK, passed 10 tests (100% feature not supported in MaryEra). AlonzoEra (686ms) +++ OK, passed 10 tests. ``` Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
ADP-1485
Summary
This PR:
checkSubsetOfto use set-based lookups instead of repeated list traversals.signTransactionproperties to only test 10 examples per property, rather than 100.This results in a substantial reduction in total time required to run the
signTransactiontests:Additional Improvements
This PR also:
spec_forAllErasto eliminate some repetition in thesignTransactionspec .signTransactionproperty tests more readable, so that the name of each property is written once as a section header, rather than being repeated for every single era. (The individual eras are still as subsections.)