[MINOR][SQL] Use resource path for test_script.sh #15246

weiqingy · 2016-09-26T21:26:51Z

What changes were proposed in this pull request?

This PR modified the test case test("script") to use resource path for test_script.sh. Make the test case portable (even in IntelliJ).

How was this patch tested?

Passed the test case.
Before:
Run test("script") in IntelliJ:

Caused by: org.apache.spark.SparkException: Subprocess exited with status 127. Error: bash: src/test/resources/test_script.sh: No such file or directory

After:
Test passed.

SparkQA · 2016-09-26T23:08:48Z

Test build #65933 has finished for PR 15246 at commit d799eea.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-09-27T05:37:41Z

Hm, I see why this happens to work, because the file is not packaged inside a jar file. Normally that's what getResource is for, and if it were in a jar this wouldn't work. It's not a bad idea though isn't it easier to just set the working dir for the project in your IDE? IJ can do that.

weiqingy · 2016-09-27T21:41:23Z

Hi, @srowen Thanks a lot for the comments. Yes, setting the working dir can work. However, working dir varies from machine to machine. It would be a little tricky to maintain and troubleshoot in the future. Configuration settings of IDE is not managed and version controlled now. So I think it will be better to make the test case independent with the IDE settings.

srowen · 2016-09-28T13:22:03Z

It's a fair point, though I'm also wondering -- aren't there many other instances of this throughout the tests? in some cases the file would happen to get put on a classpath by the IDE but not always. Is it possible to fix more, or all such issues the same way?

weiqingy · 2016-09-29T00:57:51Z

Hi, @srowen Yes, you are right. I am searching the code base to see if we can fix more.

weiqingy · 2016-10-04T04:13:22Z

I have searched src/test/resourcein the code base to get test cases which hard code src/test/resources. Except for two ignore tests in SQLQuerySuite, for those can not pass in IDE, they are modified to use resource path instead.

SparkQA · 2016-10-04T06:24:59Z

Test build #66300 has finished for PR 15246 at commit 6129187.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

weiqingy · 2016-10-04T06:35:53Z

Hi, @srowen Could you please review this again? Thanks.

srowen · 2016-10-04T09:41:18Z

I wonder if you could make a utility method for this in SparkFunSuite ? and then I think there are two more files that load a file from src/test/resources, in the hive-thriftserver module, as CliSuite and HiveThriftserver2Suites. I think those might be able to use a similar treatment.

I suppose it's fine to assume that whatever is in src/test/resources would be on the test classpath at runtime because Maven would always put it there, yes.

weiqingy · 2016-10-04T21:08:47Z

Hi, @srowen Thanks for the suggestion. Yes, I have updated the PR to add utility methods for this in SparkFunSuite and include CliSuite and HiveThriftserver2Suites. The reason CliSuite and HiveThriftserver2Suites are missed in my previouse PR is their default working dir in my IDE is /Users/wyang/WorkingSpace/sparkrepo/sql/catalyst, and their test cases does not have this issue.

SparkQA · 2016-10-04T22:50:55Z

Test build #66337 has finished for PR 15246 at commit cf161f7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

weiqingy · 2016-10-04T23:37:33Z

At commit 2ea3eea, it reverted the changes in CliSuite and HiveThriftServer2Suites since their target files are not on the their test classpath ( e.g. in the test("Commands using SerDe provided in --jars") of CliSuite, ../hive/src/test/resources/hive-hcatalog-core-0.13.1.jar is actually /Users/wyang/WorkingSpace/sparkrepo/sql/hive/src/test/resources/hive-hcatalog-core-0.13.1.jar)

SparkQA · 2016-10-05T01:16:52Z

Test build #66344 has finished for PR 15246 at commit 2ea3eea.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

weiqingy · 2016-10-05T01:49:02Z

Not sure why org.apache.spark.streaming.kafka010.DirectKafkaStreamSuite.pattern based subscription failed. Looking into it.

weiqingy · 2016-10-05T03:47:41Z

