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.

Sign up for GitHub

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

Jump to bottom

Add cardano-testnet queries test: gov-state, stake-distribution #5803

Merged
merged 1 commit into from
Apr 29, 2024
Merged

Add cardano-testnet queries test: gov-state, stake-distribution #5803

merged 1 commit into from
Apr 29, 2024

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Apr 24, 2024 •
edited
Loading

Description

Add cardano-testnet queries test: gov-state, stake-distribution

Part of fixing #5672

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@carbolymer carbolymer force-pushed the mgalazyn/test/gov-state-stake-pools-queries branch 2 times, most recently from fd3efd4 to 8fef808 Compare April 24, 2024 12:14
@carbolymer carbolymer marked this pull request as ready for review April 24, 2024 12:20
@carbolymer carbolymer requested a review from a team as a code owner April 24, 2024 12:20
Jimbo4350
Jimbo4350 reviewed Apr 24, 2024
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

I'll approve as the last commit looks good but the first commit needs a couple of changes I described in the other PR.

cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Cli/Queries.hs
_ :: Aeson.Value <- H.leftFailM . H.readJsonFile $ drepStateOutFile

-- protocol-parameters
do
Copy link
Contributor

Choose a reason for hiding this comment

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

This do is mainly for style purposes right?

Copy link
Contributor Author

@carbolymer carbolymer Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, just to visually separate each query

cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Cli/Queries.hs
let tipOutFile = work </> "tip-out.json"
H.noteM_ $ H.execCli' execConfig [ eraName, "query", "tip"
, "--out-file", tipOutFile]
_ :: QueryTipLocalStateOutput <- H.readJsonFileOk tipOutFile
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use a bang pattern here to be sure we are actually decoding the file?

Copy link
Contributor Author

@carbolymer carbolymer Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That should be done by readJsonFileOk which lifts the Either DecodeError QueryTipLocalStateOutput to MonadTest m => m QueryTipLocalStateOutput, so it should error out at this place already.

cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Cli/Queries.hs Outdated
-- to stdout
stakeDistrOut <- H.execCli' execConfig [ eraName, "query", "stake-distribution" ]
-- stake addresses with stake
let stakeAddresses = map (both T.strip . T.breakOn " " . T.strip . fromString) . drop 2 . lines $ stakeDistrOut
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not return 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.

when writing to a file - yes. When writing to stdout, nope, plain text.

Copy link
Contributor

@smelc smelc Apr 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's not good, I'll fix that: IntersectMBO/cardano-cli#739

cardano-testnet/test/cardano-testnet-test/Cardano/Testnet/Test/Cli/Queries.hs Outdated

H.success

both :: (a -> b) -> (a, a) -> (b, b)
both f ~(x, y) = (f x, f y)
Copy link
Contributor

Choose a reason for hiding this comment

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

https://hackage.haskell.org/package/base-4.19.1.0/docs/Data-Bifunctor.html#v:bimap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if bimap T.strip T.strip is more readable than both T.stip but I don't mind using it instead.

@Jimbo4350 Jimbo4350 self-requested a review April 24, 2024 13:56
@smelc smelc mentioned this pull request Apr 25, 2024
Closed
@smelc smelc mentioned this pull request Apr 26, 2024
Merged
2 tasks
@carbolymer carbolymer force-pushed the mgalazyn/test/gov-state-stake-pools-queries branch 2 times, most recently from 7dafede to 49f08af Compare April 29, 2024 14:44
@carbolymer carbolymer requested a review from smelc April 29, 2024 14:44
@carbolymer carbolymer force-pushed the mgalazyn/test/gov-state-stake-pools-queries branch from 49f08af to d608966 Compare April 29, 2024 14:47
@carbolymer
Copy link
Contributor Author

Macos & windows failures are unrelated to this PR - there's an issue with the base image.

@carbolymer carbolymer merged commit e155f27 into master Apr 29, 2024
10 of 15 checks passed
@carbolymer carbolymer deleted the mgalazyn/test/gov-state-stake-pools-queries branch April 29, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@smelc smelc Awaiting requested review from smelc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carbolymer @smelc @Jimbo4350