Skip to content

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

Merged
merged 2 commits into from
Mar 24, 2020

Conversation

rth
Copy link
Contributor

@rth rth commented Mar 21, 2020

@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Mar 21, 2020
@rth rth changed the title FIX mark test_importlib.test_zip as requiring zlib bpo-40029 mark test_importlib.test_zip as requiring zlib Mar 21, 2020
Copy link
Member

@tirkarthi tirkarthi left a 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

@rth rth force-pushed the fix-requires-zlib branch from 0de1e33 to 4efb4f7 Compare March 22, 2020 14:33
@rth
Copy link
Contributor Author

rth commented Mar 22, 2020

Thanks for double checking @tirkarthi! I updated the marked tests as suggested.

@brettcannon brettcannon requested a review from jaraco March 23, 2020 23:30
@jaraco
Copy link
Member

jaraco commented Mar 24, 2020

test_missing_metadata doesn't need to be marked since it passes without zlib. Log of my test output as below

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?

Copy link
Member

@jaraco jaraco left a 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.

@tirkarthi
Copy link
Member

Yes, decorators work skipping the classes. I felt it might unintentionally skip tests that don't require zlib too and hence listed the tests.

@rth
Copy link
Contributor Author

rth commented Mar 24, 2020

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 jaraco merged commit 15e5024 into python:master Mar 24, 2020
@bedevere-bot
Copy link

@jaraco: Please replace # with GH- in the commit message next time. Thanks!

@jaraco
Copy link
Member

jaraco commented Mar 24, 2020

Looks great. Thanks!

@rth rth deleted the fix-requires-zlib branch June 18, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants