-
Notifications
You must be signed in to change notification settings - Fork 31.7k
Fix test isolation for clear_import_cache utility #36345
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
Fix test isolation for clear_import_cache utility #36345
Conversation
dvrogozh
left a comment
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.
This PR does not work to address the reported issue #36334. Consequent tests fail as before.
$ git log -1 --oneline
c20c8466c4 (HEAD -> pr_36345) style fixes
$ python3 -m pytest tests/utils/test_import_utils.py tests/utils/test_logging.py
...
tests/utils/test_import_utils.py::test_clear_import_cache PASSED [ 14%]
tests/utils/test_logging.py::HfArgumentParserTest::test_advisory_warnings PASSED [ 28%]
tests/utils/test_logging.py::HfArgumentParserTest::test_env_invalid_override FAILED [ 42%]
tests/utils/test_logging.py::HfArgumentParserTest::test_env_override FAILED [ 57%]
tests/utils/test_logging.py::HfArgumentParserTest::test_integration PASSED [ 71%]
tests/utils/test_logging.py::HfArgumentParserTest::test_set_level PASSED [ 85%]
tests/utils/test_logging.py::test_set_progress_bar_enabled PASSED [100%]
...
@sambhavnoobcoder : I see you attached the screenshot with a single test passing - ``test_clear_import_cache`. But question is not whether this test pass or not if ran alone, but whether group of tests ran on the same command line pass. The #36334 has such a line to reproduce the issue.
ydshieh
left a comment
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.
Thank you @sambhavnoobcoder for this PR. I think it could be fixed in a very simple way by using some existing functions within the library. Could you try what I mentioned in the comments 🙏 ?
|
thank you for the comments @ydshieh . i have looked at all of them and resolved all of them . please tell me if you need anything else here . i'll make that happen as well . |
|
Thank you a lot @sambhavnoobcoder for the fix and thank @dvrogozh for opening the issue 🙏 ❤️ |
|
These changes have been made as well . i'll gladly look into any more comments on this PR if any more improvements are required . |
ydshieh
left a comment
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.
Great, and thank you again for the prompt fix ❤️

Problem
The
test_clear_import_cache()test intests/utils/test_import_utils.pyis affecting the state of subsequent tests, causing failures in the test suite. This is happening because the test modifies the global module cache without properly restoring it.Solution
Updated the test to:
clear_import_cache()functionality_LazyModuleinstancesThe core functionality of
clear_import_cache()remains unchanged - this fix only addresses test isolation.Edit : #36334
Screenshots
cc @dvrogozh