Add `--minimal-messages-config` option for functional tests #6246

DudeNr33 · 2022-04-10T11:25:58Z

Add a ChangeLog entry describing what your PR does.
If it's a new feature, or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.
If you used multiple emails or multiple names when contributing, add your mails
and preferred name in script/.contributors_aliases.json

Type of Changes

	Type
✓	✨ New feature

Description

Inspired by @DanielNoord's approach in test_messages_documentation.py.
This should be our best friend in finding false negatives due to wrong usage of @check_messages.

I added a pytest option called --minimal-messages-config. If enabled, all messages that are not explicitly defined as expected with the # [<msgid>] syntax are disabled.

However, we can't (yet) make this the default behaviour, even if we fixed all @check_messages decorators:

Some functional tests only exist to prove there are no false positives. See abstract_abc_methods.py for example.
Besides misplaced @check_messages decorators another root cause for false negatives are cases where a checker class reuses a message of another checker class.

I ran the functional tests and ended up with 32 errors (vs ~700 passing tests, so it's not that bad).

==================================================== short test summary info ====================================================
FAILED tests/test_functional.py::test_functional[typing_deprecated_alias] - AssertionError: Wrong results for file "typing_dep...
FAILED tests/test_functional.py::test_functional[mccabe] - AssertionError: Wrong results for file "mccabe":
FAILED tests/test_functional.py::test_functional[renamed_import_logging_not_lazy] - AssertionError: Wrong output for 'renamed_...
FAILED tests/test_functional.py::test_functional[regression_4680] - AssertionError: Wrong results for file "regression_4680":
FAILED tests/test_functional.py::test_functional[ungrouped_imports_suppression] - AssertionError: Wrong results for file "ungr...
FAILED tests/test_functional.py::test_functional[useless_suppression] - AssertionError: Wrong results for file "useless_suppre...
FAILED tests/test_functional.py::test_functional[use_a_generator] - AssertionError: Wrong results for file "use_a_generator":
FAILED tests/test_functional.py::test_functional[undefined_variable_py30] - AssertionError: Wrong results for file "undefined_...
FAILED tests/test_functional.py::test_functional[nan_comparison_check] - AssertionError: Wrong results for file "nan_compariso...
FAILED tests/test_functional.py::test_functional[non_ascii_name_lo\u0142] - AssertionError: Wrong results for file "non_ascii_...
FAILED tests/test_functional.py::test_functional[non_ascii_import_from_as] - AssertionError: Wrong results for file "non_ascii...
FAILED tests/test_functional.py::test_functional[non_ascii_import_as_bad] - AssertionError: Wrong results for file "non_ascii_...
FAILED tests/test_functional.py::test_functional[try_except_raise_crash] - AssertionError: Wrong results for file "try_except_...
FAILED tests/test_functional.py::test_functional[too_many_branches] - AssertionError: Wrong results for file "too_many_branches":
FAILED tests/test_functional.py::test_functional[async_functions] - AssertionError: Wrong results for file "async_functions":
FAILED tests/test_functional.py::test_functional[access_member_before_definition] - AssertionError: Wrong results for file "ac...
FAILED tests/test_functional.py::test_functional[fixme] - AssertionError: Wrong results for file "fixme":
FAILED tests/test_functional.py::test_functional[consider_using_with_open] - AssertionError: Wrong results for file "consider_...
FAILED tests/test_functional.py::test_functional[consider_using_with] - AssertionError: Wrong results for file "consider_using...
FAILED tests/test_functional.py::test_functional[consider_using_min_max_builtin] - AssertionError: Wrong results for file "con...
FAILED tests/test_functional.py::test_functional[deprecated_module_py33] - AssertionError: Wrong results for file "deprecated_...
FAILED tests/test_functional.py::test_functional[deprecated_module_py36] - AssertionError: Wrong results for file "deprecated_...
FAILED tests/test_functional.py::test_functional[deprecated_module_py39] - AssertionError: Wrong results for file "deprecated_...
FAILED tests/test_functional.py::test_functional[deprecated_module_py39_earlier_pyversion] - AssertionError: Wrong results for...
FAILED tests/test_functional.py::test_functional[deprecated_module_py310] - AssertionError: Wrong results for file "deprecated...
FAILED tests/test_functional.py::test_functional[dot_relative_import] - AssertionError: Wrong results for file "dot_relative_i...
FAILED tests/test_functional.py::test_functional[dot_dot_relative_import] - AssertionError: Wrong results for file "dot_dot_re...
FAILED tests/test_functional.py::test_functional[logging_format_interpolation_py36] - AssertionError: Wrong output for 'loggin...
FAILED tests/test_functional.py::test_functional[logging_fstring_interpolation_py36] - AssertionError: Wrong output for 'loggi...
FAILED tests/test_functional.py::test_functional[logging_not_lazy_module] - AssertionError: Wrong output for 'logging_not_lazy...
FAILED tests/test_functional.py::test_functional[logging_fstring_interpolation_py37] - AssertionError: Wrong output for 'loggi...
FAILED tests/test_functional.py::test_functional[logging_not_lazy_with_logger] - AssertionError: Wrong output for 'logging_not...
========================================== 32 failed, 697 passed, 23 skipped in 22.72s ==========================================

Ref #6060

coveralls · 2022-04-10T11:31:04Z

Pull Request Test Coverage Report for Build 2172399998

16 of 16 (100.0%) changed or added relevant lines in 7 files are covered.
161 unchanged lines in 15 files lost coverage.
Overall coverage increased (+1.6%) to 94.998%

Files with Coverage Reduction	New Missed Lines	%
pylint/extensions/code_style.py	1	99.14%
pylint/pyreverse/writer.py	1	98.55%
pylint/checkers/design_analysis.py	2	98.91%
pylint/config/arguments_manager.py	2	98.63%
pylint/pyreverse/main.py	3	92.11%
pylint/extensions/docparams.py	4	97.94%
pylint/extensions/typing.py	4	97.06%
pylint/checkers/refactoring/recommendation_checker.py	5	96.28%
pylint/checkers/base/basic_error_checker.py	6	95.45%
pylint/checkers/base/basic_checker.py	7	97.94%

Totals
Change from base Build 2171728652:	1.6%
Covered Lines:	15611
Relevant Lines:	16433

💛 - Coveralls

Pierre-Sassoulas

Imo this is a pretty great tool to find issues with 'check_message'. We could even add one job for it in continuous integration if we add a "expected to fail" list.

DudeNr33 · 2022-04-10T14:14:30Z

We can expand pylint.testutils.functional.test_file.TestFileOptions with exclude_from_minimal_message_config.
That way we don't have to maintain a hardcoded list, and it is still easy to find all functional tests that currently have issues.

This way we could integrate this check in the CI right away, probably directly with this PR.

DudeNr33 · 2022-04-10T18:24:34Z

I added the test file option.
Next I will go through the currently failing tests and check whether it makes more sense to directly fix the problem or use the new exclude option first.
If the fix just requires adding the message id to a check_messages decorator, it probably doesn't matter much in terms of reviewing, so I'd prefer to fix it right away.

… false negatives

…` run. Have to find out how the list of deprecated modules is built first.

DanielNoord · 2022-04-11T09:01:34Z

Thanks for working on this @DudeNr33!

doc/whatsnew/2.14.rst

…DudeNr33/pylint into functional-tests-enable-disable

…ffects TokenChecker/RawChecker

…ing depends on what messages are enabled.

… is ``False`` when enabling just this message.

… triggered too often in this case.

…-messages-config``

DudeNr33 · 2022-04-15T10:00:08Z

This should be ready for review now.
As the fixes and excludes now already account for a large number of changed files, I'd suggest to do the integration of this test into the CI in a separate PR.

Most fixes were trivial.
The excluded tests can be grouped as follows:

Logging-related checks, where the text output differs based on what messages are enabled (ref: LoggingChecker._helper_string(self, node))
Tests for deprecated-xy - I haven't figured out how enabling/disabling messages affect the list of modules marked as deprecated
Tests for fixme - the EncodingChecker does not implement IAstroidChecker, so I it can't be affected by a misplaced check_messages decorator. Haven't found out yet what the problem is.
Functional tests that check for useless-suppression - of course this message will be emitted more often if we already run with most messages disabled

Pierre-Sassoulas

This is a pretty great addition to our test suite. I'm wondering where exactly we're going to launch it as I don't see a new job in the CI for it ?

tests/testutils/data/m/minimal_messages_config.py

DanielNoord · 2022-04-15T11:21:41Z

tests/functional/d/deprecated/deprecated_module_py3.rc

@@ -1,2 +1,5 @@
 [typing]
 py-version=3.6
+
+[testoptions]
+exclude_from_minimal_messages_config=true


Just wondering, should we do this the other way around: allowing certain tests to be "optimised" by this option? Or would that be too much work?

No, I would leave it this way:
This check makes (imo) a lot of sense, and it should be enabled by default (once we found a suitable place for it in the CI).
Being able to check for just a single message explicitly is not an optimisation for me, but part of "correct functional behaviour". Doing this is even mentioned in our docs.

Just to be sure, we're also testing that the message is emitted when everything is activated like we did before ?

Yes. This option is not enabled by default. Right now, the CI doesn't use it in any way yet.
We should add a separate test job for it, because testing for false positives could be tricky if we made this the default behavior.

That's why I wonder if we should make this opt-in instead of opt-out. That way we can just enable this in the general CI job and opt tests in whenever we want to?

I think the default use of pylint is to check all messages so opt out is the best way to add --minimal-message-config imo.

Daniel, maybe we are talking about different things?
Right now, all test runs will behave exactly the same as before. Only if you run pytest --minimal-messages-config tests/test_functional.py, all messages except the ones which are checked by the test are disabled.

Ideally, one day we can run all of the functional tests once with all messages enabled and once with only the specific message enabled. This way we can check both: undesired side effects from code executed for other messages, and undesired side effects due to code not being run because messages are disabled.
Both could lead to bugs.

Hm okay. Yeah I was thinking of one day running just once with this flag enabled, but the way you put it it might actually be better to do both. Then this system is fine!

Co-authored-by: Pierre Sassoulas <[email protected]>

Add --minimal-messages-config option for functional tests

ed6f185

Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Apr 10, 2022

Pierre-Sassoulas added this to the 2.14.0 milestone Apr 10, 2022

Pierre-Sassoulas reviewed Apr 10, 2022

View reviewed changes

Add test file option to exclude from --minimal-messages-config run.

594a4c0

DudeNr33 added 5 commits April 10, 2022 20:26

Fix too-many-branches false negative

b8db668

Fix access-member-before-definition false negative

b5305f1

Fix consider-using-min-builtin and consider-using-max-builtin…

edd26d8

… false negatives

Fix consider-using-with false negative

a2d301b

Exclude deprecated-module tests from `--minimal-messages-config…

96b69a6

…` run. Have to find out how the list of deprecated modules is built first.

Merge branch 'main' into functional-tests-enable-disable

7352e80

Pierre-Sassoulas reviewed Apr 11, 2022

View reviewed changes

doc/whatsnew/2.14.rst Outdated Show resolved Hide resolved

Pierre-Sassoulas and others added 14 commits April 13, 2022 10:12

Update doc/whatsnew/2.14.rst

0c2832f

Merge branch 'functional-tests-enable-disable' of https://github.com/…

9660932

…DudeNr33/pylint into functional-tests-enable-disable

Ignore deprecated module false negatives

fabc645

Fix deprecated-typing-alias false negative

e04af26

Merge branch 'main' into functional-tests-enable-disable

d8a4105

Merge branch 'functional-tests-enable-disable' of https://github.com/…

05fdb48

…DudeNr33/pylint into functional-tests-enable-disable

Skip tests for fixme message, don't know yet how enable/disable a…

0281388

…ffects TokenChecker/RawChecker

Exclude functional test for various logging checks, as the output str…

9dbb41b

…ing depends on what messages are enabled.

Fix false negative for nan-comparison

1b68c3c

Fix false negative for non-ascii-module-import

a3f5da4

Fix false negative for non-ascii-file-name

9f0a671

Skip tests for logging messages

aecf171

Fix false negative for undefined-variable in metaclass argument

4f6d88f

Exclude test for catching-non-exception. exc._all_bases_known…

5dfec39

… is ``False`` when enabling just this message.

DudeNr33 added 4 commits April 15, 2022 11:37

Exclude ungrouped_imports_suppression. useless-suppression is…

43ba199

… triggered too often in this case.

Fix false negative for use-a-generator

de44dbf

Exclude functional tests for useless-suppression from ``--minimal…

9d0db0f

…-messages-config``

Exclude another deprecation check from --minimal-messages-config run

b96c7c7

Pierre-Sassoulas reviewed Apr 15, 2022

View reviewed changes

tests/testutils/data/m/minimal_messages_config.py Outdated Show resolved Hide resolved

DanielNoord reviewed Apr 15, 2022

View reviewed changes

Update tests/testutils/data/m/minimal_messages_config.py

69626c0

Co-authored-by: Pierre Sassoulas <[email protected]>

Pierre-Sassoulas approved these changes Apr 15, 2022

View reviewed changes

DudeNr33 merged commit ebf7bd6 into pylint-dev:main Apr 16, 2022

DudeNr33 mentioned this pull request Apr 17, 2022

Integrate --minimal-messages-config functional tests in CI #6370

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--minimal-messages-config` option for functional tests #6246

Add `--minimal-messages-config` option for functional tests #6246

DudeNr33 commented Apr 10, 2022

coveralls commented Apr 10, 2022 •

edited

Loading

Pierre-Sassoulas left a comment

DudeNr33 commented Apr 10, 2022

DudeNr33 commented Apr 10, 2022

DanielNoord commented Apr 11, 2022

DudeNr33 commented Apr 15, 2022

Pierre-Sassoulas left a comment

DanielNoord Apr 15, 2022

DudeNr33 Apr 15, 2022

Pierre-Sassoulas Apr 15, 2022

DudeNr33 Apr 15, 2022

DanielNoord Apr 15, 2022

Pierre-Sassoulas Apr 15, 2022

DudeNr33 Apr 15, 2022

DanielNoord Apr 15, 2022

Add --minimal-messages-config option for functional tests #6246

Add --minimal-messages-config option for functional tests #6246

Conversation

DudeNr33 commented Apr 10, 2022

Type of Changes

Description

coveralls commented Apr 10, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2172399998

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DudeNr33 commented Apr 10, 2022

DudeNr33 commented Apr 10, 2022

DanielNoord commented Apr 11, 2022

DudeNr33 commented Apr 15, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `--minimal-messages-config` option for functional tests #6246

Add `--minimal-messages-config` option for functional tests #6246

coveralls commented Apr 10, 2022 •

edited

Loading