-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-25763][SQL][PYSPARK][TEST] Use more @contextmanager to ensure clean-up each test.
#22762
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.
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
Conversation
|
Test build #97524 has finished for PR 22762 at commit
|
|
Jenkins, retest this please. |
|
cc @HyukjinKwon |
|
Test build #97526 has finished for PR 22762 at commit
|
|
I like it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if the tests pass
python/pyspark/sql/tests.py
Outdated
| """ | ||
| assert hasattr(self, "spark"), "it should have 'spark' attribute, having a spark session." | ||
|
|
||
| if len(databases) == 1 and isinstance(databases[0], (list, set)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @ueshin, how about we just allow database(a, b, ...) case only for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like .. an assert by this condition all(map(lambda d: isinstance(d, str), databases)) should be good enough.
|
Test build #97531 has finished for PR 22762 at commit
|
|
Test build #97536 has finished for PR 22762 at commit
|
|
Test build #97537 has finished for PR 22762 at commit
|
|
Merged to master. |
| def table(self, *tables): | ||
| """ | ||
| A convenient context manager to test with some specific tables. This drops the given tables | ||
| if exist when it exits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait .. not a big deal but typo: if exist when it exits..
…e clean-up each test. ## What changes were proposed in this pull request? Currently each test in `SQLTest` in PySpark is not cleaned properly. We should introduce and use more `contextmanager` to be convenient to clean up the context properly. ## How was this patch tested? Modified tests. Closes apache#22762 from ueshin/issues/SPARK-25763/cleanup_sqltests. Authored-by: Takuya UESHIN <[email protected]> Signed-off-by: hyukjinkwon <[email protected]>
What changes were proposed in this pull request?
Currently each test in
SQLTestin PySpark is not cleaned properly.We should introduce and use more
@contextmanagerto be convenient to clean up the context properly.How was this patch tested?
Modified tests.