You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by zuotingbing <gi...@git.apache.org> on 2017/05/31 08:23:26 UTC
[GitHub] spark pull request #18158: Lack of the most important case about the test of...
GitHub user zuotingbing opened a pull request:
https://github.com/apache/spark/pull/18158
Lack of the most important case about the test of resolveURI in UtilsSuite, and add it as needed.
## What changes were proposed in this pull request?
add the most important case in the test of resolveURI.
## How was this patch tested?
unit tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zuotingbing/spark spark-UtilsSuite
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18158.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #18158
----
commit 3764e55c8deb7aa46e74c5e2124d4243549d5e1d
Author: zuotingbing <zu...@zte.com.cn>
Date: 2017-05-31T08:19:41Z
Lack of the most important case about the test of resolveURI in UtilsSuite, and add it as needed.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18158: [SPARK-20936][CORE]Lack of an important case about the t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18158
**[Test build #3771 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3771/testReport)** for PR 18158 at commit [`9455fb2`](https://github.com/apache/spark/commit/9455fb25cad229e329169a44c4591472237223d1).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119534247
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119535585
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
ok, let me identify the cases between resolveURI and resolveURIs. what i meant is we should check `assert(resolve(before) === after)` just in `test("resolveURI")`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: Lack of the most important case about the test of...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119355257
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119858243
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -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)
--- End diff --
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/18158
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18158: [SPARK-20936][CORE]Lack of an important case about the t...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/18158
Merged to master
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18158: [SPARK-20936][CORE]Lack of an important case about the t...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/18158
**[Test build #3771 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3771/testReport)** for PR 18158 at commit [`9455fb2`](https://github.com/apache/spark/commit/9455fb25cad229e329169a44c4591472237223d1).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: Lack of the most important case about the test of...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119302580
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
we`d best to add `assert(resolve(before) === after)` to check before and after.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119859774
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -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)
--- End diff --
Thanks for asking me. The initial change proposed looked testing several duplicated test cases and also changing existing tests to me. I am fine with the current status.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119534690
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
sorry for my pool english. could we ask more people to review this and get more opinions? @vanzin
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: Lack of an important case about the test of resol...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119511454
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
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 param `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"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by zuotingbing <gi...@git.apache.org>.
Github user zuotingbing commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119533271
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
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`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: [SPARK-20936][CORE]Lack of an important case abou...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119523325
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
My question would be, what this test targets. It is probably okay to add missing cases that does not change the existing tests but this changes the existing regression test cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18158: Lack of the most important case about the test of...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/18158#discussion_r119298616
--- Diff: core/src/test/scala/org/apache/spark/util/UtilsSuite.scala ---
@@ -473,7 +474,7 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
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")
--- End diff --
There's no explanation of the change you are proposing. Please don't open PRs in this way
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18158: Lack of the most important case about the test of resolv...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18158
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org