POC for polysemy by newhoggy · Pull Request #771 · IntersectMBO/cardano-cli

newhoggy · 2024-05-17T15:08:16Z

Changelog

- description: |
    POC for polysemy
# 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

Using polysemy to simplify tests.

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

newhoggy · 2024-05-17T15:09:38Z

-  expectedTx <- noteInputFile "test/cardano-cli-golden/files/input/byron/tx/normal.tx"
-  createdTx <- noteTempFile tempDir "tx"
-  void $ execCardanoCLI
+hprop_byronTx = propertyOnce $ localWorkspace $ do


The tempDir argument is no longer needed because that information now propagates into our tests via Reader ProjectRoot.

newhoggy · 2024-05-17T15:10:07Z

+hprop_byronTx = propertyOnce $ localWorkspace $ do
+  signingKey <- jotPkgInputFile "test/cardano-cli-golden/files/input/byron/keys/byron.skey"
+  expectedTx <- jotPkgInputFile "test/cardano-cli-golden/files/input/byron/tx/normal.tx"
+  createdTx <- jotTempFile "tx"


We can ask for a temp file without supplying the tempDir because it can get it from Reader ProjectRoot.

newhoggy · 2024-05-17T15:13:41Z

+compareByronTxs :: ()
+  => HasCallStack
+  => Member (Embed IO) r
+  => Member Hedgehog r


Test functions have a Member Hedgehog r constraint.

newhoggy · 2024-05-17T15:14:47Z

+  => Member Hedgehog r
+  => FilePath
+  -> FilePath
+  -> Sem r ()


Even though getTxByteString is not a test function we can use it seamlessly below.

newhoggy · 2024-05-17T15:16:47Z

+  f & moduleWorkspace "cardano-cli"
+    & runReader (ProjectRoot cabalProjectDir)
+    & runReader (PackagePath "cardano-cli")
+    & runResource


The localWorkspace function sets up all the effects for the tests. For example readers for ProjectRoot and PackagePath are supplied. Additional moduleWorkspace provides a reader for Workspace which is used to create temporary files.

You can have multiple readers - very nice!

newhoggy · 2024-05-17T15:17:34Z

+  => HasCallStack
+  => Member (Embed IO) r
+  => Member Hedgehog r
+  => Member Log r


We can log in test and non-test functions and those will get annotated in the test report.

where does this Log effect come from?

The Log effect comes from polysemy-log.

The interpreters for the Log effect are registered by propertyOnce here:

https://github.com/haskell-works/hw-polysemy/blob/main/hedgehog/HaskellWorks/Polysemy/Hedgehog/Property.hs

See the lines:

& interpretLogDataLog & setLogLevel (Just Info) & interpretDataLogHedgehog formatLogEntry getLogEntryCallStack & interpretDataLogHedgehog id (const GHC.callStack) & interpretTimeGhc

newhoggy · 2024-05-18T11:43:34Z

+  -> Sem r ()
+checkVrfFilePermissions vrfSignKey =
+  embed (runExceptT (Api.checkVrfFilePermissions vrfSignKey))
+    & onLeftM throw


It's very easy to create polysemy wrappers for functions.

newhoggy · 2024-05-18T11:44:00Z

+
+  checkVrfFilePermissions (File vrfSignKey)
+    -- key-gen-VRF cli command created a VRF signing key file with the wrong permissions
+    & trapFail @VRFPrivateKeyFilePermissionError


Error handling in tests is much more succinct. If an error is thrown, we just trap it here and fail with minimal ceremony. Error handling supports multiple errors and it is always clear which error it is that occured.

