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.

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

merged 4 commits into from
Mar 2, 2023

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Feb 25, 2023

Also clean up docs, generics, and comparisons

@elharo elharo requested a review from hboutemy February 25, 2023 07:39
@elharo elharo changed the title [MNG-7700] test some edge case swith lading zeroes [MNG-7700] test some edge cases with leading zeroes Feb 25, 2023
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.

Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

I'd like to merge this so I can proceed on top of it to investigate MNG-7714 which will need some more tests in these files.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Fixup and merge please.

@elharo elharo merged commit b9b6d85 into master Mar 2, 2023
@elharo elharo deleted the 770 branch March 2, 2023 00:37
@slachiewicz
Copy link
Member

please remember to close associated jira

@elharo
Copy link
Contributor Author

elharo commented Apr 15, 2023

See above. That Issue should not be closed because of this PR. JIRA issues and PRs have a many-to-many relationship. A PR can address more than one JIRA issue and a JIRA issue often requires more than one PR to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants