fix: Fix equality for CtCodeSnippet

[slarse]

Fix #4022

This PR fixes equality between CtCodeSnippet instances. Previously, two code snippets would be considered equal regardless of their contained snippet (i.e. the raw string). With this PR, the raw snippet is now also considered part of the equality of code snippets (both statements and expressions).

[algomaster99]

Any reason why you have preferred asserThat over assertEquals?

[Rohitesh-Kumar-Jain]

Any reason why you have preferred asserThat over assertEquals?

what about assertNotEquals(one, two); 😅

[slarse]

Any reason why you have preferred asserThat over assertEquals?

Many reasons. First, I prefer the way you write the Hamcrest matchers, i.e. assertThat(one, not(equalTo(two))). It's clear which is the actual value, and which is the expected one. For assertNotEquals(one, two), not so much (these are actually reversed, because it's assertNotEquals(expected, actual), but assertThat(actual, someMatcher(expected))).

Hamcerst matchers also almost always give more informative failure information than JUnit's built-in matchers when an assertion fails.

There's however a pretty big problem with this test that is unrelated to the assertion being used. Why is one the actual value, and two the expected value? Which method is actually under test here? Of course, neither of one or two is actual or expected, they are just two values. The method under test is equals, but that's hidden by the assertion.

The test should probably look like this instead:

Suggested change

	assertThat(one, not(equalTo(two)));
	boolean snippetsAreEqual = one.equals(two);
	assertFalse(snippetsAreEqual);

[slarse]

Or more concisely, this would also be fine

Suggested change

	assertThat(one, not(equalTo(two)));
	assertFalse(one.equals(two));

Asserting true and false is probably the one place where I often use the built-in assertions to Hamcrest matchers, because the Hamcrest alternative is a bit more verbose than necessary:

Suggested change

	assertThat(one, not(equalTo(two)));
	assertThat(one.equals(two), is(false));

However, the Hamcrest failure message:

java.lang.AssertionError: 
Expected: is <false>
     but: was <true>
[... stacktrace ...]

Is much more readable than the JUnit failure message, which is essentially just the stacktrace:

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertFalse(Assert.java:65)
	at org.junit.Assert.assertFalse(Assert.java:75)
	at spoon.test.snippets.SnippetTest.testSnippetsWithDifferingValuesAreNotEqual(SnippetTest.java:255)
[... more stacktrace ...]

So actually I'll probably go with the Hamcrest one after all :)

[algomaster99]

java.lang.AssertionError:
Expected: is
but: was
[... stacktrace ...]

Agree, this is clearly better.

[slarse]

@monperrus @nharrand ping for merge :)

[monperrus]

Thanks a lot @slarse