pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.32k stars 1.14k forks source link

Fix two incorrect/broken tests in `tests/checkers/unittest_imports.py` #9911

Closed akamat10 closed 1 month ago

akamat10 commented 2 months ago

Type of Changes

Type
:hammer: Refactoring

Description

This PR addresses an issue with two tests (test_wildcard_import_init and test_wildcard_import_non_init) in tests/checkers/unittest_imports.py that I discovered while attempting to fix #9883.

The issue with the tests is that the tests are invalid and only work due to modified sys.path in https://github.com/pylint-dev/pylint/blob/main/tests/pyreverse/conftest.py#L70. This can be reproduced by running the test on only the file:

tox -e py312 -- tests/checkers/unittest_imports.py

the above command fails and I see the following:

FAILED tests/checkers/unittest_imports.py::TestImportsChecker::test_wildcard_import_init - AssertionError: Expected messages did not match actual.
FAILED tests/checkers/unittest_imports.py::TestImportsChecker::test_wildcard_import_non_init - AssertionError: Expected messages did not match actual.

The fix is to augment the sys.path as part of the test.

Refs #9883

akamat10 commented 2 months ago

Not sure if changlog should be updated in this situation. Let me know.

akamat10 commented 2 months ago

If you see the search code here, even though search_path is passed in the previous line to fetch the finder, it doesn't get used as part of the search. This is what the above tests perhaps thought should be used. Should that be fixed?

Pierre-Sassoulas commented 2 months ago

I also think it make sense but I don't understand a lot :D feel free to push the fix you envision @akamat10 , it'll help us (me) understand better imo

akamat10 commented 2 months ago

Ok I tried to get _find_spec_with_path to respect the search_path. The fix might be more involved than I thought if we don't want to rely on sys.path for the above test. That file has some implicit dependencies on sys.path in a few places that are going to be tricky to unravel. I think changing the sys.path like above is the easiest thing for now to fix the tests.

akamat10 commented 2 months ago

I took a look at the tests within astroid repo. I see quite a few modify sys.path before calling ast_from_module_name. One such example is in test_manager.py here. Passing in a directory to ast_from_module_name doesn't work (or even file I think doesn't work). Only way is through sys.path modification. May be the documentation for that function should be updated saying it is broken (or even simplify and change the api to remove the context_file argument). The above change I have pushed for this PR can be accepted I think based on my analysis.

DanielNoord commented 2 months ago

@akamat10 Could you rebase this so the coverage job can pass?

@Pierre-Sassoulas Shall we merge this?

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.80%. Comparing base (f448dca) to head (96510ad). Report is 13 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pylint-dev/pylint/pull/9911/graphs/tree.svg?width=650&height=150&src=pr&token=ZETEzayrfk&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pylint-dev)](https://app.codecov.io/gh/pylint-dev/pylint/pull/9911?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pylint-dev) ```diff @@ Coverage Diff @@ ## main #9911 +/- ## ======================================= Coverage 95.80% 95.80% ======================================= Files 174 174 Lines 18935 18935 ======================================= Hits 18141 18141 Misses 794 794 ```