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.

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

Add a transcript showing that #5178 is fixed #5263

Closed
wants to merge 1 commit into from

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Aug 2, 2024

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

Fixes #5178.

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

Fixes unisonweb#5178.
@sellout
Copy link
Contributor Author

sellout commented Aug 2, 2024

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

@aryairani
Copy link
Contributor

aryairani commented Aug 3, 2024

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

This comment was marked as outdated.

@aryairani
Copy link
Contributor

Confirmed that it displays okay in ui

image

but not with display

image

@aryairani
Copy link
Contributor

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
```
```

@sellout
Copy link
Contributor Author

sellout commented Aug 4, 2024

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.

@aryairani
Copy link
Contributor

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

@sellout
Copy link
Contributor Author

sellout commented Aug 5, 2024

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
Copy link
Contributor Author

sellout commented Aug 5, 2024

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.

@aryairani
Copy link
Contributor

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.

@hojberg hojberg marked this pull request as draft September 11, 2024 15:52
@hojberg
Copy link
Member

hojberg commented Sep 11, 2024

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

@ChrisPenner
Copy link
Contributor

cc @ChrisPenner if you happen to know why

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

@sellout
Copy link
Contributor Author

sellout commented Nov 4, 2024

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

@sellout sellout closed this Nov 4, 2024
@sellout sellout deleted the fix5178 branch November 4, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@source{Stream.emit} gives the wrong error message
4 participants