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-6535] Improve test coverage of UrlNormalizer. #264

Closed
wants to merge 3 commits into from
Closed

[MNG-6535] Improve test coverage of UrlNormalizer. #264

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c6d668
[MNG-6535] Improve test coverage of UrlNormalizer.
josephw May 20, 2019
ad2d94c
[MNG-6535] Switch behaviour on relative URIs to match Path.normalize
josephw Jul 7, 2019
33f1f11
[MNG-6703] Adopt RFC 3968 behaviour
josephw Jul 8, 2019
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
4 changes: 4 additions & 0 deletions maven-model-builder/src/test/java/org/apache/maven/model/path/DefaultUrlNormalizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,8 @@ public void testPreservationOfDoubleSlashes()
normalize( "[fetch=]http://server.org/[push=]ssh://server.org/" ) );
}

public void testNormalizeInputWithNoParentDirectoriesToAscendTo()
{
assertEquals("a/../", normalize("a/../"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the implementation is wrong, but comparing this with Path this is the result:

    assertEquals( "a/../",  Paths.get("a/../").normalize().toString() ); // fails
    assertEquals( "",  Paths.get("a/../").normalize().toString() ); // succeeds

The Path behavior is also what I would expect.

Copy link
Member

Choose a reason for hiding this comment

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

@rfscholte So our impl is wrong?

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 not sure it's worth changing the behaviour; this class is used with absolute URLs (or paths), so this is very much an edge case.

However, I think it's (slightly) valuable to test that this case doesn't cause an error -- perhaps a different test name could make that clear?

Copy link
Member

Choose a reason for hiding this comment

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

If we normalize URLs, the test for relative paths must fail. URLs are absolute.

Copy link
Member

Choose a reason for hiding this comment

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

@josephw Please raise an issue that our impl is wrong. Otherwise we will lose the findings here. Then I will add a comment abour the test referring to the new ticket and merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking again; there's no reason to retain the existing behaviour if it's wrong. The change to match Path is simple enough, and doesn't break the existing tests: I've pushed it.

Ideally, the interface would be clearer about its contract, but this is the least surprising behaviour.

}