elharo commented on code in PR #1099:
URL: https://github.com/apache/maven/pull/1099#discussion_r1231081124


##########
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:
   OK, I think I understand you now. The behavior does change. The use of 
checkVersonsEquals and checkVersionsEqualOrder hides this though. Consider 
adding a test that the versions are no equalk. 



##########
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:
   Yes, what's unreadable is that I don't know which is greater and which is 
lesser. Or if it's doing something else then I am being confused by the name. 
Perhaps a clearer name would help, but it'ds also OK to just inline it. 



##########
maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java:
##########
@@ -383,4 +390,15 @@ void testMng7644() {
             checkVersionsEqual("2.0." + x, "2.0.0." + x); // previously 
ordered, now equals
         }
     }
+
+    @Test
+    public void testMng7714() {
+        String f = "1.0.final-redhat";
+        String sp1 = "1.0-sp1-redhat";
+        String sp2 = "1.0-sp-1-redhat";
+        String sp3 = "1.0-sp.1-redhat";
+        checkVersionsOrder(f, sp1);

Review Comment:
   only new tests please. Do not change existing tests unless the behavior has 
changed.



-- 
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

Reply via email to