-
Notifications
You must be signed in to change notification settings - Fork 16
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 a test of create-cardano #907
Conversation
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.
there's already genesis in cardano-cli/test/cardano-cli-test/files/input/alonzo/genesis.alonzo.spec.json
- why not reuse it?
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.
@carbolymer> are you sure you don't have an untracked file lying around? I don't see it:
→ git ls-files | grep ^cardano-cli | grep genesis | grep json
cardano-cli/test/cardano-cli-golden/files/golden/conway/custom-lovelace-supply-shelley-genesis.json
cardano-cli/test/cardano-cli-golden/files/input/alonzo/genesis.alonzo.spec.json
cardano-cli/test/cardano-cli-golden/files/input/conway/genesis.conway.spec.json
cardano-cli/test/cardano-cli-golden/files/input/shelley/genesis/genesis.spec.json
cardano-cli/test/cardano-cli-golden/files/input/shelley/genesis/relays.json
cardano-cli/test/cardano-cli-test/files/input/conway/create-cardano/genesis.alonzo.spec.json
cardano-cli/test/cardano-cli-test/files/input/conway/create-cardano/genesis.byron.spec.json
cardano-cli/test/cardano-cli-test/files/input/conway/create-cardano/genesis.conway.spec.json
cardano-cli/test/cardano-cli-test/files/input/conway/create-cardano/genesis.shelley.spec.json
The last 4 files are the ones added by this PR.
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.
@carbolymer> Note that I didn't reuse files in cardano-cli/test/cardano-cli-golden/files/input/
, because my test is not a golden test, so my inputs files need to be in cardano-cli/test/cardano-cli-test/files/input/
.
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.
Ah right, I've confused paths. The files I mentioned (in my other comments too) are in cardano-cli-golden.
My main concern here is having two checked-in files representing the same thing. Their values (protocol params, cost models etc.) rely on some hidden assumptions and simplifications, so it may be harder to figure out the correct genesis for a new test you'd be potentially writing in the future if you have multiple choices available.
The other thing is updating them. In case of conway, the values were changing from time to time, so it was annoying to update manually input files.
I guess maybe we should make a test checking if they're up to date with mainnet ones? 🤔 I guess it does not matter at this very moment much - conway genesis won't change much.
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 it would be better to put it in cardano-cli/test/cardano-cli-test/files/input/byron/genesis.byron.spec.json
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.
Same we already have conway genesis in repo
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.
Same, we already have shelley genesis in cardano-cli/test/cardano-cli-test/files/input/shelley/genesis/genesis.spec.json
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.
Same as above, I don't find it 🙁
c1d5553
to
1b5e0f1
Compare
Changelog
Context
Since I'm going to share some code between
create-cardano
andcreate-testnet-data
(for #765), I thought this was a good occasion to at least callcreate-cardano
once in our tests. It's bad to provide a command and not test it at all.How to trust this PR
The new test passes
Checklist