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.

Sign up for GitHub

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

Jump to bottom

Removes unnecessary code #1849

Merged
merged 1 commit into from Mar 16, 2022
Merged

Removes unnecessary code #1849

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 6 additions & 18 deletions Src/FluentAssertions/Primitives/StringEqualityValidator.cs
View file
Open in desktop
Expand Up @@ -44,26 +44,14 @@ private string GetMismatchSegmentForStringsOfDifferentLengths()
{
int indexOfMismatch = Subject.IndexOfFirstMismatch(Expected, comparisonMode);

// If there is no difference it means that either
// * subject starts with expected or
// * expected starts with subject
// If there is no difference it means that expected starts with subject and subject is shorter than expected
if (indexOfMismatch == -1)
{
// If subject is shorter we are sure that expected starts with subject
if (Subject.Length < Expected.Length)
{
// Subject is shorter so we point at its last character.
// We would like to point at next character as it is the real
// index of first mismatch, but we need to point at character existing in
// subject, so the next best thing is the last subject character.
indexOfMismatch = Math.Max(0, Subject.Length - 1);
}
else
{
// If subject is longer we are sure that subject starts with expected
Copy link
Member

Choose a reason for hiding this comment

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

What makes you think this scenario is not necessary anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was red in my VS coverage. I then debugged all corresponding tests, e.g. When_string_start_is_compared_with_string_that_is_longer_it_should_throw() , they all don't go into the else branch. I even added a new test to be sure.
Looking at IndexOfFirstMismatch(), it compares Subject with Expected by iterating over the characters of Subject. So the only scenario for -1 can be if Subject is shorter than Expected (or equal) and up until the end of Subject the characters are the same, then it stops at the end of Subject and doesn't compare the rest. Otherwise, if Subject is longer than Expected, we get the position of the first differing character.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't dug into the logic, but have you tried wrapping your tests in an assertion scope to if the logic handles continuing after the first failure?

The commit/PR adding the logic maybe also has some useful insights.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that @chvollm is right. That branch isn't covered by any tests and I can't seem to find an edge case where it fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I spent quite some time on it but was still a bit unsure, so good to see that you can confirm my observation.

// and we point at first character after expected.
indexOfMismatch = Expected.Length;
}
// Subject is shorter so we point at its last character.
// We would like to point at next character as it is the real
// index of first mismatch, but we need to point at character existing in
// subject, so the next best thing is the last subject character.
indexOfMismatch = Math.Max(0, Subject.Length - 1);
}

return Subject.IndexedSegmentAt(indexOfMismatch);
Expand Down