-
-
Notifications
You must be signed in to change notification settings - Fork 33k
gh-139322: Create test_os package #139323
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
Move test_os.py and test_posix.py into a new test_os package.
Fix test_fs_holes(): use "w+b" mode, and delete the file once done.
Sort also imports.
I consider that the PR is now ready for a 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.
Overall looks good to me. If there's any new changes in test_os
or test_posix
after the delete + make split will need to make sure to incorporate those changes (Git sees file removed rather than file moved when doing these splits).
How would you suggest reviewing? (this is a big diff!) Should we verify that no test coverage was lost etc? A |
Do you like how tests are grouped? test_xxx filename and content? |
My PR was the impetus for this change, so it seemed appropriate that I scroll through it, but it's a large diff and I'm not familiar with most of the tests, so my review shouldn't count for much. I didn't see anything technically wrong with the implementation. The test groups in the files seem reasonable to me. Many of the files end with a I think running the test coverage utilities on all the supported platforms would be useful to check that no coverage was lost. Is there a CI action for that? The blank line spacing in the created files is inconsistent. Some classes have blank lines between the header and the first method, while others don't. Most classes are separated from the next by two lines, but some have three. I don't consider this important, but I don't maintain the tests. :) |
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.
I think splitting tests into separate files for each function is too much. We have separate classes for grouping.
I see only one natural way to split the tests -- for functions implemented in C (in posix
/nt
modules) and pure Python functions defined in the os
module. Maybe move some Windows and macOS specific tests into separate files -- if we are sure that they will never be relevant on other platforms.
I tried to have around 1,000 lines per Python file:
In practice, files are between 100 and 500 lines except of test_windows, test_dirs, test_file and test_process (which are between 690 and 1,033 lines).
I don't think that we can make test_os and test_posix under 1,000 lines by the way.
test_windodws contain all tests which are specific to Windows. |
@serhiy-storchaka is against this split, so I created #139453 which only creates test_windows. |
Follow-up: I created #139480 to create |
Move test_os.py and test_posix.py into a new test_os package.