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

Improve readability of assertion failure messages #838

Merged
merged 10 commits into from
Oct 10, 2024

Conversation

vbreuss
Copy link
Contributor

@vbreuss vbreuss commented Oct 10, 2024

Fixes #764 as also discussed in this comment by

  • Moving the assertion expression at the end
  • Replacing the GetFailureMessage with GetExpectation which returns only the expectation from the BaseAssertCondition
  • Replacing the Passes method with GetResult which returns not only a boolean value, but an AssertionResult which also contains the failure message.

This allows the assertion message to be combined to read like an english sentence, e.g.
instead of

Assert.That(myClass).HasMember(x => x.Nested.Nested.Nested.Number).EqualTo(1)
MyClass.Number:
    Expected: 1
    Received: 123

it would read

Expected myClass MyClass.Number to be equal to 1, but received 123.
At Assert.That(myClass).HasMember(x => x.Nested.Nested.Nested.Number).EqualTo(1)

Also added some tests for correctly throwing exceptions (see #294) where the new output reads e.g.

Expected action to have Message equal to "Fails_For_Some_Other_Reason", but it differs at index 10:
              ↓
   "Fails_For_Exceptions_With_Different_Message"
   "Fails_For_Some_Other_Reason"
              ↑.
At Assert.That(action).ThrowsException.With.Message.EqualTo("Fails_For_Some_Other_Reason", StringCompar...

@vbreuss vbreuss changed the title Simplify assertion messages Improve readability of assertion failure messages Oct 10, 2024
@vbreuss
Copy link
Contributor Author

vbreuss commented Oct 10, 2024

@thomhurst : I don't understand why the pipeline fails.
Locally all unit tests compile and succeed, but on the pipeline the TimeoutTests5 fail 🙈

@thomhurst
Copy link
Owner

@thomhurst : I don't understand why the pipeline fails. Locally all unit tests compile and succeed, but on the pipeline the TimeoutTests5 fail 🙈

It looks like one of the assertions has broken. If you go to the pipeline output, then scroll above the table you can expand each step to see it's full output.

I'm seeing this:

failed MatrixTest(2) (46ms)
    Expected value to be equal to 1
     or
    to be equal to 2
     or
    to be equal to 3, but the received value 2 is different and the received value
  2 is different.
    At Assert.That(value).IsEqualTo(1).Or.IsEqualTo(2).Or.IsEqualTo(3)

Message = message;
}

public static AssertionResult FailIf(Func<bool> isFailed, string message)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to take a func? Looks like it invokes it immediately. Couldn't you just pass in the boolean result directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could probably get rid of it here, but not in the OrFailIf method, as it would then throw, e.g. in

		=> AssertionResult
			.FailIf(
				() => actualValue is null,
				$"{ActualExpression ?? typeof(TActual).Name} is null")
			.OrFailIf(
				() => actualValue.Contains(inner, equalityComparer),
				"it was found in the collection");

because both conditions are always evaluated and not just when all previous conditions didn't fail.
However having different method signatures for FailIf and OrFailIf looks strange to me...


return $"{value[..maxLength]}...";

return $"{value[..maxLength]}...";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for the tab increase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it seems I got somehow tabs mixed within. Will have a general look at my files to use consistently spaces, as in the other files...

@thomhurst
Copy link
Owner

Thanks @vbreuss !

@thomhurst thomhurst merged commit 888c4f7 into thomhurst:main Oct 10, 2024
7 checks passed
@vbreuss vbreuss deleted the topic/simplify-messages-2 branch October 10, 2024 13:18
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.

Improve readability of assertion failure messages
2 participants