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

Exception thrown by argument constraint factory is wrapped in TargetInvocationException, not UserCallbackException #1646

Closed
blairconrad opened this issue Oct 4, 2019 · 6 comments · Fixed by #1647
Assignees
Labels
Milestone

Comments

@blairconrad
Copy link
Member

No description provided.

@thomaslevesque
Copy link
Member

@blairconrad have you started working on this already? I hadn't noticed you had assigned it to yourself, and I just implemented a fix...

@thomaslevesque
Copy link
Member

Well, I almost implemented a fix... I think the error message needs some work!

FluentAssertions.Execution.AssertionFailedException : Expected string to be 
"Argument constraint expression <constraintFactory()> threw an exception. See inner exception for details." with a length of 105, but 
"Argument constraint expression <Invoke(value(FakeItEasy.Specs.UserCallbackExceptionSpecs+<>c__DisplayClass2_0).constraintFactory)> threw an exception. See inner exception for details." has a length of 183.

@thomaslevesque
Copy link
Member

OK, that works better if the constraint factory is a method rather than a delegate.
Do you think we should try to "clean up" the expression to have constraintFactory() in the message instead of Invoke(value(FakeItEasy.Specs.UserCallbackExceptionSpecs+<>c__DisplayClass2_0).constraintFactory) when a delegate is used?

@blairconrad
Copy link
Member Author

blairconrad commented Oct 5, 2019

Hi! No, I'm not working on it. Feel free.

I hadn't given much thought to the error message.
We don't provide a huge amount of detail in most cases where we throw a UserCallbackException:
image

If cleaning up the expression is at all tricky, I'd maybe just use a semi-generic message like those examples. The inner exception will provide additional context if needed, no?

@thomaslevesque
Copy link
Member

No, I'm not working on it. Feel free.

Cool! I pushed my branch.

We don't provide a huge amount of detail in most cases where we throw a UserCallbackException:

No, but when we can provide a representation of the callback that threw, we do. In this case it's an expression, so we can just ToString() it.

If cleaning up the expression is at all tricky, I'd maybe just use a semi-generic message like those examples. The inner exception will provide additional context if needed, no?

The inner exception does provide details, but it doesn't say what the faulty expression was. I'm not going to try to improve the expression right now. The problem exists in other places as well, and if we want to do something about it, I think we should do it separately.

@blairconrad blairconrad added this to the vNext milestone Oct 6, 2019
@blairconrad
Copy link
Member Author

This change has been released as part of FakeItEasy 5.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants