fwc commented on code in PR #56190:
URL: https://github.com/apache/spark/pull/56190#discussion_r3326070806
##########
sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala:
##########
@@ -1211,7 +1211,7 @@ object QueryTest extends Assertions {
}
-class QueryTestSuite extends test.SharedSparkSession {
+class QueryTestSuite extends QueryTest with SparkSessionBinder {
Review Comment:
I want to nudge test authors towards writing (somewhat) connect-compatible
tests by default, which is why I want them to write tests with a
`sql.SparkSession` in hand.
My fear is that the 'clean' way is not the 'easiest' way. Most current tests
do not use an abstract base class and I fear that most test authors will
default to just start a new suite with `classic.SparkSessionBinder` as they
might not think about `connect` in that moment:
```scala
// hypothetical antipattern, but path of least resistance:
class FooSuite extends QueryTest with classic.SparkSessionBinder {
test("all tests, both shared and classic only") { ... }
}
```
I reworked the PR so that `SparkSessionBinder` now implements `QueryTest`.
Now `classic.SparkSessionBinder` is a drop-in replacement for
`SharedSparkSession` and `sql.SparkSessionBinder` provides the new, 'fixed'
default.
What do you think of this approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
