-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-40029 mark test_importlib.test_zip as requiring zlib #19105
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
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.
test_files under TestEgg also needs to be marked. test_missing_metadata
doesn't need to be marked since it passes without zlib. Log of my test output as below :
test_case_insensitive (test.test_importlib.test_zip.TestEgg) ... ERROR
test_files (test.test_importlib.test_zip.TestEgg) ... ERROR
test_missing_metadata (test.test_importlib.test_zip.TestEgg) ... ok
test_zip_entry_points (test.test_importlib.test_zip.TestEgg) ... ERROR
test_zip_version (test.test_importlib.test_zip.TestEgg) ... ERROR
test_zip_version_does_not_match (test.test_importlib.test_zip.TestEgg) ... ok
test_case_insensitive (test.test_importlib.test_zip.TestZip) ... ERROR
test_files (test.test_importlib.test_zip.TestZip) ... ERROR
test_missing_metadata (test.test_importlib.test_zip.TestZip) ... ok
test_zip_entry_points (test.test_importlib.test_zip.TestZip) ... ERROR
test_zip_version (test.test_importlib.test_zip.TestZip) ... ERROR
test_zip_version_does_not_match (test.test_importlib.test_zip.TestZip) ... ok
Thanks for double checking @tirkarthi! I updated the marked tests as suggested. |
My instinct would be to skip all of the tests in the file. Even though they don't fail, their assertions aren't being adequately exercised. I don't feel strongly about it, especially since each test has to be marked individually. If it were possible to mark the whole file for skip (such as you can with pytest), I'd recommend that. I don't see that there's such a feature in unittest. Does it work if you decorate the two classes instead? |
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'm happy with this regardless of how it's implemented.
Yes, decorators work skipping the classes. I felt it might unintentionally skip tests that don't require zlib too and hence listed the tests. |
Thanks for the feedback. I reverted to decorating both classes as suggested. Even if two tests pass without zlib, they still apply to the zip module which generally doesn't work without zlib. So I'm not sure that not skipping them provides much relevant information indeed. Though I can go back to the previous solution if it has more consensus. |
@jaraco: Please replace |
Looks great. Thanks! |
https://bugs.python.org/issue40029