The changes should be safe to org.apache.spark.streaming.kafka010.DirectKafkaStreamSuite.pattern based subscription, I'll re-trigger again.

weiqingy · 2016-10-05T03:47:59Z

Retest this please.

weiqingy · 2016-10-05T06:38:35Z

Hi, @srowen Could you please re-trigger jenkins to retest this? I think the the failure of org.apache.spark.streaming.kafka010.DirectKafkaStreamSuite.pattern based subscription is not caused by the change of this PR. Thank you very much.

srowen · 2016-10-05T13:00:34Z

Jenkins add to whitelist

srowen · 2016-10-05T13:00:39Z

Jenkins retest this please

SparkQA · 2016-10-05T14:51:50Z

Test build #66390 has finished for PR 15246 at commit 2ea3eea.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

weiqingy · 2016-10-05T22:21:00Z

org.apache.spark.streaming.kafka010.DirectKafkaStreamSuite.pattern based subscription failed again, but it passed in my laptop. I still think the failure is not related to this PR. @srowen Could you please give some suggestion? Thanks.

srowen · 2016-10-06T09:38:34Z

core/src/test/scala/org/apache/spark/SparkFunSuite.scala

@@ -41,6 +43,15 @@ abstract class SparkFunSuite
    }
  }

+  // helper function
+  protected final def getFile(file: String): File = {


I like what you've done here with getCanonicalPath. Does this call here need getCanonicalFile for good measure? I supposes there's also something like Paths.get(... toURI).toFile but I don't know if that acts any differently.

How about calling these getTestResourceFile and getTestResourcePath?

@srowen getTestResourceFile and getTestResourcePath look better. Thanks.

URL class doesn't have a method like getCanonicalFile. It has getFile only.

Also, I tested Paths.get(... toURI).toFile. The only difference I noticed is that it keeps spaces as usual, but getFile(file).getCanonicalPath converts spaces to "%20". I suppose they are both OK.

srowen · 2016-10-06T09:40:44Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

@@ -17,6 +17,7 @@

 package org.apache.spark.sql.hive.execution

+import java.io.File


Is this used? maybe I'm missing it

No. Will delete it.

srowen · 2016-10-06T09:41:38Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala

    if (testCommandAvailable("bash") && testCommandAvailable("echo | sed")) {
      val df = Seq(("x1", "y1", "z1"), ("x2", "y2", "z2")).toDF("c1", "c2", "c3")
      df.createOrReplaceTempView("script_table")
      val query1 = sql(
-        """
+        s"""


I was wondering whether string interpolation and triple quotes work together, and I think they do except for some odd corner cases: http://stackoverflow.com/questions/25632924/whats-the-difference-between-raw-string-interpolation-and-triple-quotes-in-scal It's probably OK here but made me double-check.

Yes. Good catch. There are some odd corner cases for s""" """, but it should be OK here.

SparkQA · 2016-10-07T05:27:31Z

Test build #66482 has finished for PR 15246 at commit 1233aa2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

weiqingy · 2016-10-07T05:44:39Z

Hi, @srowen all tests passed this time. Could you please review this PR again? Thanks.

weiqingy · 2016-10-07T17:23:08Z

Thank you for approving, @srowen

srowen · 2016-10-08T11:12:51Z

Merged to master

## What changes were proposed in this pull request? This PR modified the test case `test("script")` to use resource path for `test_script.sh`. Make the test case portable (even in IntelliJ). ## How was this patch tested? Passed the test case. Before: Run `test("script")` in IntelliJ: ``` Caused by: org.apache.spark.SparkException: Subprocess exited with status 127. Error: bash: src/test/resources/test_script.sh: No such file or directory ``` After: Test passed. Author: Weiqing Yang <yangweiqing001@gmail.com> Closes apache#15246 from weiqingy/hivetest.

weiqingy force-pushed the hivetest branch from 2974892 to 2ea3eea Compare October 4, 2016 23:25

weiqingy closed this Oct 5, 2016

weiqingy reopened this Oct 5, 2016

weiqingy added 4 commits October 5, 2016 13:34

[MINOR][SQL] Use resource path for test_script.sh

b40d110

fix more test cases to use resource path'

76d3aa6

add utility methods in SparkFunSuite

9324ba3

revert changes in CliSuite and HiveThriftServer2Suites

e7990fe

srowen requested changes Oct 6, 2016

View reviewed changes

rename methods and delete unused import

1233aa2

weiqingy force-pushed the hivetest branch from 2ea3eea to 1233aa2 Compare October 7, 2016 03:08

srowen approved these changes Oct 7, 2016

View reviewed changes

asfgit closed this in 8a6bbe0 Oct 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MINOR][SQL] Use resource path for test_script.sh #15246

[MINOR][SQL] Use resource path for test_script.sh #15246

weiqingy commented Sep 26, 2016

SparkQA commented Sep 26, 2016

srowen commented Sep 27, 2016

weiqingy commented Sep 27, 2016

srowen commented Sep 28, 2016

weiqingy commented Sep 29, 2016

weiqingy commented Oct 4, 2016

SparkQA commented Oct 4, 2016

weiqingy commented Oct 4, 2016

srowen commented Oct 4, 2016

weiqingy commented Oct 4, 2016

SparkQA commented Oct 4, 2016

weiqingy commented Oct 4, 2016

SparkQA commented Oct 5, 2016

weiqingy commented Oct 5, 2016 •

edited

weiqingy commented Oct 5, 2016

weiqingy commented Oct 5, 2016

weiqingy commented Oct 5, 2016

srowen commented Oct 5, 2016

srowen commented Oct 5, 2016

SparkQA commented Oct 5, 2016

weiqingy commented Oct 5, 2016 •

edited

srowen Oct 6, 2016

weiqingy Oct 7, 2016

srowen Oct 6, 2016

weiqingy Oct 7, 2016

srowen Oct 6, 2016

weiqingy Oct 7, 2016

SparkQA commented Oct 7, 2016

weiqingy commented Oct 7, 2016

weiqingy commented Oct 7, 2016

srowen commented Oct 8, 2016

		@@ -17,6 +17,7 @@

		package org.apache.spark.sql.hive.execution

		import java.io.File

[MINOR][SQL] Use resource path for test_script.sh #15246

[MINOR][SQL] Use resource path for test_script.sh #15246

Conversation

weiqingy commented Sep 26, 2016

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Sep 26, 2016

srowen commented Sep 27, 2016

weiqingy commented Sep 27, 2016

srowen commented Sep 28, 2016

weiqingy commented Sep 29, 2016

weiqingy commented Oct 4, 2016

SparkQA commented Oct 4, 2016

weiqingy commented Oct 4, 2016

srowen commented Oct 4, 2016

weiqingy commented Oct 4, 2016

SparkQA commented Oct 4, 2016

weiqingy commented Oct 4, 2016

SparkQA commented Oct 5, 2016

weiqingy commented Oct 5, 2016 • edited

weiqingy commented Oct 5, 2016

weiqingy commented Oct 5, 2016

weiqingy commented Oct 5, 2016

srowen commented Oct 5, 2016

srowen commented Oct 5, 2016

SparkQA commented Oct 5, 2016

weiqingy commented Oct 5, 2016 • edited

srowen Oct 6, 2016

Choose a reason for hiding this comment

weiqingy Oct 7, 2016

Choose a reason for hiding this comment

srowen Oct 6, 2016

Choose a reason for hiding this comment

weiqingy Oct 7, 2016

Choose a reason for hiding this comment

srowen Oct 6, 2016

Choose a reason for hiding this comment

weiqingy Oct 7, 2016

Choose a reason for hiding this comment

SparkQA commented Oct 7, 2016

weiqingy commented Oct 7, 2016

weiqingy commented Oct 7, 2016

srowen commented Oct 8, 2016

weiqingy commented Oct 5, 2016 •

edited

weiqingy commented Oct 5, 2016 •

edited