unison icon indicating copy to clipboard operation
unison copied to clipboard

Add a transcript showing that #5178 is fixed

Open sellout opened this issue 1 year ago • 10 comments

@source{some ability member} now shows the source of the ability.

Fixes #5178.

sellout avatar Aug 02 '24 04:08 sellout

Here it is rendered in the UI: Screenshot 2024-08-01 at 22 36 23

sellout avatar Aug 02 '24 04:08 sellout

Sorry for the delay, I'm having some trouble testing this locally. I wanted to just add a > display foo to the transcript, but I'm getting this:

scratch/main> display foo

      -- The name #rfi1v9429f is assigned to the reference
      ShortHash {prefix =
      "rfi1v9429f9qluv533l2iba77aadttilrpmnhljfapfnfa6sru2nr8ibpqvib9nc4s4nb9s1as45upsfqfqe6ivqi2p82b2vd866it8",
      cycle = Nothing, cid = Nothing}, which is missing from the
      codebase.
      Tip: You might need to repair the codebase manually.

Is that expected?

Maybe user error though.

aryairani avatar Aug 03 '24 21:08 aryairani

@sellout I'm starting to think it's not user error, but is it expected for some reason?

aryairani avatar Aug 03 '24 23:08 aryairani

Confirmed that it displays okay in ui

image

but not with display

image

aryairani avatar Aug 04 '24 00:08 aryairani

So, I think it might not be fixed, but the transcript is still good for when we do fix it; we should add the display line though

eg

```unison
foo = {{
@source{Stream.emit}
}}
```

```ucm
scratch/main> add
scratch/main> view foo
scratch/main> display foo
```
```

aryairani avatar Aug 04 '24 00:08 aryairani

Oh, thanks for catching this! From the original issue, I thought the error was happening during typechecking (and maybe it had been?), so I figured even ui was just a sanity check.

display certainly makes more sense for the transcript, but I’m surprised it and ui differ deeply enough for only one to expose this failure.

sellout avatar Aug 04 '24 06:08 sellout

I’m surprised it and ui differ deeply enough for only one to expose this failure.

Me too! cc @ChrisPenner if you happen to know why

aryairani avatar Aug 05 '24 13:08 aryairani

Do we have any way to track “known failures” in transcripts or otherwise?

This is different than “unexpected success”, as “unexpected success” are tests that are supposed to fail, whereas “known failures” are tests that should succeed but involve known bugs (like the one here).

The salient difference being that when I see “unexpected success” my reaction is “What did I break?”, but with “fixed a known failure” my reaction is “Sweet! We already have a test case for this!”

sellout avatar Aug 05 '24 20:08 sellout

scratch/main> display foo

      -- The name #rfi1v9429f is assigned to the reference
      ShortHash {prefix =
      "rfi1v9429f9qluv533l2iba77aadttilrpmnhljfapfnfa6sru2nr8ibpqvib9nc4s4nb9s1as45upsfqfqe6ivqi2p82b2vd866it8",
      cycle = Nothing, cid = Nothing}, which is missing from the
      codebase.
      Tip: You might need to repair the codebase manually.

An interesting thing with this is that display isn’t failing. UCM returns a successful result, but the content displayed is an error message.

sellout avatar Aug 05 '24 20:08 sellout

Do we have any way to track “known failures” in transcripts or otherwise?

This is different than “unexpected success”, as “unexpected success” are tests that are supposed to fail, whereas “known failures” are tests that should succeed but involve known bugs (like the one here).

We could add another directory for it and another mode to the transcripts executable.

The salient difference being that when I see “unexpected success” my reaction is “What did I break?”, but with “fixed a known failure” my reaction is “Sweet! We already have a test case for this!”

Makes sense!

An interesting thing with this is that display isn’t failing. UCM returns a successful result, but the content displayed is an error message.

Agreed, it does seem like this should be an error. I'm guessing that the error is being detected in OutputMessages which is probably an oversight.

aryairani avatar Aug 06 '24 16:08 aryairani

Converted to draft based on the above conversation that #5178 isn't fixed yet.

hojberg avatar Sep 11 '24 15:09 hojberg

cc @ChrisPenner if you happen to know why

I'm afraid I don't have any insights at a glance here 🤔

ChrisPenner avatar Sep 11 '24 18:09 ChrisPenner

Closing, as the known-failure version of this transcript is included in #5394.

sellout avatar Nov 04 '24 18:11 sellout