[
https://issues.apache.org/jira/browse/MNG-7714?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17729266#comment-17729266
]
ASF GitHub Bot commented on MNG-7714:
-------------------------------------
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.
> sp < final
> ----------
>
> Key: MNG-7714
> URL: https://issues.apache.org/jira/browse/MNG-7714
> Project: Maven
> Issue Type: Bug
> Reporter: Elliotte Rusty Harold
> Assignee: Elliotte Rusty Harold
> Priority: Major
> Fix For: 4.0.0-alpha-6
>
>
> Ported from a comment on https://issues.apache.org/jira/browse/MNG-7701
> The claim is that sp < final, which if true is incorrect according to spec.
> It is easy to demonstrate that this is not fixed and also not in line with
> the spec, with just this one important example (yes this does break for us):
> $ jbang org.apache.maven:maven-artifact:3.8.6 1.0.final-redhat-0001
> 1.0.sp1-redhat-0001
> Display parameters as parsed by Maven (in canonical form and as a list of
> tokens) and comparison result:
> 1. 1.0.final-redhat-0001 -> 1-redhat-1; tokens: [1, [redhat, [1]]]
> 1.0.final-redhat-0001 < 1.0.sp1-redhat-0001
> 2. 1.0.sp1-redhat-0001 -> 1.0.sp-1-redhat-1; tokens: [1, 0, sp, [1, [redhat,
> [1]]]]
> versus
> $ jbang org.apache.maven:maven-artifact:3.8.7 1.0.final-redhat-0001
> 1.0.sp1-redhat-0001
> Display parameters as parsed by Maven (in canonical form and as a list of
> tokens) and comparison result:
> 1. 1.0.final-redhat-0001 -> 1-redhat-1; tokens: [1, [redhat, [1]]]
> 1.0.final-redhat-0001 > 1.0.sp1-redhat-0001
> 2. 1.0.sp1-redhat-0001 -> 1-sp-1-redhat-1; tokens: [1, [sp, [1, [redhat,
> [1]]]]]
> As you can see, our `sp` release is now ordered after our `final` release
> despite this clear text in the "spec":
> Non-numeric tokens ("qualifiers") have the alphabetical order, except for
> the following tokens which come first in this order: "alpha" < "beta" <
> "milestone" < "rc" = "cr" < "snapshot" < "" = "final" = "ga" < "sp"
> It's clear that this tokenization isn't really correct by any reasonable
> measurement, and breaking large amounts of (our) existing artifacts in the
> wild is definitely not OK.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
