Skip to content
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

Fix checking of confidence in the unittests #5376

Merged
merged 8 commits into from
Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ Release date: TBA

* Fix ``install graphiz`` message which isn't needed for puml output format.

* ``MessageTest`` of the unittest ``testutil`` now requires the ``confidence`` attribute
to match the expected value. If none is provided it is set to ``UNDEFINED``.

* ``add_message`` of the unittest ``testutil`` now actually handles the ``col_offset`` parameter
and allows it to be checked against actual output in a test.

* Fix ``simplify-boolean-expression`` when condition can be inferred as False.

Closes #5200
Expand Down
6 changes: 6 additions & 0 deletions doc/whatsnew/2.12.rst
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ New checkers

Follow-up in #5259

* ``MessageTest`` of the unittest ``testutil`` now requires the ``confidence`` attribute
to match the expected value. If none is provided it is set to ``UNDEFINED``.

* ``add_message`` of the unittest ``testutil`` now actually handles the ``col_offset`` parameter
and allows it to be checked against actual output in a test.

Removed checkers
================

Expand Down
30 changes: 7 additions & 23 deletions pylint/testutils/output_line.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE

import collections
from typing import Any, NamedTuple, Optional, Sequence, Tuple, Union

from astroid import nodes
Expand All @@ -12,30 +11,15 @@
from pylint.testutils.constants import UPDATE_OPTION


class MessageTest(
collections.namedtuple(
"MessageTest", ["msg_id", "line", "node", "args", "confidence"]
)
):
class MessageTest(NamedTuple):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 My goodness
feelgoodman

But is this already possible in python 3.6 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Or at least it should. The default value only from 3.6.1 but we are already depending on that in other PRs for 2.12 as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, the change was planned for 2.12.1. Would have to look up why though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think because of the default value of NamedTuple. However, the end_line PR also used those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. With the exception in place, someone with 3.6.0 won't even get that far. pip install pylint will fail. It's probably an idea to pick up most of #5068 then too, except for bumping the python_requires version and removing the exception.

https://github.com/PyCQA/pylint/blob/9e32192fe7bf77281d0dfbc107984b751983e113/setup.py#L17-L18

msg_id: str
line: Optional[int] = None
node: Optional[nodes.NodeNG] = None
args: Optional[Any] = None
confidence: Optional[Confidence] = UNDEFINED
col_offset: Optional[int] = None
"""Used to test messages produced by pylint. Class name cannot start with Test as pytest doesn't allow constructors in test classes."""

def __new__(
cls,
msg_id: str,
line: Optional[int] = None,
node: Optional[nodes.NodeNG] = None,
args: Any = None,
confidence: Optional[Confidence] = None,
) -> "MessageTest":
return tuple.__new__(cls, (msg_id, line, node, args, confidence))

def __eq__(self, other: object) -> bool:
if isinstance(other, MessageTest):
if self.confidence and other.confidence:
return super().__eq__(other)
return tuple(self[:-1]) == tuple(other[:-1])
return NotImplemented # pragma: no cover


