-
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
Add fixup combinators to ImpTest framework #4241
Conversation
dcfd79f
to
a9b5be6
Compare
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
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.
Awesome, even better than what I had in mind!
a9b5be6
to
5ae90f6
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.
This looks great! I have even a follow up idea how we can make it even better:
Let's get rid of iteDoTxFixup
and itePostFixup
in favor of one: iteFixup :: Tx era -> ImpTestM era (Tx era)
For times when no fixup is necessary we can just use pure
, while by default in the runImpTestM
we can use fixupTx
.
Then withPostFixup
will become a composition of the current fixup function that is currently set in iteFixup
and the one that is supplied as an argument.
@Soupstraw what are your thoughts on this subject? Asking you since you really advocated for a boolean flag.
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
@lehins I like that idea, it's a more flexible version of what we have right now. We can still keep the |
d54d4d2
to
01f5726
Compare
I pushed some changes that I believe implement what @lehins 's suggestion was . I have doubts about how this will be used!
|
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.
Looks great! One tiny mistake and a suggestion
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/ImpTest.hs
Outdated
Show resolved
Hide resolved
39bd545
to
ee4b52d
Compare
withPostFixup
to ImpTest frameworkThere 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.
Awesome!
ee4b52d
to
e854891
Compare
e854891
to
58ae385
Compare
58ae385
to
180c854
Compare
180c854
to
6b82e77
Compare
Description
Resolves #4209
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
)