Fix two incorrect/broken tests in tests/checkers/unittest_imports.py

[akamat10]

Type of Changes

	Type
✓	🔨 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]

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

[Pierre-Sassoulas]

Thank you for the pull request ! sys.path is tricky to test and can cause massive problem (with namespace packages in particular), so let's be cautious before modifying it.

[Pierre-Sassoulas]

Why is the context file necessary and doesn't it change what we're testing ?

[akamat10]

Context file would be used to specify that the file where the import statement for the module. I was trying to understand the intent of the code and the test and trying to guess what we are trying to achieve. The existing test only works if the search directory is in the sys.path. If we want this test to succeed without changing sys.path, then I think we need to change (fix?) the find_spec in astroid.interpreter._import.spec module. It is not considering the directory we are passing to it. Shall I attempt to push a fix and see if it works?

I also looked at the tests in test_manager.py in the astroid repo. There is no test for ast_from_module when the second argument is specified.

[DanielNoord]

These changes make sense to me actually.

@Pierre-Sassoulas What do you think?

[akamat10]

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]

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]

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]

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]

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

@Pierre-Sassoulas Shall we merge this?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9911   +/-   ##
=======================================
  Coverage   95.80%   95.80%           
=======================================
  Files         174      174           
  Lines       18935    18935           
=======================================
  Hits        18141    18141           
  Misses        794      794           

[Pierre-Sassoulas]

Thank you @akamat10 !