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 reporting of LHS values when error occurs in RHS predicate #1634

Open
wants to merge 1 commit into
base: horizon
Choose a base branch
from

Conversation

dbp
Copy link
Collaborator

@dbp dbp commented Dec 14, 2021

This, at least partly, addresses #1633.

In particular, it adds a new case of TestResult, failure-exn-satisfies-rhs, which is used when it a satisfies test where an exception was trigger in the RHS. In those cases, we have a value on the LHS, so we should show it along with the exception. I didn't see tests of this kind of behavior, but please point me if there is a way to do it. The four cases that will trigger the new behavior are:

check:
  1 satisfies lam(v): raise("error") end
end

check:
  1 satisfies lam(v): raise("error") end because 1
end

check:
  1 violates lam(v): raise("error") end
end

check:
  1 violates lam(v): raise("error") end because 1
end

And the new error message looks like:

Screen Shot 2021-12-14 at 1 58 40 PM

I noticed, as I was doing this, that there may be other cases where there is more information that could be shown, e.g., if an error is triggered by the refinement of an is test, neither the left nor right value is shown, which it seems like they should be:

check:
  1 is%(lam(v1,v2): raise("error") end) 1
end

Of course, there are still more variations -- e.g., if the right side of an is test errors, the left side value could be shown, etc. Of course, maybe there is a way to make this visible in the stack trace which would subsume all of this? Though with the stack manipulation that's going on, perhaps that is expecting too much.

@blerner
Copy link
Member

blerner commented May 1, 2024

Trying to reconstruct the original design logic: I think we'd thought that "if a test case unexpectedly errors, that itself is a bug and is the most salient issue to present", and didn't really consider the case where a weird LHS input could cause a logic error in the RHS and trigger an unexpected error. I think we might have to workshop the rendering of these error a bit -- your proposed rendering above is good in that the first thing you see is the red unexpected-error box, and the "given the LHS" part is secondary and less salient.

Given that design logic, I'm less keen on making lhs is rhs-that-errors render the lhs value, since there's no possible meaningful scenario where rhs-that-errors should error in an is test! @jpolitz agreed?

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.

2 participants