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

Bump to cardano-node alonzo-white-1.4 #2801

Merged
merged 9 commits into from
Aug 9, 2021
Merged

Bump to cardano-node alonzo-white-1.4 #2801

merged 9 commits into from
Aug 9, 2021

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Aug 4, 2021 •
edited
Loading

Issue Number

ADP-1062

Overview

  • Bump to alonzo-white-1.4
  • Figure out how to fix the CLI tests
  • Fix any newly appearing CI errors

Comments

input-output-hk/cardano-haskell#53
input-output-hk/cardano-haskell#54

@Anviking Anviking self-assigned this Aug 4, 2021
@rvl
Copy link
Contributor

rvl commented Aug 4, 2021

Don't forget to make a PR from your branch on cardano-haskell.
The CI there should hopefully find errors if any.

@Anviking Anviking force-pushed the anviking/ADP-1062/alonzo-white-1.4 branch 6 times, most recently from 1c71306 to e5d73d7 Compare August 6, 2021 15:21
@Anviking Anviking requested a review from rvl August 6, 2021 15:23
@Anviking Anviking force-pushed the anviking/ADP-1062/alonzo-white-1.4 branch from e5d73d7 to 73ecf8b Compare August 6, 2021 15:25
@Anviking
Copy link
Member Author

Anviking commented Aug 6, 2021 •
edited
Loading

Hm, hydra was fully green before I bumped optparse-applicative again... 👀

Reverting

@Anviking Anviking force-pushed the anviking/ADP-1062/alonzo-white-1.4 branch from 7bbf540 to c5c2a19 Compare August 6, 2021 16:20
rvl
rvl approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

lib/test-utils/src/Test/Hspec/Goldens.hs Outdated
readGolden :: FilePath -> IO (Either IOError Text)
readGolden f = try (TIO.readFile f)
readGolden f
| os == "mingw32" = try $ TIO.readFile (f <> ".win") <|> TIO.readFile f
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a helper function Test.Utils.Platform.isWindows that you can use here.

The Alternative instance of IO won't work as we need here.

For example:

λ import UnliftIO.Exception 
λ try (throwString "bad" <|> putStrLn "good") :: IO (Either SomeException ())
Left UnliftIO.Exception.throwString called with:

bad
Called from:
 throwString
    ( <interactive>: 8 : 6 in interactive:Ghci4 )
λ

I have pushed a fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔

λ> try (throwString "bad" <|> putStrLn "good") :: IO (Either SomeException ())
Left UnliftIO.Exception.throwString called with:
bad
Called from:
  throwString (<interactive>:21:6 in interactive:Ghci4)

λ> try (throwIO (userError "bad") <|> putStrLn "good") :: IO (Either SomeException ())
good
Right ()

Copy link
Member Author

Choose a reason for hiding this comment

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

The Alternative instance of IO won't work as we need here.

I doubt that was a problem though I don't understand the above behaviour, but the other changes in your fix look fair enough.

lib/cli/test/data/Cardano/CLISpec/key from-recovery-phrase --help.win Outdated

Example:
$ unit.exe recovery-phrase generate \
| unit.exe key from-recovery-phrase Icarus
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these golden files have newlines at the end? Our CLI does print a final newline.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think they used to have: https://github.com/input-output-hk/cardano-wallet/blob/e4503c84811713cc66d857a32763e29e2994127a/lib/cli/test/unit/Cardano/CLISpec.hs#L131-L139

Perhaps the CLI would print an extra newline, but the string we get from optparse-applicative in CLISpec does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I like newlines at the end of text files, so I added a golden post-processing step to do that.

@rvl rvl force-pushed the anviking/ADP-1062/alonzo-white-1.4 branch 2 times, most recently from 2c4df37 to 0713d13 Compare August 9, 2021 07:39
@Anviking Anviking force-pushed the anviking/ADP-1062/alonzo-white-1.4 branch from 0713d13 to 6168f4c Compare August 9, 2021 08:32
@Anviking
Copy link
Member Author

Anviking commented Aug 9, 2021

The commit ordering of

  1. Re-write CLI tests without changing them (with passing tests)
  2. Bump, causing CLI goldens to change
    was very intentional (modulo a windows fixup)

I've now rebased.

@rvl
Copy link
Contributor

rvl commented Aug 9, 2021

OK fair enough. By ordering commits like that you have shown that the layout from this forked optparse-applicative is not as good as the previous formatting.

I added a fix to make the cli unit tests on windows pass.

@rvl rvl force-pushed the anviking/ADP-1062/alonzo-white-1.4 branch from d427bea to 76e3a16 Compare August 9, 2021 09:33
@Anviking
Copy link
Member Author

Anviking commented Aug 9, 2021

Thanks
bors r+

iohk-bors bot added a commit that referenced this pull request Aug 9, 2021
@iohk-bors @Anviking @rvl
Merge #2801
3e3bdbb 
2801: Bump to cardano-node alonzo-white-1.4 r=Anviking a=Anviking

# Issue Number

ADP-1062


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Bump to alonzo-white-1.4
- [x] Figure out how to fix the CLI tests
- [x] Fix any newly appearing CI errors 


# Comments

input-output-hk/cardano-haskell#53
input-output-hk/cardano-haskell#54

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 9, 2021

Canceled.

@rvl
Copy link
Contributor

rvl commented Aug 9, 2021

The cli unit tests were failing on Windows. I have added a fix for that.

@Anviking
Copy link
Member Author

Anviking commented Aug 9, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 9, 2021
@iohk-bors @Anviking @rvl
Merge #2801
d0ece28 
2801: Bump to cardano-node alonzo-white-1.4 r=Anviking a=Anviking

# Issue Number

ADP-1062


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Bump to alonzo-white-1.4
- [x] Figure out how to fix the CLI tests
- [x] Fix any newly appearing CI errors 


# Comments

input-output-hk/cardano-haskell#53
input-output-hk/cardano-haskell#54

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 9, 2021 •
edited by Anviking
Loading

Build failed:

#2812

@rvl
Copy link
Contributor

rvl commented Aug 9, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 9, 2021
@iohk-bors @Anviking @rvl
Merge #2801
4513082 
2801: Bump to cardano-node alonzo-white-1.4 r=rvl a=Anviking

# Issue Number

ADP-1062


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] Bump to alonzo-white-1.4
- [x] Figure out how to fix the CLI tests
- [x] Fix any newly appearing CI errors 


# Comments

input-output-hk/cardano-haskell#53
input-output-hk/cardano-haskell#54

<!-- Additional comments or screenshots to attach if any -->

<!--
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
 ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages.
-->


Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 9, 2021 •
edited by Anviking
Loading

Build failed:

#2812

@Anviking
Copy link
Member Author

Anviking commented Aug 9, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 9, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit b759e88 into master Aug 9, 2021
@iohk-bors iohk-bors bot deleted the anviking/ADP-1062/alonzo-white-1.4 branch August 9, 2021 15:58
@Anviking Anviking mentioned this pull request Aug 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvl rvl rvl approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Anviking @rvl