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

Update to FluentAssertions 5.10.2 #35

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

ronaldkroon
Copy link
Contributor

This update reveals a mismatch in escaping new line characters.
Fixed by not replacing new line characters in JSON documents any more.

This update reveals a mismatch in escaping new line characters.
Fixed by not replacing new line characters in JSON documents any more.
@dennisdoomen dennisdoomen requested a review from jnyrup February 11, 2020 14:56
@ronaldkroon
Copy link
Contributor Author

I loved the speed at which my previous PR got merged and published.

@@ -90,9 +90,9 @@ public JTokenAssertions(JToken subject)

var message = $"JSON document {difference?.ToString().Escape(true)}.{Environment.NewLine}" +
$"Actual document{Environment.NewLine}" +
$"{Format(Subject, true).Escape(true)}{Environment.NewLine}" +
$"{Format(Subject, true).Replace("{", "{{").Replace("}", "}}")}{Environment.NewLine}" +
Copy link
Member

Choose a reason for hiding this comment

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

Could you add tests to verify the fixes?
This helps showing what is currently broken and that regressions are not introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just upgrading makes existing tests fail: When_objects_are_not_equivalent_it_should_throw
Because it will print \r\n instead of going to the next line.

Copy link
Member

Choose a reason for hiding this comment

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

Ah... I assume this is due to fluentassertions/fluentassertions#987, which changed between 5.6.0 and 5.7.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds likely.

Copy link
Contributor Author

@ronaldkroon ronaldkroon Feb 15, 2020

Choose a reason for hiding this comment

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

I see that FA.json has a copy of the Escape method, which is different since the PR you mentioned. We could also align them, but that will effect other usages (for the good or the bad).

@@ -12,7 +12,7 @@
<AllowUnsafeBlocks>false</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="FluentAssertions" Version="5.0.0" />
<PackageReference Include="FluentAssertions" Version="5.10.2" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with the upgrade, but out of curiosity, is upgrading the core package part of the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're using the newer version of FluentAssertions in our project, so we see the wrong error messages. Apparently the problem is hidden by FA in the older version.

@ronaldkroon
Copy link
Contributor Author

@jnyrup better?

Copy link
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

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

For the record:
When upgrading FA from 5.6.0 to 5.7.0 #987 changed the way we escape certain characters in failure messages.

This caused these FA.Json tests to fail:

When_objects_are_not_equivalent_it_should_throw
When_properties_contains_curly_braces_BeEquivalentTo_should_not_fail_with_FormatException
When_properties_differ_BeEquivalentTo_should_fail
When_specifying_a_reason_why_object_should_be_equivalent_it_should_use_that_in_the_error_message

@dennisdoomen dennisdoomen merged commit abcb521 into fluentassertions:master Feb 18, 2020
@ronaldkroon ronaldkroon deleted the Update_to_FA_5.10.2 branch February 18, 2020 15:05
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.

3 participants