@@ -0,0 +1,36 @@
+{-# LANGUAGE DataKinds #-}
+{-# LANGUAGE FlexibleContexts #-}
+{-# LANGUAGE OverloadedStrings #-}


+{-# LANGUAGE DataKinds #-}
+{-# LANGUAGE FlexibleContexts #-}
+{-# LANGUAGE OverloadedStrings #-}
+{-# LANGUAGE TypeApplications #-}


+{-# LANGUAGE FlexibleContexts #-}
+{-# LANGUAGE OverloadedStrings #-}
+{-# LANGUAGE TypeApplications #-}
+{-# LANGUAGE TypeOperators #-}


+  (embed $ runExceptT $ Cli.readByronSigningKey bKeyFormat fp)
+    & onLeftM throw


carbolymer · 2024-05-20T19:53:28Z

+  => Member Log r
+  => Member (Embed IO) r
+  => Sem
+        ( Reader Workspace


Where does Workspace type come from?

It is defined here: https://hackage.haskell.org/package/hw-polysemy-0.2.5.0/docs/HaskellWorks-Polysemy-Hedgehog-Workspace.html

carbolymer

Very nice! I love the idea of using of inversion of control for our tests! We should do it for cardano-cli at some point too.

I haven't used polysemy much, so I'm wondering why polysemy and not some other effect system?

Sould we be worried about its peformance? I guess not much, becase it's just for tests, but it's something to be mindful of.

I'm not approving yet, I'm wondering what rest of the team thinks about it. Definitely there's some learning curve here.

smelc · 2024-05-21T12:07:41Z

+
+source-repository-package
+    type: git
+    location: https://github.com/newhoggy/polysemy-conc


Do we need this SRP? I don't see a difference between your fork and @tek's version.

And also I don't see a dependency to polysemy-conc in the modified cabal files!?

We don't use it directly. The SRP is because polysemy-conc is a transitive dependency and it has a dependency that doesn't compile on Windows.

See: tek/polysemy-conc#4

smelc · 2024-05-21T12:33:10Z

+  createdATxAuxBS <- getTxByteString createdTx & trapFail @ByronTxError
+  expectedATxAuxBS <- getTxByteString expectedTx & trapFail @ByronTxError


What happens if we don't write those trapFail (where is this function defined btw? I could not find it)?

I'm worried we're not saving on boilerplate if for every IO call, we have to annotate it with trapFail theExceptionThatItCanThrow.

What trapFail gives to is a quick way to catch the error and annotate it at the point of failure. The failure annotation will attach to the specific trapFail that failed so you can tell which one it was that failed very easily.

It is possible to trapFail once for the entire test, however what you get if you do that is the at the failure annotation will be at the trapFail at the catch-all level not at the operation that failed.

If you don't care for the usefulness of getting the annotation at the point of failure you can reduce boilerplate further.

The trapFail function is defined here:

https://hackage.haskell.org/package/hw-polysemy-0.2.5.0/docs/src/HaskellWorks.Polysemy.Hedgehog.Assert.html#trapFail

The library hw-polysemy is like hedgehog-extras by written using polysemy.

What happens if we don't write those trapFail?

If you don't call trapFail you get a compile error. For example:

test/cardano-cli-golden/Test/Golden/Byron/Tx.hs:74:23: error: [GHC-39999] • Could not deduce ‘Member (Error ByronTxError) r’ arising from a use of ‘getTxByteString’ from the context: (HasCallStack, Member (Embed IO) r, Member Hedgehog r) bound by the type signature for: compareByronTxs :: forall (r :: Polysemy.Internal.Kind.EffectRow). (HasCallStack, Member (Embed IO) r, Member Hedgehog r) => FilePath -> FilePath -> Sem r () at test/cardano-cli-golden/Test/Golden/Byron/Tx.hs:(65,1)-(71,13) • In a stmt of a 'do' block: expectedATxAuxBS <- getTxByteString expectedTx In the expression: do createdATxAuxBS <- getTxByteString createdTx & trapFail @ByronTxError expectedATxAuxBS <- getTxByteString expectedTx normalByronTxToGenTx expectedATxAuxBS === normalByronTxToGenTx createdATxAuxBS In an equation for ‘compareByronTxs’: compareByronTxs createdTx expectedTx = do createdATxAuxBS <- getTxByteString createdTx & trapFail @ByronTxError expectedATxAuxBS <- getTxByteString expectedTx normalByronTxToGenTx expectedATxAuxBS === normalByronTxToGenTx createdATxAuxBS | 74 | expectedATxAuxBS <- getTxByteString expectedTx | ^^^^^^^^^^^^^^^

If you get an error that says Could not deduce ‘Member (Error MyError) r’, you have two choices. You can handle it right there use trap, which is like catch, or trapFail which is like trap by fails immediately with a hedgehog annotation or you can propagate the error by adding Member (Error MyError) r to the function signature in which case you punt the responsibility of handling the error to the caller.

Note that a function can declare that it throws multiple errors so you don't have to create a new sum type with many constructors to propagate multiple errors.

smelc · 2024-05-21T12:37:44Z

In the current state, I am neutral:

We don't really save on boilerplate
We don't add boilerplate either
The multiple reader context is nice to have, but not essential.
If we introduce a dependency to https://github.com/tek/polysemy-conc (unclear if we do really), it's not great; because this is a library with very few users (even if, I must say, I am the manger of the library's author @tek 🤣 who works at Modus Create)

newhoggy · 2024-05-25T12:03:27Z

One advantage is that you could log in the functions being tested and then capture those logs in a test annotation like this:

           ┏━━ test/cardano-cli-golden/Test/Golden/Byron/SigningKeys.hs ━━━
        38 ┃ hprop_deserialise_nonLegacy_signing_Key :: Property
        39 ┃ hprop_deserialise_nonLegacy_signing_Key = propertyOnce $ localWorkspace $ do
        40 ┃   skeyBs <- LBS.readFile "test/cardano-cli-golden/files/input/byron/keys/byron.skey1"
           ┃   │ [info]  [T.G.B.SigningKeys#40] Reading bytestring from file: test/cardano-cli-golden/files/input/byron/keys/byron.skey1
        41 ┃     & trapFail @IOException
           ┃     ^^^^^^^^^^^^^^^^^^^^^^^
           ┃     │ Expected Right: test/cardano-cli-golden/files/input/byron/keys/byron.skey1: openBinaryFile: does not exist (No such file or directory)
        42 ┃ 
        43 ┃   fromEither (deserialiseFromBytes Crypto.fromCBORXPrv skeyBs)
        44 ┃     & trapFail @DeserialiseFailure
        45 ┃     & void

The line:

           ┃   │ [info]  [T.G.B.SigningKeys#40] Reading bytestring from file: test/cardano-cli-golden/files/input/byron/keys/byron.skey1

Is logging from the readFile function, which is pretty short but we could have logging in a more elaborate function and when we get a failure that logging could give some insight into what the function we are testing isn't behaving as expected.

newhoggy · 2024-05-25T12:20:41Z

so I'm wondering why polysemy and not some other effect system?

Mainly due to familiarity and ecosystem - in particular the logging library polysemy-log. We could select a different effect system but we may need to implement the libraries we want and we should also apart from performance evaluate how good the errors are.

smelc · 2024-05-27T07:35:49Z

All in all, I'm not convinced:

There is no significant reduction of boilerplate
By adding dependencies to https://github.com/tek/polysemy-conc and https://github.com/haskell-works/hw-polysemy we put ourselves in a very niche part of the Haskell ecosystem, and this is not good for long term maintenance. The bus factor here would be very small.

If we are convinced this is important, I would wait a few months to see if this part of the ecosystem gains traction.

github-actions · 2024-08-03T01:51:51Z

This PR is stale because it has been open 45 days with no activity.

github-actions · 2024-10-02T02:01:01Z

This issue was closed because it has been stalled for 60 days with no activity.

newhoggy requested review from Jimbo4350, carbolymer, disassembler, kevinhammond, palas and smelc as code owners May 17, 2024 15:08

newhoggy commented May 17, 2024

View reviewed changes

newhoggy force-pushed the newhoggy/polysemy branch from 2ae181c to 92ff831 Compare May 17, 2024 15:13

newhoggy commented May 17, 2024

View reviewed changes

Comment thread cardano-cli/test/cardano-cli-golden/Test/Golden/Byron/Tx.hs

newhoggy commented May 17, 2024

View reviewed changes

newhoggy force-pushed the newhoggy/polysemy branch 3 times, most recently from 7203b38 to 7805ddc Compare May 18, 2024 11:37

newhoggy commented May 18, 2024

View reviewed changes

newhoggy force-pushed the newhoggy/polysemy branch from 7805ddc to 1107902 Compare May 18, 2024 12:53

github-advanced-security AI found potential problems May 18, 2024

View reviewed changes

carbolymer reviewed May 20, 2024

View reviewed changes

smelc reviewed May 21, 2024

View reviewed changes

newhoggy force-pushed the newhoggy/polysemy branch 2 times, most recently from 5ad1e8c to 8f77407 Compare May 25, 2024 12:22

newhoggy force-pushed the newhoggy/polysemy branch 2 times, most recently from b11d193 to e06c483 Compare May 30, 2024 21:52

newhoggy force-pushed the newhoggy/polysemy branch from e06c483 to 9a46ab0 Compare June 18, 2024 12:37

POC for polysemy

63e629b

newhoggy force-pushed the newhoggy/polysemy branch from 9a46ab0 to 63e629b Compare June 18, 2024 12:49

github-actions Bot added the Stale label Aug 3, 2024

github-actions Bot closed this Oct 2, 2024

		(embed $ runExceptT $ Cli.readByronSigningKey bKeyFormat fp)
		& onLeftM throw

		createdATxAuxBS <- getTxByteString createdTx & trapFail @ByronTxError
		expectedATxAuxBS <- getTxByteString expectedTx & trapFail @ByronTxError

Uh oh!

Conversation

newhoggy commented May 17, 2024

Changelog

Context

How to trust this PR

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy May 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check warning

Check warning

Check warning

Check notice

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carbolymer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy May 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy May 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy May 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smelc commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

newhoggy commented May 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

newhoggy commented May 25, 2024

Uh oh!

smelc commented May 27, 2024

Uh oh!

github-actions Bot commented Aug 3, 2024

Uh oh!

github-actions Bot commented Oct 2, 2024

newhoggy May 18, 2024 •

edited

Loading

carbolymer left a comment •

edited

Loading

newhoggy May 25, 2024 •

edited

Loading

newhoggy May 25, 2024 •

edited

Loading

newhoggy May 25, 2024 •

edited

Loading

smelc commented May 21, 2024 •

edited

Loading

newhoggy commented May 25, 2024 •

edited

Loading