-
Notifications
You must be signed in to change notification settings - Fork 166
Add CBOR golden tests #5424
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
base: master
Are you sure you want to change the base?
Add CBOR golden tests #5424
Conversation
096540b to
b840866
Compare
a47ac70 to
b3aeb3b
Compare
lehins
left a comment
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.
I really like the direction where we are going with these golden tests!!!
Thank you for doing this work.
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Imp/Common.hs
Outdated
Show resolved
Hide resolved
libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
eras/alonzo/impl/testlib/Test/Cardano/Ledger/Alonzo/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
| defaultTests = | ||
| describe "Alonzo tests" $ do | ||
| AdaPreservation.tests @AlonzoEra 50 | ||
| Golden.tests |
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.
Did you intend to remove these tests?
I suspect what happened was you introduced a module with the same name into cardano-ledger-alonzo:testlib:Test.Cardano.Ledger.Alonzo.Golden as the one in cardano-ledger-alonzo-test:library:Test.Cardano.Ledger.Alonzo.Golden and this resulted in cabal complaining, which led to you removing this line.
Problem is that it effectively disables those tests, which we would like to keep, regardless how ugly they look.
An easy solution would be to simply moves the contents of this file: eras/alonzo/test-suite/src/Test/Cardano/Ledger/Alonzo/Golden.hs intto
eras/alonzo/impl/test/Test/Cardano/Ledger/Alonzo/GoldenSpec.hs. Note that you'll also need to move the actual golden files used by that module.
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.
I think I meant to replace it with Test.Cardano.Ledger.Binary.Golden and then call the old test tree from that spec, but then forgot to actually add the test here. I think merging those two modules together like you suggested makes much more sense anyways.
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.
I tried moving the tests out of Alonzo's test-suite, but it seems like it would entail moving some helper functions out of Mary's test-suite as well. I think this would be better done in a separate PR since there would be a lot of changes.
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.
related #3623
eras/dijkstra/impl/testlib/Test/Cardano/Ledger/Dijkstra/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
eras/conway/impl/testlib/Test/Cardano/Ledger/Conway/Binary/Golden.hs
Outdated
Show resolved
Hide resolved
f583a3d to
9de7817
Compare
5c4aa3b to
38c6db3
Compare
f9a82db to
2516532
Compare
Co-authored-by: Alexey Kuleshevich <[email protected]>
2516532 to
a8f435c
Compare
Description
This PR adds a bunch of CBOR golden tests. There are two kinds of new tests in this PR:
TxWitsfails starting withConwayTxCerts are not decoded starting withConway(close Add positiveduplicateCertsTxtests toShelleyandConway#5390)I also added
forEachEraVersionthat runs tests with each valid protocol version in that era.Checklist
CHANGELOG.mdfiles updated for packages with externally visible changes.NOTE: New section is never added with the code changes. (See RELEASING.md).
.cabalandCHANGELOG.mdfiles when necessary, according to theversioning process.
.cabalfiles updated when necessary.NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
scripts/fourmolize.sh).scripts/cabal-format.sh).scripts/gen-cddl.sh)hie.yamlupdated (usescripts/gen-hie.sh).