Fix test imports

[nickdrozd]

Type of Changes

	Type
✓	🔨 Refactoring

Description

Trying to run pytest locally without git package installed raises ModuleNotFoundError, even though primer is not run locally. Similar for pytest_benchmark.

This MR delays imports from git until they are actually used. The pytest_benchmark import was only used for typechecking, so it is pushed under TYPE_CHECKING flag. (This is yet another benefit to separating typechecking imports from runtime imports.)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.80%. Comparing base (c0ecd70) to head (d53332c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9982   +/-   ##
=======================================
  Coverage   95.80%   95.80%           
=======================================
  Files         174      174           
  Lines       18934    18934           
=======================================
  Hits        18140    18140           
  Misses        794      794           

Files with missing lines	Coverage Δ
pylint/testutils/pyreverse.py	98.11% <100.00%> (+0.03%)	⬆️
pylint/typing.py	100.00% <ø> (ø)

[nickdrozd]

Seems to work okay, but I guess the primer files aren't covered? Is there a way to skip the coverage check for this one?

[Pierre-Sassoulas]

I don't think there's anything to fix, this is due to pylint not being installed properly:

pylint/pyproject.toml

Line 55 in c0ecd70

testutils = ["gitpython>3"]

A possible solution would be to move the test utils in another package entirely, the name is reserved https://pypi.org/project/pylint-testutils/.

[nickdrozd]

Okay, I installed the testutils and the Git imports works fine. The Pytest benchmark still isn't installed though, so running pytest crashes immediately with an import error. I don't know if that's something that needs to be fixed, but the typechecking import can be moved either way.

I dropped the Git import commit and replaced it with an unrelated change that moves GetProjectCallable type to testutils.

[Pierre-Sassoulas]

The typing being all guarded is not consensual among pylint core maintainers (there's an ongoing discussion between you and Marc about this), let's not change anything before it is resolved.

Also I'm not sure what the issue is, there's a test requirements file that you need to pip install -r if you want to launch the tests locally ?

pylint/requirements_test_min.txt

Line 7 in c0ecd70

pytest-benchmark~=4.0

https://pylint.readthedocs.io/en/stable/development_guide/contributor_guide/tests/install.html#basic-installation

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit d53332c

[nickdrozd]

My understanding is that the concern is with using the type guard project-wide. Is there an objection to using the type guard in each and every case? For this particular import, using the guard provides a clear material benefit: it allows running tests without all the test dependencies installed.

[Pierre-Sassoulas]

There's a clear benefit to installing the test requirements from the test requirements file: you're not going to run into strange issue because you have an out of date dependency and we won't have to complexify the code to accomodate partially installed tests requirements.

Also introducing unneeded typing guards MR by MR is not very different from introducing them all at once. The discussion still need to happens. I found the 'this is going to create churn' argument convincing and imo the 'default' at the moment is to not have typing guard unless necessary so if Marc thinks strongly that it's not desirable I would be in favor of not changing anything unless we have strong arguments in favor (like major performance improvment, or it becoming the standard).