[SPARK-58407][SQL][TESTS][CONNECT] Fix connect.SessionQueryTest beforeAll, afterAll - #57606
[SPARK-58407][SQL][TESTS][CONNECT] Fix connect.SessionQueryTest beforeAll, afterAll#57606fwc wants to merge 6 commits into
Conversation
ea830d5 to
13c38f0
Compare
|
LGTM, cc @HyukjinKwon |
HyukjinKwon
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Correct test-lifecycle fix that brings the Connect binder in line with the classic one; well covered.
Verification
Confirmed the reordering: session-before-super on setup, super-before-close on teardown. The new SessionQueryTestBeforeAfterHooksSuite (subclassed for Connect) asserts spark.sql runs in beforeAll, afterAll, and a normal test, so both classic and Connect paths are exercised. Test-only; no production surface (no scanners applicable).
cloud-fan
left a comment
There was a problem hiding this comment.
1 blocking, 0 non-blocking, 0 nits.
The normal lifecycle ordering is correct, but teardown must retain its failure-safe cleanup guarantee.
Correctness (1)
- sql/connect/server/src/test/scala/org/apache/spark/sql/connect/SparkSessionBinder.scala:68: Keep Connect resource cleanup in a finally block after suite-level afterAll. If an inherited or suite override throws, the current ordering skips closing the Connect session and stopping SparkConnectService, leaking shared test infrastructure into later suites. The classic binder preserves this cleanup guarantee with try/finally. -- see inline
Verification
Traced the Connect binder through its SQL-core parent and confirmed that session availability now spans suite-level hooks. Also compared teardown with the classic binder's try/finally cleanup. Tests were not run as part of this review.
| } | ||
|
|
||
| override def afterAll(): Unit = { | ||
| super.afterAll() |
There was a problem hiding this comment.
Please keep cleanup in a finally block after invoking the suite hooks. If super.afterAll() throws (including from the new suite's SQL assertion), this method now skips closing _connectSpark and stopping the global service, which can contaminate later suites. The classic binder preserves this guarantee with try/finally.
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 0 new.
0 blocking, 0 non-blocking, 0 nits.
The prior teardown concern is addressed, and the current lifecycle change is ready.
Verification
Traced setup and teardown through the Connect binder and its classic parent. Confirmed that suite hooks run only after session creation, and that Connect cleanup remains in a finally block if inherited teardown throws. Reviewed the shared hook suite and its Connect binding; tests were not run as part of this review.
…All, afterAll ### What changes were proposed in this pull request? Changes `connect.SparkSessionBinder` setup / teardown, so that suites can use `spark` in `beforeAll` and `afterAll`. ### Why are the changes needed? Some tests do setup/cleanup calls (e.g. create/delete some shared table) in `beforeAll`/`afterAll`. While this works with `sql.SessionQueryTest`, this is broken in `connect.SessionQueryTest`. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? This is a test-only change ### Was this patch authored or co-authored using generative AI tooling? No Closes #57606 from fwc/fix-connect-sessionquerytest-lifecycle. Lead-authored-by: Matthis Gördel <matthis@posteo.de> Co-authored-by: Matthis Gördel <matthis.goerdel@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 56dcb54) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
…All, afterAll ### What changes were proposed in this pull request? Changes `connect.SparkSessionBinder` setup / teardown, so that suites can use `spark` in `beforeAll` and `afterAll`. ### Why are the changes needed? Some tests do setup/cleanup calls (e.g. create/delete some shared table) in `beforeAll`/`afterAll`. While this works with `sql.SessionQueryTest`, this is broken in `connect.SessionQueryTest`. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? This is a test-only change ### Was this patch authored or co-authored using generative AI tooling? No Closes #57606 from fwc/fix-connect-sessionquerytest-lifecycle. Lead-authored-by: Matthis Gördel <matthis@posteo.de> Co-authored-by: Matthis Gördel <matthis.goerdel@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 56dcb54) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
Changes
connect.SparkSessionBindersetup / teardown, so that suites can usesparkinbeforeAllandafterAll.Why are the changes needed?
Some tests do setup/cleanup calls (e.g. create/delete some shared table) in
beforeAll/afterAll.While this works with
sql.SessionQueryTest, this is broken inconnect.SessionQueryTest.Does this PR introduce any user-facing change?
No
How was this patch tested?
This is a test-only change
Was this patch authored or co-authored using generative AI tooling?
No