[SPARK-20936][CORE]Lack of an important case about the test of resolveURI in UtilsSuite, and add it as needed. #18158
Conversation
…Suite, and add it as needed.
| val cwd = if (Utils.isWindows) s"/$rawCwd".replace("\\", "/") else rawCwd | ||
| assertResolves("hdfs:/root/spark.jar", "hdfs:/root/spark.jar") | ||
| assertResolves("hdfs:///root/spark.jar#app.jar", "hdfs:/root/spark.jar#app.jar") | ||
| assertResolves("hdfs:///root/spark.jar#app.jar", "hdfs:///root/spark.jar#app.jar") |
There was a problem hiding this comment.
There's no explanation of the change you are proposing. Please don't open PRs in this way
There was a problem hiding this comment.
we need to add assert(resolve(before) === after) to check before and after.
There was a problem hiding this comment.
Do you mind if I ask to update the PR description with the reason for ^ in more details? We could focus on the topic more easily if there are details. This saves time for all of us (author and reviewers). Also, it helps to understand later when someone revisits this PR
There was a problem hiding this comment.
yes let me make it clear. the function assertResolves(before: String, after: String) have two params, it means we need to check the before value whether equals the after value which we want when we call Utils.resolveURI(uri), right? otherwise the different value of before: String make no sense.
e.g. the after value of Utils.resolveURI("hdfs:///root/spark.jar#app.jar").toString should be "hdfs:///root/spark.jar#app.jar" rather than "hdfs:/root/spark.jar#app.jar". we need assert(resolve(before) === after) to make it more safe.
Thanks!
There was a problem hiding this comment.
My question would be, what this change targets (e.g., adding missing test cases). If this tests the same thing before/after, I wonder if it is worth changing this.
It is probably okay to add missing cases that does not change the existing tests but this changes the existing regression test cases.
Are there missing test cases for this that detect the regressions within Spark's Utils.resolveURI?
There was a problem hiding this comment.
what is the before: String targets in assertResolves(before: String, after: String) ?
It seems to me that the different values of before: String test more different cases of Utils.resolveURI. if we do not use before: String to check, why add the params of before: String
There was a problem hiding this comment.
I think we do use it in assert(new URI(Utils.resolveURIs(before)) === new URI(after)).
different cases of
Utils.resolveURI
I think we should identify the cases.
If the only reason is just meant to make the test code prettier but change the existing tests for no reason, I would go -1.
There was a problem hiding this comment.
sorry for my pool english. could we ask more people to review this and get more opinions? @vanzin
There was a problem hiding this comment.
yes, let me identify the cases between resolveURI and resolveURIs. what i meant is we should check assert(resolve(before) === after) also in test("resolveURI")
zuotingbing
changed the title
zuotingbing
changed the title
…r) === after)] and [assert(resolve(after) === after)] duplicate each other,delete one. 2. little fixs make the suit clear.
|
Test build #3771 has finished for PR 18158 at commit
|
| assume(before.split(",").length === 1) | ||
| // Repeated invocations of resolveURI should yield the same result | ||
| def resolve(uri: String): String = Utils.resolveURI(uri).toString | ||
| assert(resolve(before) === after) |
There was a problem hiding this comment.
I think the change is OK @HyukjinKwon -- we do want to check that before resolves to after and this case seems missed in this fixture. Or do I miss something? The rest of the change is just for consistency
There was a problem hiding this comment.
Thanks for asking me. The initial change proposed looked testing several duplicated test cases and also changing existing test cases to me (I meant it looked changing what it tested before). I am fine with the current status.
|
Merged to master |
4 participants
What changes were proposed in this pull request?
assert(resolve(before) === after)to check before and after in test of resolveURI.the function
assertResolves(before: String, after: String)have two params, it means we should check the before value whether equals the after value which we want.e.g. the after value of Utils.resolveURI("hdfs:///root/spark.jar#app.jar").toString should be "hdfs:///root/spark.jar#app.jar" rather than "hdfs:/root/spark.jar#app.jar". we need
assert(resolve(before) === after)to make it more safe.How was this patch tested?
unit tests