[MNG-6535] Improve test coverage of UrlNormalizer. #264
Conversation
Refine the original PR to the test that improves coverage; give it a clearer description of its behaviour.
| public void testNormalizeInputWithNoParentDirectoriesToAscendTo() | ||
| { | ||
| assertEquals("a/../", normalize("a/../")); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@rfscholte So our impl is wrong?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If we normalize URLs, the test for relative paths must fail. URLs are absolute.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
This method could be passed relative URIs, so improve handling rather than doubling down on the existing behaviour. Normalize relative URIs to remove redundant '../'. Also switch to JUnit annotations and drop some boilerplate.
| public void testNormalizeInputWithNoParentDirectoriesToAscendTo() | ||
| { | ||
| assertEquals("a/../", normalize("a/../")); | ||
| } |
There was a problem hiding this comment.
@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.
| @Test | ||
| public void relativeUriReferenceLeftUnaffectedWithNoParentDirectoryToAscendTo() | ||
| { | ||
| assertEquals( "/../", normalize("/../" ) ); |
There was a problem hiding this comment.
Did you test the same with Path?
There was a problem hiding this comment.
Path and URI differ slightly here; I've gone with RFC 3986's Remove Dot Segments and adjusted the code to remove them.
* Adopt RFC 3968's behaviour for traversal past the root * Add a test that this isn't applied to relative URI references
I've opened MNG-6703. |
|
PAsses for me locally, waiting for Jenkins.. |
Refine the original PR (#199) to the test that improves coverage and give it a clearer description of its behaviour.
Although this isn't a major change, it does improve coverage (this case works, but isn't currently tested), and resolves the original ticket.
Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MNG-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MNG-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.