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

Fix multiplied ConversionSelector's descriptions appearing in assertion message #941

Conversation

@krajek
Copy link
Contributor

krajek commented Oct 8, 2018

Refers to #738.
For sure it is a step forward, but I am not certain if it fixes the root cause. I will investigate more, and possibly follow up with another PR. But for the time being...

the problem was the ConversionSelector.Clone method that treated description of type StringBuilder like it was immutable string. Obviously, StringBuilder is mutable and needs to be cloned.

Two main changes:

  • Create new StringBuilder in ConversionSelector.Clone

  • Rename description field to descriptionBuilder because description suggests it is a string indeed.

@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 8, 2018

I have just run all the tests and haven't found a place when duplicated description would appear.
I have used a conditional breakpoint and debugged all the tests.

@dennisdoomen are you aware of MCVE for the #738 that this fix does not cover?

@jnyrup
jnyrup approved these changes Oct 8, 2018
Tests/Shared.Specs/BasicEquivalencySpecs.cs Outdated Show resolved Hide resolved
@krajek krajek changed the title Fix ConversionSelector's cloning of the description [WIP] Fix ConversionSelector's cloning of the description Oct 9, 2018
@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 9, 2018

After @jnyrup comment, I found that I misspelt the assertion in test:
.Should().NotMatch("*Try conversion of all members*", "conversion description should be present");
should be
.Should().**Match**("*Try conversion of all members*", "conversion description should be present");
hence I have a problem to fix.

@dennisdoomen

This comment has been minimized.

Copy link
Member

dennisdoomen commented Oct 9, 2018

are you aware of MCVE for the #738 that this fix does not cover?

No, I just noticed it while working on some other fix.

@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 9, 2018

Thanks.
While debugging I start to see that problem with ConversionSelector.Clone is just the tip of the iceberg ;-).
Work in progress to fixing the root causes.

…ertionOptions.GetUserEquivalencySteps
@krajek krajek changed the title [WIP] Fix ConversionSelector's cloning of the description [WIP] Fix ConversionSelector's mutability Oct 9, 2018
@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 9, 2018

Ok, by now I think I have gotten to the bottom of the issue. The more facts I gathered, the more clear it was that until this PR, the feature was working almost by accident :-).

The most important parts of the code to understand it are:

  • ConversionSelector.Clone() copied the StringBuilder reference instead of cloning. @dennisdoomen found a case when the description was duplicated, but because default static StringBuilder was copied all over the place, the description could be repeated without upped bound. However, cloning would also be incorrect, because of the conditional logic in ConvertionSelector.ToString(). One of the incorrect scenarios would be when you clone ConvertionSelector without rules, and you get "Without automatic conversion.". Next, you add some conversion rule and try to add the description the builder, but "Without automatic conversion." is not erased! The only solution was to make ConvertionSelector delay building the description string until ToString is called.

  • SelfReferenceEquivalencyAssertionOptions constructor that copies from defaults had following line: userEquivalencySteps.AddRange(defaults.UserEquivalencySteps);. Seems logical, but when we go to UserEquivalencySteps implementation we can see that its not a single retrieval of the list but userEquivalencySteps.Concat(new[] { new TryConversionStep(ConversionSelector) });. This code generates convertion step with a current selector. So when it copied from defaults it used defaults.ConvertionSelector instead of using cloned selector. The only solution I found yet is to inject ConvertionSelector

I plan to add some more tests tomorrow, but I could use some review. I hope I did not miss anything important.

Copy link
Collaborator

jnyrup left a comment

I like the idea about grouping predicates and descriptions into ConversionSelectorRule 👍

@krajek krajek changed the title [WIP] Fix ConversionSelector's mutability [WIP] Fix multiplied ConversionSelector's descriptions appearing in assertion message Oct 10, 2018
@krajek

This comment has been minimized.

Copy link
Contributor Author

krajek commented Oct 10, 2018

Ready for merge(with squash).

I have added one test checking the exception message exactly, I hope you won't crucify me for that :-). My intent was to have at least one test that verifies the whole mechanism of gathering descriptions from the equivalency steps.

As it was discussed, the PR introduces minor breaking change in public API.

@krajek krajek changed the title [WIP] Fix multiplied ConversionSelector's descriptions appearing in assertion message Fix multiplied ConversionSelector's descriptions appearing in assertion message Oct 10, 2018
@jnyrup jnyrup merged commit feeb5dd into fluentassertions:master Oct 11, 2018
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@krajek krajek deleted the krajek:multiple-conversion-rules-in-failure-message branch Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.