[GitHub] spark pull request #23151: [SPARK-26180][CORE][TEST] Reuse withTempDir funct...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/23151 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23151: [SPARK-26180][CORE][TEST] Reuse withTempDir funct...
Github user heary-cao commented on a diff in the pull request: https://github.com/apache/spark/pull/23151#discussion_r238049434 --- Diff: core/src/test/scala/org/apache/spark/SparkContextSuite.scala --- @@ -245,8 +245,7 @@ class SparkContextSuite extends SparkFunSuite with LocalSparkContext with Eventu } test("cannot call addFile with different paths that have the same filename") { --- End diff -- ok, i have update it. please review it. thanks. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23151: [SPARK-26180][CORE][TEST] Reuse withTempDir funct...
Github user dongjoon-hyun commented on a diff in the pull request: https://github.com/apache/spark/pull/23151#discussion_r237977599 --- Diff: core/src/test/scala/org/apache/spark/SparkContextSuite.scala --- @@ -245,8 +245,7 @@ class SparkContextSuite extends SparkFunSuite with LocalSparkContext with Eventu } test("cannot call addFile with different paths that have the same filename") { --- End diff -- Hi, @heary-cao . This PR is very useful. Could you replace more `val dir = Utils.createTempDir()` with new `withTempDir` in this file? For example, [the above test case at line 235](https://github.com/apache/spark/pull/23151/files#diff-8d5858d578a2dda1a2edb0d8cefa4f24R235). --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23151: [SPARK-26180][CORE][TEST] Reuse withTempDir funct...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/23151#discussion_r237886340 --- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala --- @@ -105,5 +105,16 @@ abstract class SparkFunSuite logInfo(s"\n\n= FINISHED $shortSuiteName: '$testName' =\n") } } - + /** + * Creates a temporary directory, which is then passed to `f` and will be deleted after `f` + * returns. + * + * @todo Probably this method should be moved to a more general place + */ + protected def withCreateTempDir(f: File => Unit): Unit = { +val dir = Utils.createTempDir() --- End diff -- Yes shouldn't be necessary here. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #23151: [SPARK-26180][CORE][TEST] Reuse withTempDir funct...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/23151#discussion_r237886193 --- Diff: core/src/test/scala/org/apache/spark/SparkFunSuite.scala --- @@ -105,5 +105,16 @@ abstract class SparkFunSuite logInfo(s"\n\n= FINISHED $shortSuiteName: '$testName' =\n") } } - + /** + * Creates a temporary directory, which is then passed to `f` and will be deleted after `f` + * returns. + * + * @todo Probably this method should be moved to a more general place + */ + protected def withCreateTempDir(f: File => Unit): Unit = { --- End diff -- Yes, it seems like we should be able to use an override. The subclass that needs to inject an additional method call in the block can call the super method with a lambda that calls the user-supplied block, then this other method. It's probably worth whatever surgery is needed to make this clean and reduce duplication. We already have a lot of "create temp thing" methods all over. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org