You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by nkronenfeld <gi...@git.apache.org> on 2017/10/19 00:05:19 UTC
[GitHub] spark pull request #19529: Support alternative unit testing styles in extern...
GitHub user nkronenfeld opened a pull request:
https://github.com/apache/spark/pull/19529
Support alternative unit testing styles in external applications
## What changes were proposed in this pull request?
Support unit tests of external code (i.e., applications that use spark) using scalatest that don't want to use FunSuite. SharedSparkContext already supports this, but SharedSQLContext does not.
I've introduced SharedSparkSession as a parent to SharedSQLContext, written in a way that it does support all scalatest styles.
## How was this patch tested?
There are three new unit test suites added that just test using FunSpec, FlatSpec, and WordSpec.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nkronenfeld/spark alternative-style-tests-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19529.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 #19529
----
commit b9d41cd79f05f6c420d070ad07cdfa8f853fd461
Author: Nathan Kronenfeld <ni...@gmail.com>
Date: 2017-10-15T03:04:16Z
Separate out the portion of SharedSQLContext that requires a FunSuite from the part that works with just any old test suite.
commit 0d4bd97247a2d083c7de55663703b38a34298c9c
Author: Nathan Kronenfeld <ni...@gmail.com>
Date: 2017-10-15T15:57:09Z
Fix typo in trait name
commit 83c44f1c24619e906af48180d0aace38587aa88d
Author: Nathan Kronenfeld <ni...@gmail.com>
Date: 2017-10-15T15:57:42Z
Add simple tests for each non-FunSuite test style
commit e460612ec6f36e62d8d21d88c2344378ecba581a
Author: Nathan Kronenfeld <ni...@gmail.com>
Date: 2017-10-15T16:20:44Z
Document testing possibilities
commit 0ee2aadf29b681b23bed356b14038525574204a5
Author: Nathan Kronenfeld <ni...@gmail.com>
Date: 2017-10-18T23:46:44Z
Better documentation of testing procedures
commit 802a958b640067b99fda0b2c8587dea5b8000495
Author: Nathan Kronenfeld <ni...@gmail.com>
Date: 2017-10-18T23:46:58Z
Same initialization issue in SharedSparkContext as is in SharedSparkSession
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #83006 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83006/testReport)** for PR 19529 at commit [`2d927e9`](https://github.com/apache/spark/commit/2d927e94f627919ac1546b47072276b23d3e8da2).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `trait PlanTestBase extends PredicateHelper `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #82969 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/82969/testReport)** for PR 19529 at commit [`4218b86`](https://github.com/apache/spark/commit/4218b86d5a8ff2321232ff38ed3e1b217ff7db2a).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146185400
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/GenericFlatSpecSuite.scala ---
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.test
+
+import org.scalatest.FlatSpec
+
+/**
+ * The purpose of this suite is to make sure that generic FlatSpec-based scala
+ * tests work with a shared spark session
+ */
+class GenericFlatSpecSuite extends FlatSpec with SharedSparkSession {
--- End diff --
These are testing tests? I get it, but I don't know if it's necessary. If FlatSpec didn't work, scalatest would hopefully catch it. If initializeSession / SharedSparkSession didn't, presumably tons of tests wouldn't work.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #83046 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83046/testReport)** for PR 19529 at commit [`24fc4a3`](https://github.com/apache/spark/commit/24fc4a324008b2acfcf5a2617eb7cc320565e83c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: Support alternative unit testing styles in external appl...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
There is one small hack in the way this was done, which is documented - see the comments and documentation on SharedSparkSession.initializeSession and SharedSparkContext.initializeContext. I would rather just have the session and context be lazy transient val's, which would work fine without this initialize call, but I didn't want to change the way tests currently ran without input.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146885926
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala ---
@@ -52,17 +55,142 @@ import org.apache.spark.util.{UninterruptibleThread, Utils}
* Subclasses should *not* create `SQLContext`s in the test suite constructor, which is
* prone to leaving multiple overlapping [[org.apache.spark.SparkContext]]s in the same JVM.
*/
-private[sql] trait SQLTestUtils
- extends SparkFunSuite with Eventually
+private[sql] trait SQLTestUtils extends SparkFunSuite with SQLTestUtilsBase with PlanTest {
+ // Whether to materialize all test data before the first test is run
+ private var loadTestDataBeforeTests = false
+
+ protected override def beforeAll(): Unit = {
+ super.beforeAll()
+ if (loadTestDataBeforeTests) {
+ loadTestData()
+ }
+ }
+
+ /**
+ * Materialize the test data immediately after the `SQLContext` is set up.
+ * This is necessary if the data is accessed by name but not through direct reference.
+ */
+ protected def setupTestData(): Unit = {
+ loadTestDataBeforeTests = true
+ }
+
+ /**
+ * Disable stdout and stderr when running the test. To not output the logs to the console,
+ * ConsoleAppender's `follow` should be set to `true` so that it will honors reassignments of
+ * System.out or System.err. Otherwise, ConsoleAppender will still output to the console even if
+ * we change System.out and System.err.
+ */
+ protected def testQuietly(name: String)(f: => Unit): Unit = {
+ test(name) {
+ quietly {
+ f
+ }
+ }
+ }
+
+ /**
+ * Run a test on a separate `UninterruptibleThread`.
+ */
+ protected def testWithUninterruptibleThread(name: String, quietly: Boolean = false)
+ (body: => Unit): Unit = {
+ val timeoutMillis = 10000
+ @transient var ex: Throwable = null
+
+ def runOnThread(): Unit = {
+ val thread = new UninterruptibleThread(s"Testing thread for test $name") {
+ override def run(): Unit = {
+ try {
+ body
+ } catch {
+ case NonFatal(e) =>
+ ex = e
+ }
+ }
+ }
+ thread.setDaemon(true)
+ thread.start()
+ thread.join(timeoutMillis)
+ if (thread.isAlive) {
+ thread.interrupt()
+ // If this interrupt does not work, then this thread is most likely running something that
+ // is not interruptible. There is not much point to wait for the thread to termniate, and
+ // we rather let the JVM terminate the thread on exit.
+ fail(
+ s"Test '$name' running on o.a.s.util.UninterruptibleThread timed out after" +
+ s" $timeoutMillis ms")
+ } else if (ex != null) {
+ throw ex
+ }
+ }
+
+ if (quietly) {
+ testQuietly(name) { runOnThread() }
+ } else {
+ test(name) { runOnThread() }
+ }
+ }
+}
+
+private[sql] object SQLTestUtils {
--- End diff --
done
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146312166
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/GenericFlatSpecSuite.scala ---
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.test
+
+import org.scalatest.FlatSpec
+
+/**
+ * The purpose of this suite is to make sure that generic FlatSpec-based scala
+ * tests work with a shared spark session
+ */
+class GenericFlatSpecSuite extends FlatSpec with SharedSparkSession {
--- End diff --
Yeah, I wasn't totally sure about that either, but I eventually came to the conclusion that they are useful. I think of it more in terms of preventing regressions; the case they will prevent is the FunSuite dependecy creeping back in to SharedSparkSession.
For similar reasons, I should probably put similar tests in for SharedSparkContext.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #3960 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3960/testReport)** for PR 19529 at commit [`2d927e9`](https://github.com/apache/spark/commit/2d927e94f627919ac1546b47072276b23d3e8da2).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #3960 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3960/testReport)** for PR 19529 at commit [`2d927e9`](https://github.com/apache/spark/commit/2d927e94f627919ac1546b47072276b23d3e8da2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `trait PlanTestBase extends PredicateHelper `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
Documentation removed as per @srowen 's request in the associated JIRA issue [SPARK-22308]
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #82969 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/82969/testReport)** for PR 19529 at commit [`4218b86`](https://github.com/apache/spark/commit/4218b86d5a8ff2321232ff38ed3e1b217ff7db2a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146977773
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,86 +17,8 @@
package org.apache.spark.sql.test
-import scala.concurrent.duration._
-
-import org.scalatest.BeforeAndAfterEach
-import org.scalatest.concurrent.Eventually
-
-import org.apache.spark.{DebugFilesystem, SparkConf}
-import org.apache.spark.sql.{SparkSession, SQLContext}
-import org.apache.spark.sql.internal.SQLConf
-
-/**
- * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
- */
-trait SharedSQLContext extends SQLTestUtils with BeforeAndAfterEach with Eventually {
-
- protected def sparkConf = {
- new SparkConf()
- .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
- .set("spark.unsafe.exceptionOnMemoryLeak", "true")
- .set(SQLConf.CODEGEN_FALLBACK.key, "false")
- }
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- *
- * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
- * mode with the default test configurations.
- */
- private var _spark: TestSparkSession = null
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- */
- protected implicit def spark: SparkSession = _spark
-
- /**
- * The [[TestSQLContext]] to use for all tests in this suite.
- */
- protected implicit def sqlContext: SQLContext = _spark.sqlContext
-
- protected def createSparkSession: TestSparkSession = {
- new TestSparkSession(sparkConf)
- }
-
- /**
- * Initialize the [[TestSparkSession]].
- */
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
protected override def beforeAll(): Unit = {
- SparkSession.sqlListener.set(null)
- if (_spark == null) {
- _spark = createSparkSession
- }
- // Ensure we have initialized the context before calling parent code
super.beforeAll()
--- End diff --
If this `beforeAll` is just calling `super.beforeAll`, why do we still need to override it?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83074/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146595869
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,86 +17,8 @@
package org.apache.spark.sql.test
-import scala.concurrent.duration._
-
-import org.scalatest.BeforeAndAfterEach
-import org.scalatest.concurrent.Eventually
-
-import org.apache.spark.{DebugFilesystem, SparkConf}
-import org.apache.spark.sql.{SparkSession, SQLContext}
-import org.apache.spark.sql.internal.SQLConf
-
-/**
- * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
- */
-trait SharedSQLContext extends SQLTestUtils with BeforeAndAfterEach with Eventually {
-
- protected def sparkConf = {
- new SparkConf()
- .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
- .set("spark.unsafe.exceptionOnMemoryLeak", "true")
- .set(SQLConf.CODEGEN_FALLBACK.key, "false")
- }
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- *
- * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
- * mode with the default test configurations.
- */
- private var _spark: TestSparkSession = null
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- */
- protected implicit def spark: SparkSession = _spark
-
- /**
- * The [[TestSQLContext]] to use for all tests in this suite.
- */
- protected implicit def sqlContext: SQLContext = _spark.sqlContext
-
- protected def createSparkSession: TestSparkSession = {
- new TestSparkSession(sparkConf)
- }
-
- /**
- * Initialize the [[TestSparkSession]].
- */
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
protected override def beforeAll(): Unit = {
--- End diff --
Why do we still need this?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19529
I just reverted this PR. @nkronenfeld Could you submit another PR and update the title to
`[SPARK-22308][test-maven] Support alternative unit testing styles in external applications`? Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r147038504
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,86 +17,8 @@
package org.apache.spark.sql.test
-import scala.concurrent.duration._
-
-import org.scalatest.BeforeAndAfterEach
-import org.scalatest.concurrent.Eventually
-
-import org.apache.spark.{DebugFilesystem, SparkConf}
-import org.apache.spark.sql.{SparkSession, SQLContext}
-import org.apache.spark.sql.internal.SQLConf
-
-/**
- * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
- */
-trait SharedSQLContext extends SQLTestUtils with BeforeAndAfterEach with Eventually {
-
- protected def sparkConf = {
- new SparkConf()
- .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
- .set("spark.unsafe.exceptionOnMemoryLeak", "true")
- .set(SQLConf.CODEGEN_FALLBACK.key, "false")
- }
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- *
- * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
- * mode with the default test configurations.
- */
- private var _spark: TestSparkSession = null
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- */
- protected implicit def spark: SparkSession = _spark
-
- /**
- * The [[TestSQLContext]] to use for all tests in this suite.
- */
- protected implicit def sqlContext: SQLContext = _spark.sqlContext
-
- protected def createSparkSession: TestSparkSession = {
- new TestSparkSession(sparkConf)
- }
-
- /**
- * Initialize the [[TestSparkSession]].
- */
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
protected override def beforeAll(): Unit = {
- SparkSession.sqlListener.set(null)
- if (_spark == null) {
- _spark = createSparkSession
- }
- // Ensure we have initialized the context before calling parent code
super.beforeAll()
--- End diff --
we don't. Removed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/82895/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: Support alternative unit testing styles in external appl...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #82895 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/82895/testReport)** for PR 19529 at commit [`802a958`](https://github.com/apache/spark/commit/802a958b640067b99fda0b2c8587dea5b8000495).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146185476
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala ---
@@ -52,249 +36,23 @@ import org.apache.spark.util.{UninterruptibleThread, Utils}
* Subclasses should *not* create `SQLContext`s in the test suite constructor, which is
* prone to leaving multiple overlapping [[org.apache.spark.SparkContext]]s in the same JVM.
*/
-private[sql] trait SQLTestUtils
- extends SparkFunSuite with Eventually
- with BeforeAndAfterAll
- with SQLTestData
- with PlanTest { self =>
-
- protected def sparkContext = spark.sparkContext
-
+private[sql] trait SQLTestUtils extends SparkFunSuite with SQLTestUtilsBase with PlanTest {
--- End diff --
Same general comment as PlanTestBase vs PlanTest above
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #82895 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/82895/testReport)** for PR 19529 at commit [`802a958`](https://github.com/apache/spark/commit/802a958b640067b99fda0b2c8587dea5b8000495).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146591649
--- Diff: core/src/test/scala/org/apache/spark/SharedSparkContext.scala ---
@@ -29,10 +29,25 @@ trait SharedSparkContext extends BeforeAndAfterAll with BeforeAndAfterEach { sel
var conf = new SparkConf(false)
+ /**
+ * Initialize the [[SparkContext]]. Generally, this is just called from
--- End diff --
nit: The max length of each line in the comment should also be 100, seems you are limiting that to 80?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
@gatorsmile sounds good, giving that a try now... assuming tests pass, I'll check it in and see if it's any better.
I've so far done this for PlanTest and SQLTestUtils
PlanTest I suspect it will make much cleaner.
In SQLTestUtils I suspect it won't help as much, as it was more a pick-and-choose (this function goes in base, this doesn't)
I haven't done it at all for SharedSQLContext/SharedSparkSession... that one seems more deserving of a first-level place to me, so I'm more hesitant to, but if you want, I'll do that one too.
I suspect the correct answer is going to be redoing the PR, with careful commits that are clearer about what each does... but I'll try this first anyway :-)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146185226
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala ---
@@ -18,158 +18,9 @@
package org.apache.spark.sql.catalyst.plans
import org.apache.spark.SparkFunSuite
-import org.apache.spark.sql.AnalysisException
-import org.apache.spark.sql.catalyst.analysis.SimpleAnalyzer
-import org.apache.spark.sql.catalyst.expressions._
-import org.apache.spark.sql.catalyst.expressions.aggregate.AggregateExpression
-import org.apache.spark.sql.catalyst.plans.logical._
-import org.apache.spark.sql.catalyst.util._
-import org.apache.spark.sql.internal.SQLConf
/**
* Provides helper methods for comparing plans.
*/
-trait PlanTest extends SparkFunSuite with PredicateHelper {
-
- // TODO(gatorsmile): remove this from PlanTest and all the analyzer rules
- protected def conf = SQLConf.get
-
- /**
- * Since attribute references are given globally unique ids during analysis,
- * we must normalize them to check if two different queries are identical.
- */
- protected def normalizeExprIds(plan: LogicalPlan) = {
- plan transformAllExpressions {
- case s: ScalarSubquery =>
- s.copy(exprId = ExprId(0))
- case e: Exists =>
- e.copy(exprId = ExprId(0))
- case l: ListQuery =>
- l.copy(exprId = ExprId(0))
- case a: AttributeReference =>
- AttributeReference(a.name, a.dataType, a.nullable)(exprId = ExprId(0))
- case a: Alias =>
- Alias(a.child, a.name)(exprId = ExprId(0))
- case ae: AggregateExpression =>
- ae.copy(resultId = ExprId(0))
- }
- }
-
- /**
- * Normalizes plans:
- * - Filter the filter conditions that appear in a plan. For instance,
- * ((expr 1 && expr 2) && expr 3), (expr 1 && expr 2 && expr 3), (expr 3 && (expr 1 && expr 2)
- * etc., will all now be equivalent.
- * - Sample the seed will replaced by 0L.
- * - Join conditions will be resorted by hashCode.
- */
- protected def normalizePlan(plan: LogicalPlan): LogicalPlan = {
- plan transform {
- case Filter(condition: Expression, child: LogicalPlan) =>
- Filter(splitConjunctivePredicates(condition).map(rewriteEqual).sortBy(_.hashCode())
- .reduce(And), child)
- case sample: Sample =>
- sample.copy(seed = 0L)
- case Join(left, right, joinType, condition) if condition.isDefined =>
- val newCondition =
- splitConjunctivePredicates(condition.get).map(rewriteEqual).sortBy(_.hashCode())
- .reduce(And)
- Join(left, right, joinType, Some(newCondition))
- }
- }
-
- /**
- * Rewrite [[EqualTo]] and [[EqualNullSafe]] operator to keep order. The following cases will be
- * equivalent:
- * 1. (a = b), (b = a);
- * 2. (a <=> b), (b <=> a).
- */
- private def rewriteEqual(condition: Expression): Expression = condition match {
- case eq @ EqualTo(l: Expression, r: Expression) =>
- Seq(l, r).sortBy(_.hashCode()).reduce(EqualTo)
- case eq @ EqualNullSafe(l: Expression, r: Expression) =>
- Seq(l, r).sortBy(_.hashCode()).reduce(EqualNullSafe)
- case _ => condition // Don't reorder.
- }
-
- /** Fails the test if the two plans do not match */
- protected def comparePlans(
- plan1: LogicalPlan,
- plan2: LogicalPlan,
- checkAnalysis: Boolean = true): Unit = {
- if (checkAnalysis) {
- // Make sure both plan pass checkAnalysis.
- SimpleAnalyzer.checkAnalysis(plan1)
- SimpleAnalyzer.checkAnalysis(plan2)
- }
-
- val normalized1 = normalizePlan(normalizeExprIds(plan1))
- val normalized2 = normalizePlan(normalizeExprIds(plan2))
- if (normalized1 != normalized2) {
- fail(
- s"""
- |== FAIL: Plans do not match ===
- |${sideBySide(normalized1.treeString, normalized2.treeString).mkString("\n")}
- """.stripMargin)
- }
- }
-
- /** Fails the test if the two expressions do not match */
- protected def compareExpressions(e1: Expression, e2: Expression): Unit = {
- comparePlans(Filter(e1, OneRowRelation()), Filter(e2, OneRowRelation()), checkAnalysis = false)
- }
-
- /** Fails the test if the join order in the two plans do not match */
- protected def compareJoinOrder(plan1: LogicalPlan, plan2: LogicalPlan) {
- val normalized1 = normalizePlan(normalizeExprIds(plan1))
- val normalized2 = normalizePlan(normalizeExprIds(plan2))
- if (!sameJoinPlan(normalized1, normalized2)) {
- fail(
- s"""
- |== FAIL: Plans do not match ===
- |${sideBySide(normalized1.treeString, normalized2.treeString).mkString("\n")}
- """.stripMargin)
- }
- }
-
- /** Consider symmetry for joins when comparing plans. */
- private def sameJoinPlan(plan1: LogicalPlan, plan2: LogicalPlan): Boolean = {
- (plan1, plan2) match {
- case (j1: Join, j2: Join) =>
- (sameJoinPlan(j1.left, j2.left) && sameJoinPlan(j1.right, j2.right)) ||
- (sameJoinPlan(j1.left, j2.right) && sameJoinPlan(j1.right, j2.left))
- case (p1: Project, p2: Project) =>
- p1.projectList == p2.projectList && sameJoinPlan(p1.child, p2.child)
- case _ =>
- plan1 == plan2
- }
- }
-
- /**
- * Sets all SQL configurations specified in `pairs`, calls `f`, and then restore all SQL
- * configurations.
- */
- protected def withSQLConf(pairs: (String, String)*)(f: => Unit): Unit = {
- val conf = SQLConf.get
- val (keys, values) = pairs.unzip
- val currentValues = keys.map { key =>
- if (conf.contains(key)) {
- Some(conf.getConfString(key))
- } else {
- None
- }
- }
- (keys, values).zipped.foreach { (k, v) =>
- if (SQLConf.staticConfKeys.contains(k)) {
- throw new AnalysisException(s"Cannot modify the value of a static config: $k")
- }
- conf.setConfString(k, v)
- }
- try f finally {
- keys.zip(currentValues).foreach {
- case (key, Some(value)) => conf.setConfString(key, value)
- case (key, None) => conf.unsetConf(key)
- }
- }
- }
+trait PlanTest extends SparkFunSuite with PlanTestBase {
--- End diff --
Overall, it looks like the content of this file was copied to a superclass, and now this is trait that extends it without adding anything. Is the point that it "extends SparkFunSuite"?
I don't know if this is possible in git, but renaming the class to PlanTestBase and then re-creating the current trait might have led to a diff where it's possible to see what did and didn't change in the move. Was there any substantive change? that's what I'm having trouble evaluating.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
@gatorsmile the code changes aren't huge - there's almost no new code here, it's all just moving code around from one file to another in order to expose a SharedSparkSession with no dependence on FunSuite.
I don't *think* that could be broken up into multiple PRs - half-done, it won't compile.
As for taking over the code movement, I'm not sure what you mean; please explain further?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
nope, using lazy val initialization won't work - at the very least, UnsafeKryoSerializerSuite modifies conf before context construction
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #83046 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83046/testReport)** for PR 19529 at commit [`24fc4a3`](https://github.com/apache/spark/commit/24fc4a324008b2acfcf5a2617eb7cc320565e83c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19529
The Jenkins jobs get killed not-infrequently for various reasons. You can ignore that and retest.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146308234
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala ---
@@ -18,158 +18,9 @@
package org.apache.spark.sql.catalyst.plans
import org.apache.spark.SparkFunSuite
-import org.apache.spark.sql.AnalysisException
-import org.apache.spark.sql.catalyst.analysis.SimpleAnalyzer
-import org.apache.spark.sql.catalyst.expressions._
-import org.apache.spark.sql.catalyst.expressions.aggregate.AggregateExpression
-import org.apache.spark.sql.catalyst.plans.logical._
-import org.apache.spark.sql.catalyst.util._
-import org.apache.spark.sql.internal.SQLConf
/**
* Provides helper methods for comparing plans.
*/
-trait PlanTest extends SparkFunSuite with PredicateHelper {
-
- // TODO(gatorsmile): remove this from PlanTest and all the analyzer rules
- protected def conf = SQLConf.get
-
- /**
- * Since attribute references are given globally unique ids during analysis,
- * we must normalize them to check if two different queries are identical.
- */
- protected def normalizeExprIds(plan: LogicalPlan) = {
- plan transformAllExpressions {
- case s: ScalarSubquery =>
- s.copy(exprId = ExprId(0))
- case e: Exists =>
- e.copy(exprId = ExprId(0))
- case l: ListQuery =>
- l.copy(exprId = ExprId(0))
- case a: AttributeReference =>
- AttributeReference(a.name, a.dataType, a.nullable)(exprId = ExprId(0))
- case a: Alias =>
- Alias(a.child, a.name)(exprId = ExprId(0))
- case ae: AggregateExpression =>
- ae.copy(resultId = ExprId(0))
- }
- }
-
- /**
- * Normalizes plans:
- * - Filter the filter conditions that appear in a plan. For instance,
- * ((expr 1 && expr 2) && expr 3), (expr 1 && expr 2 && expr 3), (expr 3 && (expr 1 && expr 2)
- * etc., will all now be equivalent.
- * - Sample the seed will replaced by 0L.
- * - Join conditions will be resorted by hashCode.
- */
- protected def normalizePlan(plan: LogicalPlan): LogicalPlan = {
- plan transform {
- case Filter(condition: Expression, child: LogicalPlan) =>
- Filter(splitConjunctivePredicates(condition).map(rewriteEqual).sortBy(_.hashCode())
- .reduce(And), child)
- case sample: Sample =>
- sample.copy(seed = 0L)
- case Join(left, right, joinType, condition) if condition.isDefined =>
- val newCondition =
- splitConjunctivePredicates(condition.get).map(rewriteEqual).sortBy(_.hashCode())
- .reduce(And)
- Join(left, right, joinType, Some(newCondition))
- }
- }
-
- /**
- * Rewrite [[EqualTo]] and [[EqualNullSafe]] operator to keep order. The following cases will be
- * equivalent:
- * 1. (a = b), (b = a);
- * 2. (a <=> b), (b <=> a).
- */
- private def rewriteEqual(condition: Expression): Expression = condition match {
- case eq @ EqualTo(l: Expression, r: Expression) =>
- Seq(l, r).sortBy(_.hashCode()).reduce(EqualTo)
- case eq @ EqualNullSafe(l: Expression, r: Expression) =>
- Seq(l, r).sortBy(_.hashCode()).reduce(EqualNullSafe)
- case _ => condition // Don't reorder.
- }
-
- /** Fails the test if the two plans do not match */
- protected def comparePlans(
- plan1: LogicalPlan,
- plan2: LogicalPlan,
- checkAnalysis: Boolean = true): Unit = {
- if (checkAnalysis) {
- // Make sure both plan pass checkAnalysis.
- SimpleAnalyzer.checkAnalysis(plan1)
- SimpleAnalyzer.checkAnalysis(plan2)
- }
-
- val normalized1 = normalizePlan(normalizeExprIds(plan1))
- val normalized2 = normalizePlan(normalizeExprIds(plan2))
- if (normalized1 != normalized2) {
- fail(
- s"""
- |== FAIL: Plans do not match ===
- |${sideBySide(normalized1.treeString, normalized2.treeString).mkString("\n")}
- """.stripMargin)
- }
- }
-
- /** Fails the test if the two expressions do not match */
- protected def compareExpressions(e1: Expression, e2: Expression): Unit = {
- comparePlans(Filter(e1, OneRowRelation()), Filter(e2, OneRowRelation()), checkAnalysis = false)
- }
-
- /** Fails the test if the join order in the two plans do not match */
- protected def compareJoinOrder(plan1: LogicalPlan, plan2: LogicalPlan) {
- val normalized1 = normalizePlan(normalizeExprIds(plan1))
- val normalized2 = normalizePlan(normalizeExprIds(plan2))
- if (!sameJoinPlan(normalized1, normalized2)) {
- fail(
- s"""
- |== FAIL: Plans do not match ===
- |${sideBySide(normalized1.treeString, normalized2.treeString).mkString("\n")}
- """.stripMargin)
- }
- }
-
- /** Consider symmetry for joins when comparing plans. */
- private def sameJoinPlan(plan1: LogicalPlan, plan2: LogicalPlan): Boolean = {
- (plan1, plan2) match {
- case (j1: Join, j2: Join) =>
- (sameJoinPlan(j1.left, j2.left) && sameJoinPlan(j1.right, j2.right)) ||
- (sameJoinPlan(j1.left, j2.right) && sameJoinPlan(j1.right, j2.left))
- case (p1: Project, p2: Project) =>
- p1.projectList == p2.projectList && sameJoinPlan(p1.child, p2.child)
- case _ =>
- plan1 == plan2
- }
- }
-
- /**
- * Sets all SQL configurations specified in `pairs`, calls `f`, and then restore all SQL
- * configurations.
- */
- protected def withSQLConf(pairs: (String, String)*)(f: => Unit): Unit = {
- val conf = SQLConf.get
- val (keys, values) = pairs.unzip
- val currentValues = keys.map { key =>
- if (conf.contains(key)) {
- Some(conf.getConfString(key))
- } else {
- None
- }
- }
- (keys, values).zipped.foreach { (k, v) =>
- if (SQLConf.staticConfKeys.contains(k)) {
- throw new AnalysisException(s"Cannot modify the value of a static config: $k")
- }
- conf.setConfString(k, v)
- }
- try f finally {
- keys.zip(currentValues).foreach {
- case (key, Some(value)) => conf.setConfString(key, value)
- case (key, None) => conf.unsetConf(key)
- }
- }
- }
+trait PlanTest extends SparkFunSuite with PlanTestBase {
--- End diff --
I actually did start with 'git mv PlanTest.scala PlanTestBase.scala' - sadly, the diffs don't catch that. :-(
You understand the intent exactly - to pull the FunSuite part out of PlanTestBase and include it in PlanTest.
To confirm that nothing else changed, I ran:
git checkout 4a779bdac3e75c17b7d36c5a009ba6c948fa9fb6 PlanTest.scala
diff PlanTest.scala PlanTestBase.scala
and got the following:
20c20,21
< import org.apache.spark.SparkFunSuite
---
> import org.scalatest.Suite
>
30c31,32
< * Provides helper methods for comparing plans.
---
> * Provides helper methods for comparing plans, but without the overhead of
> * mandating a FunSuite.
32c34
< trait PlanTest extends SparkFunSuite with PredicateHelper {
---
> trait PlanTestBase extends PredicateHelper { self: Suite =>
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
I assume the "unknown error code, -9" is:
[error] running /home/jenkins/workspace/SparkPullRequestBuilder/build/sbt -Phadoop-2.6 -Pflume -Phive-thriftserver -Pyarn -Pkafka-0-8 -Phive -Pkinesis-asl -Pmesos -Dtest.exclude.tags=org.apache.spark.tags.ExtendedHiveTest,org.apache.spark.tags.ExtendedYarnTest test ; process was terminated by signal 9
It started popping up between build 82895 and 82969 - between which all I did was eliminate the documentation on unit testing. I wouldn't think this should affect the build.
Signal 9 is a kill signal - which means something external killed the build, I think. Any idea why this is happening for the last several builds?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19529
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #83074 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83074/testReport)** for PR 19529 at commit [`6c0b0d5`](https://github.com/apache/spark/commit/6c0b0d569ae1d779fd9253da0c7e97d12634063c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
Yeah, as predicted, that made PlanTest very easy to review, but didn't do as well with SQLTestUtils. I suspect I reordered functions and what-not when I was moving stuff around.
If this is still too confusing to deal with, just let me know. Even if I can't make the end diffs of the entire PR non-trivial, I could certainly re-implement this in a way that the individual commits would each be trivial; then it'd just be a question of following along commit-by-commit, and shouldn't be too bad.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19529
Generally, it makes sense to me. Since the code changes are pretty large here, it is not very straightforward for us to review it. Do you mind if I taking over some of them? Or could you split the PR to multiple smaller ones?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146976375
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala ---
@@ -29,7 +31,14 @@ import org.apache.spark.sql.internal.SQLConf
/**
* Provides helper methods for comparing plans.
*/
-trait PlanTest extends SparkFunSuite with PredicateHelper {
+trait PlanTest extends SparkFunSuite with PlanTestBase {
+}
--- End diff --
Please remove the useless `{` and `}`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146186223
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSparkSession.scala ---
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.test
+
+import scala.concurrent.duration._
+
+import org.scalatest.{BeforeAndAfterEach, Suite}
+import org.scalatest.concurrent.Eventually
+
+import org.apache.spark.{DebugFilesystem, SparkConf}
+import org.apache.spark.sql.{SparkSession, SQLContext}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
+ */
+trait SharedSparkSession
--- End diff --
Is this actually specific to SQL?
And is it really for symmetry with SharedSparkContext?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146978824
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,86 +17,8 @@
package org.apache.spark.sql.test
-import scala.concurrent.duration._
-
-import org.scalatest.BeforeAndAfterEach
-import org.scalatest.concurrent.Eventually
-
-import org.apache.spark.{DebugFilesystem, SparkConf}
-import org.apache.spark.sql.{SparkSession, SQLContext}
-import org.apache.spark.sql.internal.SQLConf
-
-/**
- * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
- */
-trait SharedSQLContext extends SQLTestUtils with BeforeAndAfterEach with Eventually {
-
- protected def sparkConf = {
- new SparkConf()
- .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
- .set("spark.unsafe.exceptionOnMemoryLeak", "true")
- .set(SQLConf.CODEGEN_FALLBACK.key, "false")
- }
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- *
- * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
- * mode with the default test configurations.
- */
- private var _spark: TestSparkSession = null
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- */
- protected implicit def spark: SparkSession = _spark
-
- /**
- * The [[TestSQLContext]] to use for all tests in this suite.
- */
- protected implicit def sqlContext: SQLContext = _spark.sqlContext
-
- protected def createSparkSession: TestSparkSession = {
- new TestSparkSession(sparkConf)
- }
-
- /**
- * Initialize the [[TestSparkSession]].
- */
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
--- End diff --
Does this work? Could we move `SharedSparkSession.scala` in this file?
```Scala
trait SharedSQLContext extends SQLTestUtils with SharedSparkSession
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83006/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83046/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146311180
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala ---
@@ -52,249 +36,23 @@ import org.apache.spark.util.{UninterruptibleThread, Utils}
* Subclasses should *not* create `SQLContext`s in the test suite constructor, which is
* prone to leaving multiple overlapping [[org.apache.spark.SparkContext]]s in the same JVM.
*/
-private[sql] trait SQLTestUtils
- extends SparkFunSuite with Eventually
- with BeforeAndAfterAll
- with SQLTestData
- with PlanTest { self =>
-
- protected def sparkContext = spark.sparkContext
-
+private[sql] trait SQLTestUtils extends SparkFunSuite with SQLTestUtilsBase with PlanTest {
--- End diff --
This has more changes - I left the data initialization stuff in SQLTestUtils, as it wouldn't be needed by external tests. All the <'s below are things that I pulled out of SQLTestUtilsBase and put back into SQLTestUtils
Running similarly:
git checkout 4a779bdac3e75c17b7d36c5a009ba6c948fa9fb6 SQLTestUtils.scala
diff SQLTestUtils.scala SQLTestUtilsBase.scala
I get
27d26
< import scala.util.control.NonFatal
30c29
< import org.scalatest.BeforeAndAfterAll
---
> import org.scalatest.{BeforeAndAfterAll, Suite}
33d31
< import org.apache.spark.SparkFunSuite
38c36
< import org.apache.spark.sql.catalyst.plans.PlanTest
---
> import org.apache.spark.sql.catalyst.plans.PlanTestBase
40d37
< import org.apache.spark.sql.catalyst.util._
43c40
< import org.apache.spark.util.{UninterruptibleThread, Utils}
---
> import org.apache.spark.util.Utils
46c43
< * Helper trait that should be extended by all SQL test suites.
---
> * Helper trait that can be extended by all external SQL test suites.
48,49c45
< * This allows subclasses to plugin a custom `SQLContext`. It comes with test data
< * prepared in advance as well as all implicit conversions used extensively by dataframes.
---
> * This allows subclasses to plugin a custom `SQLContext`.
55,56c51,52
< private[sql] trait SQLTestUtils
< extends SparkFunSuite with Eventually
---
> private[sql] trait SQLTestUtilsBase
> extends Eventually
59c55
< with PlanTest { self =>
---
> with PlanTestBase { self: Suite =>
63,65d58
< // Whether to materialize all test data before the first test is run
< private var loadTestDataBeforeTests = false
<
80,94d72
< /**
< * Materialize the test data immediately after the `SQLContext` is set up.
< * This is necessary if the data is accessed by name but not through direct reference.
< */
< protected def setupTestData(): Unit = {
< loadTestDataBeforeTests = true
< }
<
< protected override def beforeAll(): Unit = {
< super.beforeAll()
< if (loadTestDataBeforeTests) {
< loadTestData()
< }
< }
<
300,354d277
< /**
< * Disable stdout and stderr when running the test. To not output the logs to the console,
< * ConsoleAppender's `follow` should be set to `true` so that it will honors reassignments of
< * System.out or System.err. Otherwise, ConsoleAppender will still output to the console even if
< * we change System.out and System.err.
< */
< protected def testQuietly(name: String)(f: => Unit): Unit = {
< test(name) {
< quietly {
< f
< }
< }
< }
<
< /**
< * Run a test on a separate `UninterruptibleThread`.
< */
< protected def testWithUninterruptibleThread(name: String, quietly: Boolean = false)
< (body: => Unit): Unit = {
< val timeoutMillis = 10000
< @transient var ex: Throwable = null
<
< def runOnThread(): Unit = {
< val thread = new UninterruptibleThread(s"Testing thread for test $name") {
< override def run(): Unit = {
< try {
< body
< } catch {
< case NonFatal(e) =>
< ex = e
< }
< }
< }
< thread.setDaemon(true)
< thread.start()
< thread.join(timeoutMillis)
< if (thread.isAlive) {
< thread.interrupt()
< // If this interrupt does not work, then this thread is most likely running something that
< // is not interruptible. There is not much point to wait for the thread to termniate, and
< // we rather let the JVM terminate the thread on exit.
< fail(
< s"Test '$name' running on o.a.s.util.UninterruptibleThread timed out after" +
< s" $timeoutMillis ms")
< } else if (ex != null) {
< throw ex
< }
< }
<
< if (quietly) {
< testQuietly(name) { runOnThread() }
< } else {
< test(name) { runOnThread() }
< }
< }
365,407d287
< }
< }
<
< private[sql] object SQLTestUtils {
<
< def compareAnswers(
< sparkAnswer: Seq[Row],
< expectedAnswer: Seq[Row],
< sort: Boolean): Option[String] = {
< def prepareAnswer(answer: Seq[Row]): Seq[Row] = {
< // Converts data to types that we can do equality comparison using Scala collections.
< // For BigDecimal type, the Scala type has a better definition of equality test (similar to
< // Java's java.math.BigDecimal.compareTo).
< // For binary arrays, we convert it to Seq to avoid of calling java.util.Arrays.equals for
< // equality test.
< // This function is copied from Catalyst's QueryTest
< val converted: Seq[Row] = answer.map { s =>
< Row.fromSeq(s.toSeq.map {
< case d: java.math.BigDecimal => BigDecimal(d)
< case b: Array[Byte] => b.toSeq
< case o => o
< })
< }
< if (sort) {
< converted.sortBy(_.toString())
< } else {
< converted
< }
< }
< if (prepareAnswer(expectedAnswer) != prepareAnswer(sparkAnswer)) {
< val errorMessage =
< s"""
< | == Results ==
< | ${sideBySide(
< s"== Expected Answer - ${expectedAnswer.size} ==" +:
< prepareAnswer(expectedAnswer).map(_.toString()),
< s"== Actual Answer - ${sparkAnswer.size} ==" +:
< prepareAnswer(sparkAnswer).map(_.toString())).mkString("\n")}
< """.stripMargin
< Some(errorMessage)
< } else {
< None
< }
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146595345
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala ---
@@ -52,17 +55,142 @@ import org.apache.spark.util.{UninterruptibleThread, Utils}
* Subclasses should *not* create `SQLContext`s in the test suite constructor, which is
* prone to leaving multiple overlapping [[org.apache.spark.SparkContext]]s in the same JVM.
*/
-private[sql] trait SQLTestUtils
- extends SparkFunSuite with Eventually
+private[sql] trait SQLTestUtils extends SparkFunSuite with SQLTestUtilsBase with PlanTest {
+ // Whether to materialize all test data before the first test is run
+ private var loadTestDataBeforeTests = false
+
+ protected override def beforeAll(): Unit = {
+ super.beforeAll()
+ if (loadTestDataBeforeTests) {
+ loadTestData()
+ }
+ }
+
+ /**
+ * Materialize the test data immediately after the `SQLContext` is set up.
+ * This is necessary if the data is accessed by name but not through direct reference.
+ */
+ protected def setupTestData(): Unit = {
+ loadTestDataBeforeTests = true
+ }
+
+ /**
+ * Disable stdout and stderr when running the test. To not output the logs to the console,
+ * ConsoleAppender's `follow` should be set to `true` so that it will honors reassignments of
+ * System.out or System.err. Otherwise, ConsoleAppender will still output to the console even if
+ * we change System.out and System.err.
+ */
+ protected def testQuietly(name: String)(f: => Unit): Unit = {
+ test(name) {
+ quietly {
+ f
+ }
+ }
+ }
+
+ /**
+ * Run a test on a separate `UninterruptibleThread`.
+ */
+ protected def testWithUninterruptibleThread(name: String, quietly: Boolean = false)
+ (body: => Unit): Unit = {
+ val timeoutMillis = 10000
+ @transient var ex: Throwable = null
+
+ def runOnThread(): Unit = {
+ val thread = new UninterruptibleThread(s"Testing thread for test $name") {
+ override def run(): Unit = {
+ try {
+ body
+ } catch {
+ case NonFatal(e) =>
+ ex = e
+ }
+ }
+ }
+ thread.setDaemon(true)
+ thread.start()
+ thread.join(timeoutMillis)
+ if (thread.isAlive) {
+ thread.interrupt()
+ // If this interrupt does not work, then this thread is most likely running something that
+ // is not interruptible. There is not much point to wait for the thread to termniate, and
+ // we rather let the JVM terminate the thread on exit.
+ fail(
+ s"Test '$name' running on o.a.s.util.UninterruptibleThread timed out after" +
+ s" $timeoutMillis ms")
+ } else if (ex != null) {
+ throw ex
+ }
+ }
+
+ if (quietly) {
+ testQuietly(name) { runOnThread() }
+ } else {
+ test(name) { runOnThread() }
+ }
+ }
+}
+
+private[sql] object SQLTestUtils {
--- End diff --
Could you move this after `SQLTestUtilsBase` ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r147038487
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala ---
@@ -29,7 +31,14 @@ import org.apache.spark.sql.internal.SQLConf
/**
* Provides helper methods for comparing plans.
*/
-trait PlanTest extends SparkFunSuite with PredicateHelper {
+trait PlanTest extends SparkFunSuite with PlanTestBase {
+}
--- End diff --
done
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19529
LGTM pending Jenkins
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/82969/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19529
I found a very simple way to reduce the line of changes.
Could you put the PlanTest and PlanTestBase in the same file? We can refactor it later, if necessary. For example, in `PlanTest.scala`
```Scala
/**
* Provides helper methods for comparing plans.
*/
trait PlanTest extends SparkFunSuite with PlanTestBase
/**
* Provides helper methods for comparing plans, but without the overhead of
* mandating a FunSuite.
*/
trait PlanTestBase extends PredicateHelper { self: Suite =>
```
You also can do the same thing for the other files.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19529
Maven is the build of reference... YMMV but I also have a bunch of trouble with SBT whereas not with Maven. I'd honestly prefer we not support an SBT build explicitly for reasons like this, though make sure it's not hard to let it work if people want to. at least the PR builder should be Maven.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r147038647
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,86 +17,8 @@
package org.apache.spark.sql.test
-import scala.concurrent.duration._
-
-import org.scalatest.BeforeAndAfterEach
-import org.scalatest.concurrent.Eventually
-
-import org.apache.spark.{DebugFilesystem, SparkConf}
-import org.apache.spark.sql.{SparkSession, SQLContext}
-import org.apache.spark.sql.internal.SQLConf
-
-/**
- * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
- */
-trait SharedSQLContext extends SQLTestUtils with BeforeAndAfterEach with Eventually {
-
- protected def sparkConf = {
- new SparkConf()
- .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
- .set("spark.unsafe.exceptionOnMemoryLeak", "true")
- .set(SQLConf.CODEGEN_FALLBACK.key, "false")
- }
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- *
- * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
- * mode with the default test configurations.
- */
- private var _spark: TestSparkSession = null
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- */
- protected implicit def spark: SparkSession = _spark
-
- /**
- * The [[TestSQLContext]] to use for all tests in this suite.
- */
- protected implicit def sqlContext: SQLContext = _spark.sqlContext
-
- protected def createSparkSession: TestSparkSession = {
- new TestSparkSession(sparkConf)
- }
-
- /**
- * Initialize the [[TestSparkSession]].
- */
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
--- End diff --
We could... that would more fit the pattern of what we've done now for PlanTest/PlanTestBase and SQLTestUtils/SQLTestUtilsBase.
I hesitated in this case just because the two are such conceptually different concepts, and the idea is that both would actually get used - SharedSQLContext in internal tests, SharedSparkSession in external tests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #83074 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83074/testReport)** for PR 19529 at commit [`6c0b0d5`](https://github.com/apache/spark/commit/6c0b0d569ae1d779fd9253da0c7e97d12634063c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
hm... I was always testing with sbt, because maven was so slow to do anything.
Will do
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19529
**[Test build #83006 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83006/testReport)** for PR 19529 at commit [`2d927e9`](https://github.com/apache/spark/commit/2d927e94f627919ac1546b47072276b23d3e8da2).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19529
Thanks! Merged to master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146311684
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSparkSession.scala ---
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.test
+
+import scala.concurrent.duration._
+
+import org.scalatest.{BeforeAndAfterEach, Suite}
+import org.scalatest.concurrent.Eventually
+
+import org.apache.spark.{DebugFilesystem, SparkConf}
+import org.apache.spark.sql.{SparkSession, SQLContext}
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
+ */
+trait SharedSparkSession
--- End diff --
I'm not sure what you're asking.
It's specific to the Spark SQL package - in that SparkSession resides there - but not specific to using the language SQL. When I say, 'SQL test suites', I'm referring to the former.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146887435
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/test/SharedSQLContext.scala ---
@@ -17,86 +17,8 @@
package org.apache.spark.sql.test
-import scala.concurrent.duration._
-
-import org.scalatest.BeforeAndAfterEach
-import org.scalatest.concurrent.Eventually
-
-import org.apache.spark.{DebugFilesystem, SparkConf}
-import org.apache.spark.sql.{SparkSession, SQLContext}
-import org.apache.spark.sql.internal.SQLConf
-
-/**
- * Helper trait for SQL test suites where all tests share a single [[TestSparkSession]].
- */
-trait SharedSQLContext extends SQLTestUtils with BeforeAndAfterEach with Eventually {
-
- protected def sparkConf = {
- new SparkConf()
- .set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
- .set("spark.unsafe.exceptionOnMemoryLeak", "true")
- .set(SQLConf.CODEGEN_FALLBACK.key, "false")
- }
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- *
- * By default, the underlying [[org.apache.spark.SparkContext]] will be run in local
- * mode with the default test configurations.
- */
- private var _spark: TestSparkSession = null
-
- /**
- * The [[TestSparkSession]] to use for all tests in this suite.
- */
- protected implicit def spark: SparkSession = _spark
-
- /**
- * The [[TestSQLContext]] to use for all tests in this suite.
- */
- protected implicit def sqlContext: SQLContext = _spark.sqlContext
-
- protected def createSparkSession: TestSparkSession = {
- new TestSparkSession(sparkConf)
- }
-
- /**
- * Initialize the [[TestSparkSession]].
- */
+trait SharedSQLContext extends SQLTestUtils with SharedSparkSession {
protected override def beforeAll(): Unit = {
--- End diff --
It's still used throughout the unit tests.
I had noticed another issue related to that ([SPARK-15037]), but that is marked resolved without having gotten rid of this.
Note that SharedSQLContext is to SharedSessionContext as PlanTest is to PlanTestBase - it extends FunSuite.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19529: [SPARK-22308] Support alternative unit testing st...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on a diff in the pull request:
https://github.com/apache/spark/pull/19529#discussion_r146885975
--- Diff: core/src/test/scala/org/apache/spark/SharedSparkContext.scala ---
@@ -29,10 +29,25 @@ trait SharedSparkContext extends BeforeAndAfterAll with BeforeAndAfterEach { sel
var conf = new SparkConf(false)
+ /**
+ * Initialize the [[SparkContext]]. Generally, this is just called from
--- End diff --
fixed here, not elsewhere yet
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/19529
@nkronenfeld @gatorsmile I think this has been failing the master build (Maven only) for a few days:
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.6/3981/consoleFull
```
SQLQuerySuite:
*** RUN ABORTED ***
org.apache.spark.SparkException: Only one SparkContext may be running in this JVM (see SPARK-2243). To ignore this error, set spark.driver.allowMultipleContexts = true. The currently running SparkContext was created at:
org.apache.spark.sql.test.GenericFunSpecSuite.<init>(GenericFunSpecSuite.scala:28)
sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
java.lang.reflect.Constructor.newInstance(Constructor.java:422)
java.lang.Class.newInstance(Class.java:442)
org.scalatest.tools.DiscoverySuite$.getSuiteInstance(DiscoverySuite.scala:66)
org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:38)
org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:37)
scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
scala.collection.Iterator$class.foreach(Iterator.scala:893)
scala.collection.AbstractIterator.foreach(Iterator.scala:1336)
scala.collection.IterableLike$class.foreach(IterableLike.scala:72)
scala.collection.AbstractIterable.foreach(Iterable.scala:54)
scala.collection.TraversableLike$class.map(TraversableLike.scala:234)
scala.collection.AbstractTraversable.map(Traversable.scala:104)
org.scalatest.tools.DiscoverySuite.<init>(DiscoverySuite.scala:37)
org.scalatest.tools.Runner$.genDiscoSuites$1(Runner.scala:1165)
org.scalatest.tools.Runner$.doRunRunRunDaDoRunRun(Runner.scala:1250)
```
I suspect that there's some slightly different way that the tests execute in Maven that may highlight a problem with how the SQL tests use and reuse SparkContexts. It's likely this change did cause it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: Support alternative unit testing styles in external appl...
Posted by nkronenfeld <gi...@git.apache.org>.
Github user nkronenfeld commented on the issue:
https://github.com/apache/spark/pull/19529
I made my original changes here by using
git mv PlanTest.scala PlanTestBase.scala
git mv SQLTestUnit.scala SQLTestUnitBase.scala
git mv SharedSQLContext.scala SharedSparkSession.scala
and then created new PlanTest, SQLTestUnit, and SharedSQLContext files, under the assumption that most of the code would go in the base, and this would help git provide better history and continuity. I'm not sure if that was the right decision or not - probably it is with PlanTest and SQLTestUnit, possibly not with SharedSQLContext, but since the diff in the PR doesn't reflect the git mv properly, I'm not sure if it will make a difference either way.
If reviewers wish me to redo this PR without the initial `git mv`, I'll be happy to.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19529: [SPARK-22308] Support alternative unit testing styles in...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19529
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org