Skip to content
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

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Add a test of create-cardano #907

merged 1 commit into from
Sep 24, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Sep 20, 2024

Changelog

- description: |
    Add a test of create-cardano
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
  - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Since I'm going to share some code between create-cardano and create-testnet-data (for #765), I thought this was a good occasion to at least call create-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

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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/.

Copy link
Contributor

@carbolymer carbolymer Sep 24, 2024

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.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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 🙁

@smelc smelc added this pull request to the merge queue Sep 24, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 24, 2024
@carbolymer carbolymer added this pull request to the merge queue Sep 24, 2024
Merged via the queue into main with commit 028f22c Sep 24, 2024
25 checks passed
@carbolymer carbolymer deleted the smelc/test-create-cardano branch September 24, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants