Skip to content

Invoking tests against editable installs have been masking bugs #8854

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

Closed
jacobtylerwalls opened this issue Jul 14, 2023 · 1 comment · Fixed by #8878
Closed

Invoking tests against editable installs have been masking bugs #8854

jacobtylerwalls opened this issue Jul 14, 2023 · 1 comment · Fixed by #8878
Labels
Bug 🪲 Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Jul 14, 2023

We've been invoking tests on CI against an editable install of pylint for a long time. Doing so has the potential to mask issues. See blog post.

We now have evidence of some of those issues from the failures in the PR testing against Python 3.12, which for unrelated reasons, forced us to upgrade setuptools. Without using the legacy behavior, bugs became evident.

One of the most minimal bugs is this one:

with mock.patch(
"pylint.lint.PyLinter.check_astroid_module", side_effect=RuntimeError
):

That mock shouldn't work, since the real location of the PyLinter class is at pylint.lint.pylinter.PyLinter. It was only working because the pylint.lint module imports PyLinter, but AFIAU there's no reason to expect that to work when testing the installation of the pylint package, which won't install pylint.lint (whereas an editable install will, assuming you're invoking tests from pylint.

That's just a bug in a test, but there are functional bugs, too, such as the moved check in #8702 potentially crashing:

File "/Users/<user>/pylint/pylint/config/config_initialization.py", line 114, in _config_initialization
    for exc_name in linter.config.overgeneral_exceptions:
AttributeError: 'Namespace' object has no attribute 'overgeneral_exceptions'

I've found a workaround for the 3.12 branch to keep that one using legacy behavior, but suggesting we also look at this fixing this for the next pylint alpha.

@jacobtylerwalls jacobtylerwalls added Bug 🪲 Crash 💥 A bug that makes pylint crash Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much labels Jul 14, 2023
@jacobtylerwalls jacobtylerwalls added this to the 3.0.0a7 milestone Jul 14, 2023
@jacobtylerwalls jacobtylerwalls added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Jul 14, 2023
@jacobtylerwalls
Copy link
Member Author

This is no longer as crash-y as before. I haven't worked out when or why, but it's still worth doing.

@jacobtylerwalls jacobtylerwalls removed Crash 💥 A bug that makes pylint crash Beta-Blocker 🙅🩸 If this issue is not fixed before the beta release, our blood pressure increase too much labels Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant