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

create-staked: add test #498

Merged
merged 1 commit into from
Dec 6, 2023
Merged

create-staked: add test #498

merged 1 commit into from
Dec 6, 2023

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Dec 4, 2023

Changelog

- description: |
    This PR adds some tests to the create-staked command
# 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
  # - improvement    # QoL changes e.g. refactoring
  # - 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

  • This PR adds some tests to create-staked, to be able to catch regressions in the list of files being generated and check a bit the content of the generated genesis.json file.
  • Later on, we will port those tests to create-testenet-data (Add create-testnet-data command #488)

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

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

@smelc smelc force-pushed the smelc/test-create-staked branch 2 times, most recently from c36e980 to 95ca168 Compare December 4, 2023 15:46
@smelc smelc marked this pull request as ready for review December 4, 2023 15:47
tree root = do
-- listDirectory returns a path relative to 'root'. We need to prepend
-- root to it for queries below.
content <- map (root </>) <$> listDirectory root
Copy link
Contributor

@carbolymer carbolymer Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the order of the files? If listDirectory does not give us any order
guarantees, it would be nice to sort the list to have consistency between filesystems.

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> actually it's the caller that postprocesses the result of tree to make it stable, along with normalization and making it relative (to avoid depending on a temporary directory name) 👍


import Cardano.Api.Shelley (ShelleyGenesis (sgNetworkMagic, sgStaking))

import Cardano.Ledger.Crypto (StandardCrypto)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StandardCrypto is exposed via Cardano.Api.Ledger. If you find yourself reaching for things directly from the ledger repo, check that it's not already exposed via Cardano.Api.Ledger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to this @Jimbo4350, I could avoid adding a dependency to cardano-ledger-core! Good catch 🙏

@smelc smelc enabled auto-merge December 4, 2023 20:37
@smelc smelc added this pull request to the merge queue Dec 4, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 4, 2023
@smelc smelc enabled auto-merge December 6, 2023 09:27
@smelc smelc added this pull request to the merge queue Dec 6, 2023
Merged via the queue into main with commit 6d2d2b0 Dec 6, 2023
19 checks passed
@smelc smelc deleted the smelc/test-create-staked branch December 6, 2023 10:00
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.

3 participants