class MalformedOutputLineException(Exception):
def __init__(
Expand Down
17 changes: 14 additions & 3 deletions pylint/testutils/unittest_linter.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from astroid import nodes

from pylint.interfaces import Confidence
from pylint.interfaces import UNDEFINED, Confidence
from pylint.testutils.global_test_linter import linter
from pylint.testutils.output_line import MessageTest
from pylint.utils import LinterStats
Expand Down Expand Up @@ -37,10 +37,21 @@ def add_message(
end_lineno: Optional[int] = None,
end_col_offset: Optional[int] = None,
) -> None:
# Do not test col_offset for now since changing Message breaks everything
"""Add a MessageTest to the _messages attribute of the linter class."""
# If confidence is None we set it to UNDEFINED as well in PyLinter
if confidence is None:
confidence = UNDEFINED
# pylint: disable=fixme
# TODO: Test col_offset
# pylint: disable=fixme
# TODO: Initialize col_offset on every node (can be None) -> astroid
# if col_offset is None and hasattr(node, "col_offset"):
# col_offset = node.col_offset
# pylint: disable=fixme
# TODO: Test end_lineno and end_col_offset :)
self._messages.append(MessageTest(msg_id, line, node, args, confidence))
self._messages.append(
MessageTest(msg_id, line, node, args, confidence, col_offset)
)

@staticmethod
def is_message_enabled(*unused_args, **unused_kwargs):
Expand Down
51 changes: 40 additions & 11 deletions tests/checkers/unittest_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import astroid

from pylint.checkers import base
from pylint.interfaces import HIGH, INFERENCE
from pylint.testutils import CheckerTestCase, MessageTest, set_config


Expand All @@ -43,7 +44,7 @@ class TestDocstring(CheckerTestCase):

def test_missing_docstring_module(self) -> None:
module = astroid.parse("something")
message = MessageTest("missing-module-docstring", node=module)
message = MessageTest("missing-module-docstring", node=module, confidence=HIGH)
with self.assertAddsMessages(message):
self.checker.visit_module(module)

Expand All @@ -54,7 +55,9 @@ def test_missing_docstring_empty_module(self) -> None:

def test_empty_docstring_module(self) -> None:
module = astroid.parse("''''''")
message = MessageTest("empty-docstring", node=module, args=("module",))
message = MessageTest(
"empty-docstring", node=module, args=("module",), confidence=HIGH
)
with self.assertAddsMessages(message):
self.checker.visit_module(module)

Expand All @@ -69,7 +72,9 @@ def __init__(self, my_param: int) -> None:
'''
"""
)
message = MessageTest("empty-docstring", node=node.body[0], args=("method",))
message = MessageTest(
"empty-docstring", node=node.body[0], args=("method",), confidence=INFERENCE
)
with self.assertAddsMessages(message):
self.checker.visit_functiondef(node.body[0])

Expand All @@ -79,7 +84,7 @@ def test_empty_docstring_function(self) -> None:
def func(tion):
pass"""
)
message = MessageTest("missing-function-docstring", node=func)
message = MessageTest("missing-function-docstring", node=func, confidence=HIGH)
with self.assertAddsMessages(message):
self.checker.visit_functiondef(func)

Expand All @@ -102,7 +107,7 @@ def func(tion):
pass
"""
)
message = MessageTest("missing-function-docstring", node=func)
message = MessageTest("missing-function-docstring", node=func, confidence=HIGH)
with self.assertAddsMessages(message):
self.checker.visit_functiondef(func)

Expand All @@ -117,7 +122,7 @@ def func(tion):
pass
"""
)
message = MessageTest("missing-function-docstring", node=func)
message = MessageTest("missing-function-docstring", node=func, confidence=HIGH)
with self.assertAddsMessages(message):
self.checker.visit_functiondef(func)

Expand All @@ -141,7 +146,9 @@ def __init__(self, my_param: int) -> None:
pass
"""
)
message = MessageTest("missing-function-docstring", node=node.body[0])
message = MessageTest(
"missing-function-docstring", node=node.body[0], confidence=INFERENCE
)
with self.assertAddsMessages(message):
self.checker.visit_functiondef(node.body[0])

Expand All @@ -158,7 +165,11 @@ def __eq__(self, other):
return True
"""
)
message = MessageTest("missing-function-docstring", node=module.body[1].body[0])
message = MessageTest(
"missing-function-docstring",
node=module.body[1].body[0],
confidence=INFERENCE,
)
with self.assertAddsMessages(message):
self.checker.visit_functiondef(module.body[1].body[0])

Expand All @@ -168,7 +179,7 @@ def test_class_no_docstring(self) -> None:
class Klass(object):
pass"""
)
message = MessageTest("missing-class-docstring", node=klass)
message = MessageTest("missing-class-docstring", node=klass, confidence=HIGH)
with self.assertAddsMessages(message):
self.checker.visit_classdef(klass)

Expand Down Expand Up @@ -232,6 +243,7 @@ def QUX(self): #@
"invalid-name",
node=methods[1],
args=("Attribute", "bar", "'[A-Z]+' pattern"),
confidence=INFERENCE,
)
):
self.checker.visit_functiondef(methods[1])
Expand Down Expand Up @@ -310,6 +322,8 @@ class async: #@
msg_id="assign-to-new-keyword",
node=ast[0].targets[0],
args=("async", "3.7"),
confidence=HIGH,
col_offset=None,
)
):
self.checker.visit_assignname(ast[0].targets[0])
Expand All @@ -318,18 +332,28 @@ class async: #@
msg_id="assign-to-new-keyword",
node=ast[1].targets[0],
args=("await", "3.7"),
confidence=HIGH,
col_offset=None,
)
):
self.checker.visit_assignname(ast[1].targets[0])
with self.assertAddsMessages(
MessageTest(
msg_id="assign-to-new-keyword", node=ast[2], args=("async", "3.7")
msg_id="assign-to-new-keyword",
node=ast[2],
args=("async", "3.7"),
confidence=HIGH,
col_offset=None,
)
):
self.checker.visit_functiondef(ast[2])
with self.assertAddsMessages(
MessageTest(
msg_id="assign-to-new-keyword", node=ast[3], args=("async", "3.7")
msg_id="assign-to-new-keyword",
node=ast[3],
args=("async", "3.7"),
confidence=HIGH,
col_offset=None,
)
):
self.checker.visit_classdef(ast[3])
Expand Down Expand Up @@ -360,6 +384,7 @@ class CLASSC(object): #@
"classb",
"the `UP` group in the '(?:(?P<UP>[A-Z]+)|(?P<down>[a-z]+))$' pattern",
),
confidence=HIGH,
)
with self.assertAddsMessages(message):
cls = None
Expand Down Expand Up @@ -389,6 +414,7 @@ class CLASSC(object): #@
"class_a",
"'(?:(?P<UP>[A-Z]+)|(?P<down>[a-z]+))$' pattern",
),
confidence=HIGH,
),
MessageTest(
"invalid-name",
Expand All @@ -398,6 +424,7 @@ class CLASSC(object): #@
"CLASSC",
"the `down` group in the '(?:(?P<UP>[A-Z]+)|(?P<down>[a-z]+))$' pattern",
),
confidence=HIGH,
),
]
with self.assertAddsMessages(*messages):
Expand Down Expand Up @@ -432,6 +459,7 @@ def FUNC(): #@
"FUNC",
"the `down` group in the '(?:(?P<UP>[A-Z]+)|(?P<down>[a-z]+))$' pattern",
),
confidence=HIGH,
)
with self.assertAddsMessages(message):
func = None
Expand Down Expand Up @@ -464,6 +492,7 @@ def UPPER(): #@
"UPPER",
"the `down` group in the '(?:(?P<ignore>FOO)|(?P<UP>[A-Z]+)|(?P<down>[a-z]+))$' pattern",
),
confidence=HIGH,
)
with self.assertAddsMessages(message):
func = None
Expand Down
21 changes: 15 additions & 6 deletions tests/checkers/unittest_misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,26 @@ def test_fixme_with_message(self) -> None:
# FIXME message
"""
with self.assertAddsMessages(
MessageTest(msg_id="fixme", line=2, args="FIXME message")
MessageTest(msg_id="fixme", line=2, args="FIXME message", col_offset=17)
):
self.checker.process_tokens(_tokenize_str(code))

def test_todo_without_message(self) -> None:
code = """a = 1
# TODO
"""
with self.assertAddsMessages(MessageTest(msg_id="fixme", line=2, args="TODO")):
with self.assertAddsMessages(
MessageTest(msg_id="fixme", line=2, args="TODO", col_offset=17)
):
self.checker.process_tokens(_tokenize_str(code))

def test_xxx_without_space(self) -> None:
code = """a = 1
#XXX
"""
with self.assertAddsMessages(MessageTest(msg_id="fixme", line=2, args="XXX")):
with self.assertAddsMessages(
MessageTest(msg_id="fixme", line=2, args="XXX", col_offset=17)
):
self.checker.process_tokens(_tokenize_str(code))

def test_xxx_middle(self) -> None:
Expand All @@ -59,7 +63,9 @@ def test_without_space_fixme(self) -> None:
code = """a = 1
#FIXME
"""
with self.assertAddsMessages(MessageTest(msg_id="fixme", line=2, args="FIXME")):
with self.assertAddsMessages(
MessageTest(msg_id="fixme", line=2, args="FIXME", col_offset=17)
):
self.checker.process_tokens(_tokenize_str(code))

@set_config(notes=[])
Expand All @@ -79,7 +85,7 @@ def test_other_present_codetag(self) -> None:
# FIXME
"""
with self.assertAddsMessages(
MessageTest(msg_id="fixme", line=2, args="CODETAG")
MessageTest(msg_id="fixme", line=2, args="CODETAG", col_offset=17)
):
self.checker.process_tokens(_tokenize_str(code))

Expand All @@ -92,7 +98,10 @@ def test_issue_2321_should_trigger(self) -> None:
code = "# TODO this should not trigger a fixme"
with self.assertAddsMessages(
MessageTest(
msg_id="fixme", line=1, args="TODO this should not trigger a fixme"
msg_id="fixme",
line=1,
args="TODO this should not trigger a fixme",
col_offset=1,
)
):
self.checker.process_tokens(_tokenize_str(code))
Expand Down
Loading