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

Error messages improvements #14

Merged
merged 38 commits into from
Jun 3, 2020

Conversation

faddiv
Copy link
Collaborator

@faddiv faddiv commented Apr 30, 2020

Hy Kevin,

At last, I'm finished with my error messages improvements. :)
I also put several other improvements in the code. Here is the list:

  • Improved error messages
  • Documentation comments now visible.
  • Merged the methods of ActionResultAssertions's parameterless methods with their counterparts.
  • Extracted FileResultAssertions common method into base class which fixes some method's return type.
  • Both PhysicalFileResultAssertions and VirtualFileResultAssertions have WithFileName. Also they have dedicated: WithPhysicalFile and WithVirtualFile.

Example for "Improved error messages":

[Fact]
public void Test1()
{
	var controller = new HomeController();

	var result = controller.Index();

	result.Should().BeJsonResult()
		.ValueAs<string>();
}

Expected ActionResult to be "JsonResult", but found "ViewResult" -> Expected result to be of type Microsoft.AspNetCore.Mvc.JsonResult but was Microsoft.AspNetCore.Mvc.ViewResult.

Expected "Value" to be of type '"String"' but was '"Int32"' -> Expected JsonResult.Value to be of type System.String but was System.Int32.

  • Documentation comments now visible. Until now these wasn't published with the nuget package, but now there it is:
//
// Summary:
//     Asserts that the subject is an Microsoft.AspNetCore.Mvc.JsonResult.
//
// Parameters:
//   reason:
//     A formatted phrase as is supported by System.String.Format(System.String,System.Object[])
//     explaining why the assertion is needed. If the phrase does not start with the
//     word because, it is prepended automatically.
//
//   reasonArgs:
//     Zero or more objects to format using the placeholders in reason.
[CustomAssertion]
public JsonResultAssertions BeJsonResult(string reason = "", params object[] reasonArgs);

faddiv added 30 commits April 6, 2020 21:36
# Conflicts:
#	src/FluentAssertions.AspNetCore.Mvc/ActionResultAssertions.cs
#	src/FluentAssertions.AspNetCore.Mvc/FailureMessages.Designer.cs
#	src/FluentAssertions.AspNetCore.Mvc/FailureMessages.resx
#	src/FluentAssertions.AspNetCore.Mvc/PhysicalFileResultAssertions.cs
#	src/FluentAssertions.AspNetCore.Mvc/VirtualFileResultAssertions.cs
#	tests/FluentAssertions.AspNetCore.Mvc.Tests/PhysicalFileResultAssertions_Tests.cs
#	tests/FluentAssertions.AspNetCore.Mvc.Tests/VirtualFileResultAssertions_Tests.cs
@faddiv
Copy link
Collaborator Author

faddiv commented Apr 30, 2020

@SoftwareWizard I would like to ask about one of your improvement: The WithAuthenticationProperties method, which is on several Assertions, checks the property by reference not by equivalence. Is this intentional? If yes then these error messages should be improved more.
Also you are welcome to check my work. Is these changes okay for you?

@faddiv
Copy link
Collaborator Author

faddiv commented Apr 30, 2020

Oh I almost forgot:
I also fixed the following bug:

  • reason and reasonArgs wasn't included in every message.

@kevinkuszyk
Copy link
Member

@SoftwareWizard do these changes look ok to you?

Copy link

@SoftwareWizard SoftwareWizard left a comment

Choose a reason for hiding this comment

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

Hi Kevin,
thanks for your improvements.
Looks good so far for me.

@kevinkuszyk
Copy link
Member

@faddiv @SoftwareWizard thanks for this.

I have to admit, I just scanned through the changes, but nothing bad caught my eye.

@kevinkuszyk kevinkuszyk merged commit f818f7f into fluentassertions:master Jun 3, 2020
@kevinkuszyk kevinkuszyk added this to the 3.0.0 milestone Jun 3, 2020
@faddiv faddiv deleted the ErrorMessagesFix branch June 4, 2020 11:16
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.

None yet

3 participants