Fixes a FormatException when a bad string is included in the 'because' clause

[bernard-chen]

IMPORTANT

SINCE FLUENT ASSERTIONS IS CURRENTLY UNDER HEAVY REFACTORING FOR VERSION 5.0, WE CURRENTLY ONLY ACCEPT BUGFIX REQUESTS. SORRY FOR THE INCONVENIENCE

• The Pull Request is targeted at the master branch.
• The code complies with the Coding Guidelines for C# 3.0, 4.0 and 5.0/.
• The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
• If the contribution affects the documentation, please include your changes to documentation.md in this pull request so the documentation will appear on the website.

Fixes #756.

[jnyrup]

Thanks for having a look into this.

I'm a bit hesitant in trying to fix stuff, that throws a System.FormatException.
E.g. 1.Should().Be(2, "{{{{}}}}"); still fails with a FormatException.

In #717 the summary notes for BecauseOf was changed to mention, that it could throw a FormatException if the string is not compatible with string.Format.

[bernard-chen]

@jnyrup I think you're right. We're not going to protect against all inputs that will fail with string.Format.

Should anything be done to make it clearer why there was a FormatException? In FailWith, one could separate out the invocation of the reason and rethrow the FormatException with a bit more context that it was the reason that was failing to format (as opposed to anything else). If you look at the callstack in the original post of the issue, it's not clear what is failing to format.

This is the code being passed to MessageBuilder.Build:

reason != null ? reason() : ""

[jnyrup]

For the sake of insight, we changed reason to only evaluated on test failure, to avoid a potentially expensive string.Format, e.g. when formatting large collections.

But I agree that a test shouldn't fail, because the because throws an exception.
I would prefer if we changed the Func in BecauseOf to wrap string.Format in a try/catch and provided a meaningful fallback message if string.Format throws a FormatException including the stack trace.

[bernard-chen]

I can add a try/catch to the Func, but are you saying that the result of evaluating the reason would be a string that says something like "because of a reason that could not be formatted with string.Format"? I would think we would still want to throw a FormatException so that the behavior matches the summary notes of #717, but maybe rethrow it with another FormatException that says that it was the reason that could not be formatted.

[dennisdoomen]

Something like that could work

[jnyrup]

To reformulate myself.
(because what I wrote earlier was not specific enough)

A test will only fail from bad because string, if:

• the assertion was wrapped inside an Action,
• that assertion fails,
• BecauseOf throws a FormatException,
• and you asserted an exception different from FormatException on the Action.

The notes in #717 was written before #725 was merged in.
Maybe @krajek has a different view on this topic now?

If we go with a fallback reason, I was thinking about something like **WARNING** because message could not be formatted with string.Format\r\n[StackTrace].

[krajek]

It still seems to me that changing anything in formatting grammar is harmful. Standard formatting grammar is well documented and understood, let's stick to it.

On the other hand, I really like the idea of enriching assertion message instead of throwing FormatException to the caller. Even if the reason part is flawed, part of the assertion message would still be rendered, that's desirable property.
Just repharsing, for clarity. I suggest:

• try/catch the reason evaluation
• even if reason throws, never let the exception out
• display some simple message in the reason part, like it was suggested
• remove the note about possibiliy of throwing FormatException from documentation(basically revert Explicitly mention that "because" formatting can throw FormatException #717)

@jnyrup what do you think? If I understood you correctly you suggested still throwing FormatException. How is the idea of completely containing the exception and using only assertion message works for you?

[jnyrup]

@krajek I agree with everything you just suggested.

[bernard-chen]

Let me take a pass at this. I'll undo the changes I proposed in this PR in favor of what I think we're now talking about.

[bernard-chen]

I made the change proposed by @krajek, except that I also included the original because parameter in the warning message.

[bernard-chen]

If becauseArgs is empty, should we skip the string.Format call?

[krajek]

Tiny issue: you are reproducing the logic from production code in tests. I suggest avoiding $ formatting and just prepare expected message manually.

[bernard-chen]

I embedded the text directly in the expected message with the most recent commit.

[krajek]

Good job, nice improvement.

Regarding empty becauseArgs message: that is an interesting question but best answered in other PR or issue.

[jnyrup]

For the sake of platform independence, please use {Environment.NewLine} instead of `\r\n.

[bernard-chen]

I just pushed a commit with this change.

[jnyrup]

Looks good to me.

[dennisdoomen]

Looks good to me too. Now, to really help us keep the history clean, I would like you to update the PR so it only includes the final implementation. You can do that by using an interactive rebase and skipping the first commits or by creating a new branch, cherry picking the right commits and force pushing it into the one that is linked to the current PR. Would that work for you?

[bernard-chen]

I did the rebase. Please let me know if the commit history and the file changes look like you expect.

[dennisdoomen]

@bernard-chen congrats with your first contribution to Fluent Assertions