Skip to content

[SPARK-20936][CORE]Lack of an important case about the test of resolveURI in UtilsSuite, and add it as needed. #18158

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

Closed
zuotingbing wants to merge 3 commits into from
Closed

[SPARK-20936][CORE]Lack of an important case about the test of resolveURI in UtilsSuite, and add it as needed. #18158

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3764e55
Lack of the most important case about the test of resolveURI in Utils…
May 31, 2017
b9bc87e
identify the cases between resolveURI and resolveURIs
Jun 1, 2017
9455fb2
1. In resolveURIs test,These two cases [assert(Utils.resolveURIs(afte…
Jun 1, 2017
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
21 changes: 10 additions & 11 deletions core/src/test/scala/org/apache/spark/util/UtilsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,19 +461,17 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
def assertResolves(before: String, after: String): Unit = {
// This should test only single paths
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)
Copy link
Member

@srowen srowen Jun 2, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

assert(resolve(after) === after)
// Repeated invocations of resolveURI should yield the same result
assert(resolve(resolve(after)) === after)
assert(resolve(resolve(resolve(after))) === after)
// Also test resolveURIs with single paths
assert(new URI(Utils.resolveURIs(before)) === new URI(after))
assert(new URI(Utils.resolveURIs(after)) === new URI(after))
}
val rawCwd = System.getProperty("user.dir")
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")
Copy link
Member

@srowen srowen May 31, 2017

Choose a reason for hiding this comment

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

There's no explanation of the change you are proposing. Please don't open PRs in this way

Copy link
Author

@zuotingbing zuotingbing May 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

we need to add assert(resolve(before) === after) to check before and after.

Copy link
Member

@HyukjinKwon HyukjinKwon May 31, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

@zuotingbing zuotingbing Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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!

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

@zuotingbing zuotingbing Jun 1, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@HyukjinKwon HyukjinKwon Jun 1, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Author

@zuotingbing zuotingbing Jun 1, 2017

Choose a reason for hiding this comment

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

sorry for my pool english. could we ask more people to review this and get more opinions? @vanzin

Copy link
Author

@zuotingbing zuotingbing Jun 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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")

assertResolves("spark.jar", s"file:$cwd/spark.jar")
assertResolves("spark.jar#app.jar", s"file:$cwd/spark.jar#app.jar")
assertResolves("path to/file.txt", s"file:$cwd/path%20to/file.txt")
Expand All @@ -482,20 +480,19 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
assertResolves("C:\\path to\\file.txt", "file:/C:/path%20to/file.txt")
}
assertResolves("file:/C:/path/to/file.txt", "file:/C:/path/to/file.txt")
assertResolves("file:///C:/path/to/file.txt", "file:/C:/path/to/file.txt")
assertResolves("file:///C:/path/to/file.txt", "file:///C:/path/to/file.txt")
assertResolves("file:/C:/file.txt#alias.txt", "file:/C:/file.txt#alias.txt")
assertResolves("file:foo", s"file:foo")
assertResolves("file:foo:baby", s"file:foo:baby")
assertResolves("file:foo", "file:foo")
assertResolves("file:foo:baby", "file:foo:baby")
}

test("resolveURIs with multiple paths") {
def assertResolves(before: String, after: String): Unit = {
assume(before.split(",").length > 1)
assert(Utils.resolveURIs(before) === after)
assert(Utils.resolveURIs(after) === after)
// Repeated invocations of resolveURIs should yield the same result
def resolve(uri: String): String = Utils.resolveURIs(uri)
assert(resolve(before) === after)
assert(resolve(after) === after)
// Repeated invocations of resolveURIs should yield the same result
assert(resolve(resolve(after)) === after)
assert(resolve(resolve(resolve(after))) === after)
}
Expand All @@ -511,6 +508,8 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
s"hdfs:/jar1,file:/jar2,file:$cwd/jar3,file:/C:/pi.py%23py.pi,file:/C:/path%20to/jar4")
}
assertResolves(",jar1,jar2", s"file:$cwd/jar1,file:$cwd/jar2")
// Also test resolveURIs with single paths
assertResolves("hdfs:/root/spark.jar", "hdfs:/root/spark.jar")
}

test("nonLocalPaths") {
Expand Down