Add interactive shrink view to conformance-test-viewer #18
Conversation
isovector
left a comment
isovector
left a comment
There was a problem hiding this comment.
I really love the idea, but am not sold on the implementation. In particular, it seems like we should reuse the ShrinkIndex combinators rather than having a parallel implementation in Data.RoseTree.
| '\ESC' -> do | ||
| c2 <- getChar | ||
| c3 <- getChar | ||
| case (c2, c3) of | ||
| ('[', 'A') -> pure ToLeftSibling | ||
| ('[', 'B') -> pure ToRightSibling | ||
| ('[', 'C') -> pure ToFirstChild | ||
| ('[', 'D') -> pure ToParent |
There was a problem hiding this comment.
I guess you're parsing ansi codes for arrows here? But a comment would go a long way!
| | Quit | ||
| deriving (Eq, Show) | ||
|
|
||
| readCommand :: IO Command |
There was a problem hiding this comment.
Can this instead have type
| readCommand :: IO Command | |
| readCommand :: String -> [Command] |
?
Laziness will allow us to stream it off stdin, which then means we can drop the unnecessary IO.
| = NoOp | ||
| | ToParent |
There was a problem hiding this comment.
How about removing this constructor
| = NoOp | |
| | ToParent | |
| = ToParent |
and then use Maybe Command instead? Then we get the whole bevy of Maybe combinators like catMaybes or a foldable instance that automatically ignores the no-ops.
| let allShrinks = makeRoseTreeZipper $ applyRoseTreeWithIndex (,) $ makeRoseTree (\x -> (node x, branches x)) $ tree | ||
| interactWithShrinks allShrinks opts Nothing | ||
|
|
||
| interactWithShrinks |
There was a problem hiding this comment.
This function is really, really cool!
| ToParent -> case toParent zipper of | ||
| Just zipper' -> interactWithShrinks zipper' opts Nothing | ||
| Nothing -> interactWithShrinks zipper opts $ Just "already at the root!" |
There was a problem hiding this comment.
This pattern is crying out for a helper.
withZipper :: (Zipper -> Maybe Zipper) -> String -> Zipper -> ...and then:
| ToParent -> case toParent zipper of | |
| Just zipper' -> interactWithShrinks zipper' opts Nothing | |
| Nothing -> interactWithShrinks zipper opts $ Just "already at the root!" | |
| ToParent -> withZipper toParent "already at the root" zipper |
| interactWithShrinks allShrinks opts Nothing | ||
|
|
||
| interactWithShrinks | ||
| :: (Show a, ToExpr a) => RoseTreeZipper ([Int], a) -> Options -> Maybe String -> IO () |
There was a problem hiding this comment.
I think this function can/should be decomposed into three pieces:
- folding
Commands into state updates on the zipper - generating output
- some IO plumbing to make it all work
As a general rule, the more code you can move outside of IO the better. Which makes this a prime candidate.
|
|
||
|
|
||
|
|
||
| data RoseTree a = RoseTree |
There was a problem hiding this comment.
This is identical to our ShrinkTree!
| makeRoseTree decompose x = case decompose x of | ||
| (a, bs) -> RoseTree a $ fmap (makeRoseTree decompose) bs |
There was a problem hiding this comment.
My interpretation of this can't-fail unipattern match is to force the decompose x thunk? If that's on purpose, please document what's going on. If not, I think a let binding would be more idiomatic (and thus avoid me wondering).
| } deriving (Eq, Show, Functor, Foldable, Traversable) | ||
|
|
||
| data RoseTreeZipper a | ||
| = RoseTreeZipper (RoseTree a) [RoseTreeCtx a] (Maybe a) |
There was a problem hiding this comment.
Please add some per-field haddock (and maybe record names) to document what each of these is.
| {-# LANGUAGE RankNTypes #-} | ||
| {-# LANGUAGE ScopedTypeVariables #-} | ||
|
|
||
| module Data.RoseTree where |
There was a problem hiding this comment.
I'm wondering what this module buys us. It seems like we can already do all of these operations via algebra on the ShrinkIndex, and manifest the results via lookup. IMO such would be much more preferable, since it would reuse the logic.
If your concerns are repeatedly paying the lookup costs, we could use MemoTrie to automatically memoize it.
ninioArtillero
left a comment
ninioArtillero
left a comment
There was a problem hiding this comment.
I like this new interactive mode. A neat way of exploring a shrinking strategy :) I approve this, as the few concerns I had were already pointed out in @isovector's review: primary wondering that would be better to use the existing shrink index machinery, but I suspect it could be argued someway (which I'm open to).
|
🎉 All dependencies have been resolved ! |
Description
Addresses tweag/cardano-conformance-testing-of-consensus#61.
Depends on #17.
This PR adds a new option
--interactiveto theconformance-test-viewertool. When present, the tool enters a primitive REPL that does the following:q.The input must be supplied by flag in this mode, since we need to use stdin to read online input.
Here is an example. Suppose the file
test.jsoncontains"hello world". An interactive session where I press Right, Down, Down, Right, Down looks like this:Diffing is supplied by the
ToExprclass fromquickcheck-state-machine, which can be derived generically.Checklist
See Runnings tests for more details
CHANGELOG.mdfor affected package.cabalfiles are updatedhlint. See.github/workflows/check-hlint.ymlto get thehlintversionstylish-haskell. See.github/workflows/stylish-haskell.ymlto get thestylish-haskellversionghc-9.6andghc-9.12Note 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.