Skip to content

bpo-39299: Add more tests for mimetypes and its cli. #17949

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jan 13, 2020
Merged

bpo-39299: Add more tests for mimetypes and its cli. #17949

merged 8 commits into from
Jan 13, 2020

Conversation

tirkarthi
Copy link
Member

@tirkarthi tirkarthi commented Jan 11, 2020 •
edited by bedevere-bot
Loading

  • Add test for case insensitive check of types and extensions.
  • Add test for data url with no comma.
  • Add test for read_mime_types.
  • Add tests for the mimetypes cli.

https://bugs.python.org/issue39299

@tirkarthi tirkarthi added tests Tests in the Lib/test dir skip news labels Jan 11, 2020
@tirkarthi tirkarthi requested a review from a team as a code owner January 11, 2020 06:07
@tirkarthi
Copy link
Member Author

Ah, the logic to load the initial database is with known types, common types and the list of files from mimetypes.knownfiles subsequently overriding each other. At the start of the test mimetypes.knownfiles is set as empty list but since I am using script_helper.assert_python_ok setting the list of knownfiles to empty list doesn't work and mac seems to load a mimetype for .pic that is different from common types and overrides it.

I guess my tests can be rewritten to make sure the tests are not machine dependent based on the files.

@tirkarthi
Refactor test setup and main call for mimetypes.
a572c09 
* Set knownfiles for mimetypes to be empty list only during setup so that it doesn't leak into other tests.
* Move the __main__ code to separate function for better testing.
@tirkarthi
Copy link
Member Author

I have moved the code for __main__ to be a private function to help testing by directly calling it instead of using subprocess to invoke python -m mimetypes. I have also found that mimetypes.knownfiles was set as empty at the top of the module. It has a potential side effect that all other tests that follow after test_mimetypes will also have empty list of files when accessing mimetypes.knownfiles. I have moved it to setup call so that this change doesn't affect other tests.

@tirkarthi
Copy link
Member Author

I am slightly confused since importing mimetypes itself sets inited as False and also calls _default_mime_types to set global values so I removed them from the test since it's already done. In the test when I call the _default_mime_types which sets the global variables the value for suffix, encoding and types maps it poses a strange failure for windows test which copies the types_map and clears it to reinitialize it again where _types_map[True] is empty. I need to figure out yet over why the call has a side effect.

@tirkarthi
Copy link
Member Author

I resorted to using setUpModule and tearDownModule to have the original test code and not to use mixins for the setup. This will restore knownfiles value in the end. The test coverage with this PR stands at 80% with the code for windows not as part of the coverage report.

maxking
maxking approved these changes Jan 12, 2020
View reviewed changes
Copy link
Contributor

@maxking maxking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @tirkarthi

@tirkarthi tirkarthi merged commit d8efc14 into python:master Jan 13, 2020
@tirkarthi tirkarthi deleted the test-cases-mimetypes branch January 13, 2020 14:39
sthagen added a commit to sthagen/python-cpython that referenced this pull request Jan 13, 2020
@sthagen
Merge pull request #174 from python/master
b21fcff 
bpo-39299: Add more tests for mimetypes and its cli. (pythonGH-17949)
@tirkarthi
Copy link
Member Author

Thanks @maxking and @asvetlov for the review. I would like to backport this PR to 3.8 and 3.7. It moves code under __main__ to _main but could help in catching regressions if any. Thoughts?

@asvetlov
Copy link
Contributor

The change is safe to backport; please feel free to do it if you want.
Start from adding needs backport to xx labels here, the bot does the rest and commits the backport after approval if everything goes smoothly.

@tirkarthi
Copy link
Member Author

Thanks Andrew, I will add the labels and wait for the CI.

@miss-islington
Copy link
Contributor

Thanks @tirkarthi for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @tirkarthi for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 13, 2020
@tirkarthi @miss-islington
bpo-39299: Add more tests for mimetypes and its cli. (pythonGH-17949)
be748ee 
* Add tests for case insensitive check of types and extensions as fallback.
* Add tests for data url with no comma.
* Add tests for read_mime_types.
* Add tests for the mimetypes cli and refactor __main__ code to private function.
* Restore mimetypes.knownfiles value at the end of the test.
(cherry picked from commit d8efc14)

Co-authored-by: Karthikeyan Singaravelan <[email protected]>
@bedevere-bot
Copy link

GH-17991 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 13, 2020
@tirkarthi @miss-islington
bpo-39299: Add more tests for mimetypes and its cli. (pythonGH-17949)
094bd63 
* Add tests for case insensitive check of types and extensions as fallback.
* Add tests for data url with no comma.
* Add tests for read_mime_types.
* Add tests for the mimetypes cli and refactor __main__ code to private function.
* Restore mimetypes.knownfiles value at the end of the test.
(cherry picked from commit d8efc14)

Co-authored-by: Karthikeyan Singaravelan <[email protected]>
@bedevere-bot
Copy link

GH-17992 is a backport of this pull request to the 3.8 branch.

@ned-deily
Copy link
Member

I would prefer that a change like this not be backported to 3.7 at this stage of its lifecycle. It's not fixing any problems and, at worst, could add new ones.

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@tirkarthi @shihai1991
bpo-39299: Add more tests for mimetypes and its cli. (pythonGH-17949)
c1e5be0 
* Add tests for case insensitive check of types and extensions as fallback.
* Add tests for data url with no comma.
* Add tests for read_mime_types.
* Add tests for the mimetypes cli and refactor __main__ code to private function.
* Restore mimetypes.knownfiles value at the end of the test.
miss-islington added a commit that referenced this pull request Feb 11, 2020
@miss-islington @tirkarthi
bpo-39299: Add more tests for mimetypes and its cli. (GH-17949)
d3f9fb2 
* Add tests for case insensitive check of types and extensions as fallback.
* Add tests for data url with no comma.
* Add tests for read_mime_types.
* Add tests for the mimetypes cli and refactor __main__ code to private function.
* Restore mimetypes.knownfiles value at the end of the test.
(cherry picked from commit d8efc14)

Co-authored-by: Karthikeyan Singaravelan <[email protected]>
@tzabal tzabal mentioned this pull request Apr 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asvetlov asvetlov asvetlov approved these changes

@maxking maxking maxking approved these changes

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tirkarthi @asvetlov @miss-islington @bedevere-bot @ned-deily @maxking @the-knights-who-say-ni