Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-31003][TESTS] Fix incorrect uses of assume() in tests #27754

Closed
wants to merge 3 commits into from
Closed

[SPARK-31003][TESTS] Fix incorrect uses of assume() in tests #27754

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91e0ae2
WIP
JoshRosen Mar 2, 2020
082f590
Update LocalDirsSuite.scala
JoshRosen Mar 2, 2020
84f5105
Update MesosCoarseGrainedSchedulerBackend.scala
JoshRosen Mar 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/test/scala/org/apache/spark/storage/LocalDirsSuite.scala
View file
Open in desktop
Expand Up @@ -39,7 +39,7 @@ class LocalDirsSuite extends SparkFunSuite with BeforeAndAfter {

private def assumeNonExistentAndNotCreatable(f: File): Unit = {
try {
assume(!f.exists() && !f.mkdirs())
assert(!f.exists() && !f.mkdirs())
Copy link
Member

Choose a reason for hiding this comment

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

@JoshRosen, this assume was intentional (see https://github.com/apache/spark/pull/17987/files#r116532209) because /NENEXISTENT_PATH can be a valid path. We should ideally fix the tests to make it invalid paths in all OSs.

Copy link
Member

Choose a reason for hiding this comment

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

I am okay reverting this line for now too as it will be tricky to find out one invalid path in "both Windows and UNIX-like systems (e.g. Linux, Mac OS)".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted this line.

} finally {
Utils.deleteRecursively(f)
}
Expand Down
2 changes: 1 addition & 1 deletion ...n/scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackend.scala
View file
Open in desktop
Expand Up @@ -637,7 +637,7 @@ private[spark] class MesosCoarseGrainedSchedulerBackend(
if (state.equals(TaskState.RUNNING) &&
shuffleServiceEnabled &&
!slave.shuffleRegistered) {
assume(mesosExternalShuffleClient.isDefined,
Copy link
Member

Choose a reason for hiding this comment

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

@JoshRosen . Since this is not a test case, could you remove [TESTS] in the PR title?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rolled back this non-test change, so PR is now test-only.

assert(mesosExternalShuffleClient.isDefined,
"External shuffle client was not instantiated even though shuffle service is enabled.")
// TODO: Remove this and allow the MesosExternalShuffleService to detect
// framework termination when new Mesos Framework HTTP API is available.
Expand Down
2 changes: 1 addition & 1 deletion sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/OrderingSuite.scala
View file
Open in desktop
Expand Up @@ -106,7 +106,7 @@ class OrderingSuite extends SparkFunSuite with ExpressionEvalHelper {
StructField("a", dataType, nullable = true) ::
StructField("b", dataType, nullable = true) :: Nil)
val maybeDataGenerator = RandomDataGenerator.forType(rowType, nullable = false)
assume(maybeDataGenerator.isDefined)
assert(maybeDataGenerator.isDefined)
val randGenerator = maybeDataGenerator.get
val toCatalyst = CatalystTypeConverters.createToCatalystConverter(rowType)
for (_ <- 1 to 50) {
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/CachedTableSuite.scala
View file
Open in desktop
Expand Up @@ -195,7 +195,7 @@ class CachedTableSuite extends QueryTest with SQLTestUtils
}

test("SPARK-1669: cacheTable should be idempotent") {
assume(!spark.table("testData").logicalPlan.isInstanceOf[InMemoryRelation])
assert(!spark.table("testData").logicalPlan.isInstanceOf[InMemoryRelation])

spark.catalog.cacheTable("testData")
assertCached(spark.table("testData"))
Expand Down
4 changes: 2 additions & 2 deletions sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala
View file
Open in desktop
Expand Up @@ -1033,7 +1033,7 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
df.write.insertInto("students")
spark.catalog.cacheTable("students")
checkAnswer(spark.table("students"), df)
assume(spark.catalog.isCached("students"), "bad test: table was not cached in the first place")
assert(spark.catalog.isCached("students"), "bad test: table was not cached in the first place")
sql("ALTER TABLE students RENAME TO teachers")
sql("CREATE TABLE students (age INT, name STRING) USING parquet")
// Now we have both students and teachers.
Expand Down Expand Up @@ -1959,7 +1959,7 @@ abstract class DDLSuite extends QueryTest with SQLTestUtils {
Seq("json", "parquet").foreach { format =>
withTable("rectangles") {
data.write.format(format).saveAsTable("rectangles")
assume(spark.table("rectangles").collect().nonEmpty,
assert(spark.table("rectangles").collect().nonEmpty,
"bad test; table was empty to begin with")

sql("TRUNCATE TABLE rectangles")
Expand Down
6 changes: 3 additions & 3 deletions sql/core/src/test/scala/org/apache/spark/sql/internal/CatalogSuite.scala
View file
Open in desktop
Expand Up @@ -89,9 +89,9 @@ class CatalogSuite extends SharedSparkSession {
val columns = dbName
.map { db => spark.catalog.listColumns(db, tableName) }
.getOrElse { spark.catalog.listColumns(tableName) }
assume(tableMetadata.schema.nonEmpty, "bad test")
assume(tableMetadata.partitionColumnNames.nonEmpty, "bad test")
assume(tableMetadata.bucketSpec.isDefined, "bad test")
assert(tableMetadata.schema.nonEmpty, "bad test")
assert(tableMetadata.partitionColumnNames.nonEmpty, "bad test")
assert(tableMetadata.bucketSpec.isDefined, "bad test")
assert(columns.collect().map(_.name).toSet == tableMetadata.schema.map(_.name).toSet)
val bucketColumnNames = tableMetadata.bucketSpec.map(_.bucketColumnNames).getOrElse(Nil).toSet
columns.collect().foreach { col =>
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/sources/BucketedReadSuite.scala
View file
Open in desktop
Expand Up @@ -42,7 +42,7 @@ import org.apache.spark.util.collection.BitSet
class BucketedReadWithoutHiveSupportSuite extends BucketedReadSuite with SharedSparkSession {
protected override def beforeAll(): Unit = {
super.beforeAll()
assume(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "in-memory")
assert(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "in-memory")
}
}

Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/sources/BucketedWriteSuite.scala
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@ import org.apache.spark.sql.test.{SharedSparkSession, SQLTestUtils}
class BucketedWriteWithoutHiveSupportSuite extends BucketedWriteSuite with SharedSparkSession {
protected override def beforeAll(): Unit = {
super.beforeAll()
assume(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "in-memory")
assert(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "in-memory")
}

override protected def fileFormatsToTest: Seq[String] = Seq("parquet", "json")
Expand Down
6 changes: 3 additions & 3 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala
View file
Open in desktop
Expand Up @@ -981,8 +981,8 @@ class HiveDDLSuite
val expectedSerdePropsString =
expectedSerdeProps.map { case (k, v) => s"'$k'='$v'" }.mkString(", ")
val oldPart = catalog.getPartition(TableIdentifier("boxes"), Map("width" -> "4"))
assume(oldPart.storage.serde != Some(expectedSerde), "bad test: serde was already set")
assume(oldPart.storage.properties.filterKeys(expectedSerdeProps.contains) !=
assert(oldPart.storage.serde != Some(expectedSerde), "bad test: serde was already set")
assert(oldPart.storage.properties.filterKeys(expectedSerdeProps.contains) !=
expectedSerdeProps, "bad test: serde properties were already set")
sql(s"""ALTER TABLE boxes PARTITION (width=4)
| SET SERDE '$expectedSerde'
Expand Down Expand Up @@ -1735,7 +1735,7 @@ class HiveDDLSuite
Seq("json", "parquet").foreach { format =>
withTable("rectangles") {
data.write.format(format).saveAsTable("rectangles")
assume(spark.table("rectangles").collect().nonEmpty,
assert(spark.table("rectangles").collect().nonEmpty,
"bad test; table was empty to begin with")

sql("TRUNCATE TABLE rectangles")
Expand Down
2 changes: 1 addition & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/hive/test/TestHive.scala
View file
Open in desktop
Expand Up @@ -212,7 +212,7 @@ private[hive] class TestHiveSparkSession(
}
}

assume(sc.conf.get(CATALOG_IMPLEMENTATION) == "hive")
assert(sc.conf.get(CATALOG_IMPLEMENTATION) == "hive")

@transient
override lazy val sharedState: TestHiveSharedState = {
Expand Down
2 changes: 1 addition & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/sources/BucketedReadWithHiveSupportSuite.scala
View file
Open in desktop
Expand Up @@ -23,6 +23,6 @@ import org.apache.spark.sql.internal.StaticSQLConf.CATALOG_IMPLEMENTATION
class BucketedReadWithHiveSupportSuite extends BucketedReadSuite with TestHiveSingleton {
protected override def beforeAll(): Unit = {
super.beforeAll()
assume(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "hive")
assert(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "hive")
}
}
2 changes: 1 addition & 1 deletion sql/hive/src/test/scala/org/apache/spark/sql/sources/BucketedWriteWithHiveSupportSuite.scala
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ import org.apache.spark.sql.internal.StaticSQLConf.CATALOG_IMPLEMENTATION
class BucketedWriteWithHiveSupportSuite extends BucketedWriteSuite with TestHiveSingleton {
protected override def beforeAll(): Unit = {
super.beforeAll()
assume(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "hive")
assert(spark.sparkContext.conf.get(CATALOG_IMPLEMENTATION) == "hive")
}

override protected def fileFormatsToTest: Seq[String] = Seq("parquet", "orc")
Expand Down