DNM: messy test to try to find undo bug; only found some other bugs

[brprice]

I finally hacked together the test I have mentioned to try to flush out any undo bugs: "run a bunch of actions, then ensure can undo at the end". This did not uncover any bugs. Thus I conclude that either

• the test is inadequate: perhaps some interaction (other than taking an action listed in the test) can tick the fresh counter but not alter the log, thus when a future action is taken it has a higher ID than if the "silent" action was not taken, thus breaking replays (i.e. later undos)
• or @georgefst's hunch that the only bug is related to migrations and will only show up if we change the implementation in such a way as to change generated IDs and then try to "undo" in a saved session that was created before that change.

I am mostly opening this PR so the code is not lost. In the case that this bug does crop up again, this may be a good starting point to find it. I'm happy to do the (fairly minor) clean up to get this into a mergeable state if you think it would be a good addition to the testsuite, but I doubt I'll push for its inclusion myself.

[brprice]

Hmm, actually scrap that! I have found the bug: leaving this running much longer results in a ActionError (CustomFailure Delete "internal error: lost ID after typechecking"), but Hedgehog/Tasty does not reproduce it: the run ends with

              This failure can be reproduced by running:                                                                                                       
              > recheckAt (Seed 16811957402576231177 3671377713982875533) "336:dHa3NcAcAbCaHdAeCaDaGeCa3MaDa" undo redo                                        
                                                                                                                                                               
          Use "--pattern '$NF ~ /undo redo/' --hedgehog-replay '336:dHa3NcAcAbCaHdAeCaDaGeCa3MaDa Seed 16811957402576231177 3671377713982875533'" to reproduce from the command-line.

but using those options just result in ⚐ undo redo gave up after 0 discards, passed 336 tests.

[brprice]

Hmm, actually scrap that! I have found the bug: leaving this running much longer results in a ActionError (CustomFailure Delete "internal error: lost ID after typechecking"), but Hedgehog/Tasty does not reproduce it: the run ends with

              This failure can be reproduced by running:                                                                                                       
              > recheckAt (Seed 16811957402576231177 3671377713982875533) "336:dHa3NcAcAbCaHdAeCaDaGeCa3MaDa" undo redo                                        
                                                                                                                                                               
          Use "--pattern '$NF ~ /undo redo/' --hedgehog-replay '336:dHa3NcAcAbCaHdAeCaDaGeCa3MaDa Seed 16811957402576231177 3671377713982875533'" to reproduce from the command-line.

but using those options just result in ⚐ undo redo gave up after 0 discards, passed 336 tests.

Judging from the log (see https://gist.github.com/brprice/d9e678e6961ebdc0e977248d35cc762e), it appears to do three actions, starting from a somewhat complex program

• make a new type def (not part of the offered actions API, but hardcoded as an option in the test)
• "raise"
• "raise"
  and this last "raise" fails with the "lost an ID" message

[dhess]

That's great news! Any idea what's causing it? (Also, maybe temper "found the bug" with "found a bug"; as far as I recall, I've previously encountered the [rather, an] undo bug without performing a raise operation, though obviously it's possible there's one common root cause.)

[georgefst]

Hedgehog/Tasty does not reproduce it

I think you mentioned yesterday that this is an upstream issue with Hedgehog's last release. I've seen similarly confusing output myself whenever I've tried to use the --hedgehog-replay flag. Did you get any further with investigating this?

[brprice]

Hedgehog/Tasty does not reproduce it

I think you mentioned yesterday that this is an upstream issue with Hedgehog's last release. I've seen similarly confusing output myself whenever I've tried to use the --hedgehog-replay flag. Did you get any further with investigating this?

I have tried to get a minimal broken example, but not yet boiled it down to anything worth submitting upstream. When you got such output was it on a smaller testcase than the "available actions accepted" one? (Preferably not dependent on action code at all)

[georgefst]

When you got such output was it on a smaller testcase than the "available actions accepted" one?

That's the only one I've been using, unfortunately.

[brprice]

I have now submitted the hedgehog issue upstream: hedgehogqa/haskell-hedgehog#487. This was aided by CI finding a failure in a test that does not touch actions, which was much, much easier to minimise! (this ci failure was #972 (comment) / https://buildkite.com/hackworthltd/primer/builds/3339#01882a2d-6f71-4481-9463-4391e75b865c)

[brprice]

I have diagnosed one bug here (I am unsure if there are multiple ones lurking).
However, this bug, similarly to the one in the gist linked earlier actually fails before we test undo, i.e. it is a bug in offered-actions-being-accepted! Thus, this is not actually an undo bug.

Sometimes we find a situation where 71c86a4 "feat: actions handle smart elision of hole/ann better" is incomplete, such as the following minimised example

unit_tmp_5 :: Assertion
unit_tmp_5 =
  let ((builtins, d), i) = create $ do
        m <- builtinModule
        astDefExpr <- hole $ emptyHole `ann` tcon tBool
        astDefType <- tcon tNat
        pure (m,ASTDef
               { astDefExpr
               , astDefType
               })
  in case evalTestM i $ applyActionsToBody SmartHoles [builtins] d [Action.Move Child1, Action.EnterType, Action.Delete] of
    Left err -> assertFailure $ show err
    Right _ -> pure ()

this starts with foo : Nat ; foo = {? ? : Bool ?}, focuses on the Bool and issues Delete which gives foo = {? ? : ? ?} and then smartholes gives foo = ? Note that the ID of the remaining hole is the same as the original empty hole. Our fix in 71c86a4 only deals with what happens if we are focused on an annotation node which is smartholes-elided, and not if we are focussed on the type child of an annotation node!

This particular bug is fixed in #1032, though I note that the test in this PR uncovers some others (seemingly unrelated to undo -- I have no idea why this test seems much better than the existing actions-are-accepted test)

[brprice]

Hmm, I am rather confused about this test. It uncovered some bugs that were fixed in #1032 , but none of these were related to undo! I do not understand why it seems a more effective test than the current "offered actions are accepted" test, though I have not looked into it closely. It may be worth (tidying up and) merging just for the more effective test, even if it does not tickle an undo bug.

[brprice]

Given that we now have understood at least one undo bug #1056 which can only be triggered by doing multiple interleaved edit, eval, undo and redo actions, I think it would be worth getting this test (upgraded? and) merged, as our current test only does one action, starting from a complex generated App

[dhess]

Can this be closed now?

[dhess]

Bumping, can this PR be closed?

[brprice]

Closing in favour of #1167