elharo commented on code in PR #1099:
URL: https://github.com/apache/maven/pull/1099#discussion_r1217854060
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -101,6 +101,13 @@ private void checkVersionsEqual(String v1, String v2) {
assertEquals(c2, c1, "expected " + v2 + ".equals( " + v1 + " )");
}
+ private void checkVersionsEqualOrder(String v1, String v2) {
Review Comment:
The name of this method does not make it obvious what it does. It probably
isn;t needed at all. Inlining it would likely be easier to read.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -101,6 +101,13 @@ private void checkVersionsEqual(String v1, String v2) {
assertEquals(c2, c1, "expected " + v2 + ".equals( " + v1 + " )");
}
+ private void checkVersionsEqualOrder(String v1, String v2) {
+ Comparable c1 = newComparable(v1);
+ Comparable c2 = newComparable(v2);
Review Comment:
Inlining newComparable is probably also a good idea. Reading it now, I don't
know what this method does.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -147,10 +154,10 @@ void testVersionsEqual() {
checkVersionsEqual("1.0.0x", "1-x");
// aliases
- checkVersionsEqual("1ga", "1");
- checkVersionsEqual("1release", "1");
- checkVersionsEqual("1final", "1");
- checkVersionsEqual("1cr", "1rc");
+ checkVersionsEqualOrder("1ga", "1");
+ checkVersionsEqualOrder("1release", "1");
+ checkVersionsEqualOrder("1final", "1");
Review Comment:
What you seem to have done is remove existing tests and replace them with
new ones. Instead, leave the existing tests in place and unchanged. Then. add
new tests in new methods.
We need to be really sure that existing behavior doesn't change unless we
want it to.
##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -147,10 +154,10 @@ void testVersionsEqual() {
checkVersionsEqual("1.0.0x", "1-x");
// aliases
- checkVersionsEqual("1ga", "1");
- checkVersionsEqual("1release", "1");
- checkVersionsEqual("1final", "1");
- checkVersionsEqual("1cr", "1rc");
+ checkVersionsEqualOrder("1ga", "1");
Review Comment:
Does checkVersionsEqual("1ga", "1") now fail? This is important.
If it does, then I need to dig into this much more carefully to understand
what's going on.
If it doesn't, then the code shouldn't change here. It should be line by
line identical.
New cases not previously tested should be in completely new test methods.
related: Existing utility methods should not change. New ones can (but
probably shouldn't) be added.
Assume the existing tests are correct unless you're deliberately changing
behavior, and then call that out. However, do not assume the existing tests are
well written examples of good testing practice that should be imitated. They
aren't. The existing code makes a number of novice mistakes (testing more than
one case in a single method; unclear method names, hiding test behavior in
utility methods) that should not be copied in new code. There is no benefit to
being consistent with bad practices.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
