-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-35721][PYTHON] Path level discover for python unittests #32867
Conversation
cc @HyukjinKwon @xinrong-databricks It would be good if you could give some inputs on this. |
Test build #139642 has finished for PR 32867 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status success |
be3877a
to
88b252d
Compare
88b252d
to
5a7e6e2
Compare
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #139670 has finished for PR 32867 at commit
|
Test build #139667 has finished for PR 32867 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #139791 has finished for PR 32867 at commit
|
Test build #139854 has finished for PR 32867 at commit
|
eb835c5
to
9f4388a
Compare
Test build #139862 has finished for PR 32867 at commit
|
9f4388a
to
22015a5
Compare
Test build #139865 has finished for PR 32867 at commit
|
Kubernetes integration test starting |
BTW, this change will likely affect many other vendors who maintain their forks so I will take a close look few more times. Thanks for bearing with me in advance .. ;-). |
Sure, thanks for your patient review! |
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.
Looks okay.
@Yikun sorry but mind resolving the conflicts please? |
Looks pretty good to me too. |
Just wondering we can only verify it manually? If by any chance, some tests are not found, can we easily know it? |
It just same as before, if we forgot to add the path of tests, these tests would not be ran. But compare to original implementations, the path level discover need less redundant work to add the test file name one by one, and also it decrease the possibility of forgetting to add test on some level. |
Merged to master, thanks @Yikun |
I'm sorry the the late review, but is this triggering unit tests? I don't see any test cases running.
|
Ur, this might be what I said before, how we make sure the tests are run... @Yikun Can you check it again? |
Let me revert this for now. |
Uhoh, thanks @ueshin for reverting this. |
Thanks for revert for it. I'm so sorry for the trouble, I will recheck and fix soon. |
### What changes were proposed in this pull request? Add path level discover for python unittests. ### Why are the changes needed? Now we need to specify the python test cases by manually when we add a new testcase. Sometime, we forgot to add the testcase to module list, the testcase would not be executed. Such as: - pyspark-core pyspark.tests.test_pin_thread Thus we need some auto-discover way to find all testcase rather than specified every case by manually. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add below code in end of `dev/sparktestsupport/modules.py` ```python for m in sorted(all_modules): for g in sorted(m.python_test_goals): print(m.name, g) ``` Compare the result before and after: https://www.diffchecker.com/iO3FvhKL Closes apache#32867 from Yikun/SPARK_DISCOVER_TEST. Authored-by: Yikun Jiang <yikunkero@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
The root reason of failed to discover is that the deps of PySpark modules is not installed, so we get the wrong list when we do discover. I re-propose it in #33174 , the mainly changed:
Thank you for your patience. I will fight this test discover to the end to make up for my mistake. : ) |
What changes were proposed in this pull request?
Add path level discover for python unittests.
Why are the changes needed?
Now we need to specify the python test cases by manually when we add a new testcase. Sometime, we forgot to add the testcase to module list, the testcase would not be executed.
Such as:
Thus we need some auto-discover way to find all testcase rather than specified every case by manually.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add below code in end of
dev/sparktestsupport/modules.py
Compare the result before and after:
https://www.diffchecker.com/iO3FvhKL