-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[python][tests] Migrates test_basic.py to use pytest #3764
[python][tests] Migrates test_basic.py to use pytest #3764
Conversation
@thomasjpfan Thanks a lot for this PR! I'll check it a little bit later today. @jameslamb A lot of diff tools I tried sucked to produce a nice diff. But this one looks quite readable. At least it is much better than one which GitHub offers to review. Feel free to use it to save (a lot of, I believe) time 🙂 https://www.textcompare.org/index.html?id=6000f45b143e0d0017c81ac3 |
thanks so much @StrikerRUS ! I'm glad you said that, I also found GitHub's diff for this type of PR really hard to read. |
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.
Thanks so much for the help @thomasjpfan ! Awesome PR.
I reviewed this with the diff tool @StrikerRUS mentioned, and it looks great to me. I also compared the results to those on the recent jobs from master
, and confirm all the tests ran.
this PR
https://github.com/microsoft/LightGBM/pull/3764/checks?check_run_id=1702945130
====== 207 passed, 2 skipped, 2 xfailed, 72 warnings in 384.25s (0:06:24) ======
most recent tests on master
https://github.com/microsoft/LightGBM/runs/1697195023
====== 207 passed, 2 skipped, 2 xfailed, 72 warnings in 120.19s (0:02:00) ======
Do you have any idea what the tests now take 3 times as long to run? Is there some significant overhead introduced by the tmp_path
fixture? I'd be surprised to learn that, but I'm not sure what else the cause could be.
comparing total runtime for this PR to most recent master
, for the Python-package tests on GitHub Actions:
master |
this PR | |
---|---|---|
regular | 7m22s | 31m24s |
sdist | 6m35s | 9m20s |
bdist | 6m26s | 7m16s |
if-else | 3m28s | 6m40s |
mpi source | 10m45s | 12m7s |
mpi pip | 11m34s | 9m40s |
mpi wheel | 8m8s | 9m44s |
I understand that these differences could be random and not related to the changes in this PR, but I'd like to know if you have any theories.
@jameslamb
we don't run Lines 62 to 69 in a15a370
|
I reran the tests and the timings are more reasonable. My first guess would be that the environment took longer for conda to solve. |
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.
@thomasjpfan Just great! Many thanks for contribution!
I have only one non-blocking the merge question below.
|
||
def test_add_features_equal_data_on_alternating_used_unused(self): | ||
self.maxDiff = None |
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.
pytest simply doesn't need this line, right?
Ok thanks, sounds good to me! |
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.
thanks again for the help @thomasjpfan ! If you have time and interest, we'd welcome PRs for the other files that still use unittest
classes and assertions (#3732 (comment))
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
Related to #3732
This PR migrates the unitest-based test to use pytest. Additionally, this PR uses the
tmp_path
fixture when the test needs to create a temporary file.