-
Notifications
You must be signed in to change notification settings - Fork 156
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
Added UnitTestTools and IncrementalStakeTest #4242
Conversation
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/UnitTestTools.hs
Outdated
Show resolved
Hide resolved
853ee07
to
251eda9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test looks awesome!
As usual I have a bunch of suggestions, but none of them are about the test case. Hopefully you'll like my suggestion to improve the tuple solution 😁
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/UnitTests/IncrementalStakeTest.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/UnitTestTools.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/UnitTestTools.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/UnitTestTools.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/UnitTestTools.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/UnitTests/IncrementalStakeTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/UnitTests/IncrementalStakeTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/UnitTests/IncrementalStakeTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/UnitTests/IncrementalStakeTest.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/UnitTestTools.hs
Outdated
Show resolved
Hide resolved
251eda9
to
c30016c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful.
Thank you for your patience, Tim!
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/UnitTests/IncrementalStakeTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/UnitTests/IncrementalStakeTest.hs
Outdated
Show resolved
Hide resolved
bcf9bf2
to
5af4596
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Versioning is off.
67c91ab
to
03c5874
Compare
…utes the part of the stake attributed to rewards.
03c5874
to
51023c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect
This is a second try at addressing #4171.
It replaces PR #4211, which has been abandoned.
Issue #4171 identified some potential problems with the code in [eras/shelley/impl/src/Cardano/Ledger/Shelley/LedgerState/IncrementalStake.hs]. It suggested some tests to resolve the issue. Such a test need specific values with types such as Credential, StakeReference, UtxO, IncrementalStake, PParams, Rewards, Deposits, all with complex interrelation-ships.
Test.Cardano.Ledger.UnitTestTools makes it easy to express such relationships
Test.Cardano.Ledger.Shelley.UnitTests.IncrementalStakeTest exhibits a test, that resolves the issue.
Checklist
.cabal
andCHANGELOG.md
files according to theversioning process.
.cabal
files for all affected packages are updated. If you change the bounds in a cabal file, that package itself must have a version increase. (See RELEASING.md)CHANGELOG.md
for the affected packages. New section is never added with the code changes. (See RELEASING.md)fourmolu
(usescripts/fourmolize.sh
)scripts/cabal-format.sh
)hie.yaml
has been updated (usescripts/gen-hie.sh
)