Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MNG-7700] test some edge cases with leading zeroes #1014

Merged
merged 4 commits into from
Mar 2, 2023
Merged

[MNG-7700] test some edge cases with leading zeroes #1014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d33c37d
wip
elharo Feb 25, 2023
7a0fa64
add tests for MNG-7700
elharo Feb 25, 2023
2ec7d3e
format
elharo Feb 25, 2023
300fea1
Merge branch 'master' into 770
elharo Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion maven-artifact/src/main/java/org/apache/maven/artifact/versioning/ComparableVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
* over {@code 1.0.0.X1}.</li>
* </ul>
*
* @see <a href="https://cwiki.apache.org/confluence/display/MAVENOLD/Versioning">"Versioning" on Maven Wiki</a>
* @see <a href="https://maven.apache.org/pom.html#version-order-specification">"Versioning" in the POM reference</a>
* @author <a href="mailto:kenney@apache.org">Kenney Westerhof</a>
* @author <a href="mailto:hboutemy@apache.org">Hervé Boutemy</a>
*/
Expand Down
35 changes: 27 additions & 8 deletions maven-artifact/src/test/java/org/apache/maven/artifact/versioning/ComparableVersionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,11 @@
*/
@SuppressWarnings("unchecked")
public class ComparableVersionTest {
private Comparable newComparable(String version) {
private ComparableVersion newComparable(String version) {
ComparableVersion ret = new ComparableVersion(version);
String canonical = ret.getCanonical();
String parsedCanonical = new ComparableVersion(canonical).getCanonical();

System.out.println("canonical( " + version + " ) = " + canonical);
assertEquals(
canonical,
parsedCanonical,
Expand Down Expand Up @@ -95,11 +94,11 @@ private void checkVersionsOrder(String[] versions) {
private void checkVersionsEqual(String v1, String v2) {
Comparable c1 = newComparable(v1);
Comparable c2 = newComparable(v2);
assertTrue(c1.compareTo(c2) == 0, "expected " + v1 + " == " + v2);
assertTrue(c2.compareTo(c1) == 0, "expected " + v2 + " == " + v1);
assertTrue(c1.hashCode() == c2.hashCode(), "expected same hashcode for " + v1 + " and " + v2);
assertTrue(c1.equals(c2), "expected " + v1 + ".equals( " + v2 + " )");
assertTrue(c2.equals(c1), "expected " + v2 + ".equals( " + v1 + " )");
assertEquals(0, c1.compareTo(c2), "expected " + v1 + " == " + v2);
assertEquals(0, c2.compareTo(c1), "expected " + v2 + " == " + v1);
assertEquals(c1.hashCode(), c2.hashCode(), "expected same hashcode for " + v1 + " and " + v2);
assertEquals(c1, c2, "expected " + v1 + ".equals( " + v2 + " )");
assertEquals(c2, c1, "expected " + v2 + ".equals( " + v1 + " )");
}

private void checkVersionsArrayEqual(String[] array) {
Expand Down Expand Up @@ -209,6 +208,26 @@ public void testVersionComparing() {
checkVersionsOrder("2.0.1-xyz", "2.0.1-123");
}

@Test
public void testLeadingZeroes() {
checkVersionsOrder("0.7", "2");
checkVersionsOrder("0.2", "1.0.7");
}

@Test
public void testGetCanonical() {
// MNG-7700
newComparable("0.x");
newComparable("0-x");
newComparable("0.rc");
newComparable("0-1");

ComparableVersion version = new ComparableVersion("0.x");
assertEquals("x", version.getCanonical());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why this should not be 0 with x as qualifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never define or promise anything about the canonical representation of a ComparableVersion. Indeed, this method probably shouldn't have been public. All we promise is that two of them compare according to spec, and as best I can tell they do. For now this is just a characterization test of existing behavior. I'm leaving MNG-7700 open in case folks feel we should change the canonical representation. However neither that issue nor this PR needs to block the release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depressing...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not really a useful test. You're just testing that the output is what the output is. If you do this instead, you'll see that the "canonical" version is not really a canonical version at all:

ComparableVersion version = new ComparableVersion("0.x");
ComparableVersion canonical = new ComparableVersion(version.getCanonical());
assertEquals(canonical, version); // actually see if the *versions* are equal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm testing that the input produces the output. That is useful. There's a case to be made that the current behavior is wrong, which can be discussed on the bug. If we decide it is and change it, then having this test here will make it clear what changed when.

The test you suggest is also good. However, it's (not so obviously) performed in line 220 by newComparable("0.x") which includes an assert to check that.

ComparableVersion version2 = new ComparableVersion("0.2");
assertEquals("0.2", version2.getCanonical());
}

/**
* Test <a href="https://issues.apache.org/jira/browse/MNG-5568">MNG-5568</a> edge case
* which was showing transitive inconsistency: since A &gt; B and B &gt; C then we should have A &gt; C
Expand Down Expand Up @@ -343,7 +362,7 @@ public void testReuse() {
ComparableVersion c1 = new ComparableVersion("1");
c1.parseVersion("2");

Comparable c2 = newComparable("2");
Comparable<?> c2 = newComparable("2");

assertEquals(c1, c2, "reused instance should be equivalent to new instance");
}
Expand Down