Issue #1432 Asserts for charsequence

[ghost]

Closed Pull Request #1438 opened this one as requested

[kcooney]

Thanks for making the changes.

I pushed another commit to the branch with some minor style fixes, so you'll probably want to pull from your github branch to your local branch before making new commits.

(yes, ideally we should use a plugin like "spotless" to fix and enforce our style, but that hasn't be implemented yet)

[kcooney]

I think we should consider writing it like this:

if (expected == actual) {
  return;
}
if (expected == null || actual == null) {
  failNotEquals(message, expected, actual
}
...

That way, we avoid calling actual.toString() when expected and actual point to the same instance.

That being said, since the method is named "assertContentEquals" (and not "assertEquals") perhaps passing in null for expected should be an error (resulting in a NullPointerException). We don't do that anywhere else in this class, but I thought I'd put the idea out there in case someone might want to argue for it.

[ghost]

Should I make these changes, or are they still open for discussion from others?

[kcooney]

As a potential user of this method, how should it handle null values?

(I assume short-circuiting the method if expected == actual is not controversial)

[IgorNosach25]

Signature of method is not correct. We must have opportunity to compare 2 instances of Char Sequence interface.

[kcooney]

We decided that the expected value should be a string. According to the CharSequence Javadoc "The result of comparing two objects that implement CharSequence is therefore, in general, undefined"

[IgorNosach25]

You are right. I was confused because the question is open. Please close topic if we don't need compare 2 CharSequence instances (or String, because CharSequence doesn't has contract for equals
method ). How I can see actual open issues? Please answer me in private message) thanks

[IgorNosach25]

In my view Junit must has only methods for comparing primitive types and objects by equals. There are tests that fail in JUnit also. It's strange for me. Also there are some explainings why these tests fall.

[kcooney]

Moving the conversation about checking the length first to this pull request...

@sabi0 apparently suggested to check for the lengths to avoid the toString() call. I agree that it wouldn't be that helpful to failing early if the lengths differ with a message that just prints the lengths. We do, however, have one close precedent in JUnit, and that's when comparing arrays. If the lengths are different, we use a message like this:

"<user supplied message>: array lengths differed, expected.length=2 actual.length=1; arrays first differed at element [1]; expected:<false> but was:<end of array>"

(with no user-supplied message, the message starts with "array lengths...")

But I think assertArrayEquals does that because 1) arrays are often large, 2) the printed representation of arrays can be hard to read and 3) visually seeing the difference between two arrays in an error message is difficult. Here, we throw a ComparisonFailure, and the exception message generated by JUnit will show which parts of the string differ (plus IDEs often show a nice UI for ComparisonFailure). None of that applies here, so I think that even stating that the lengths are different in the exception isn't that useful for the method here.

[ghost]

Should I make these changes, or are they still open for discussion from others?

[kcooney]

No one chimed in, so let's not mention the length difference in the exception message. I do think we should optimize for the expected == actual.

[ashleyfrieze]

This could be simplified:

return (message != null ? message + ": " : "") + "Content of different length";

[ashleyfrieze]

Suggestion - why not make the expected a CharSequence too?

[sabi0]

See the original discussion: #1432 (comment)

[sabi0]

... and also here: #1438 (comment)