Update functional test expected output #5349

DanielNoord · 2021-11-21T12:05:29Z

Add yourself to CONTRIBUTORS if you are a new contributor.
Write a good description on what the PR does.

Type of Changes

	Type
✓	🔨 Refactoring

Description

This adds the confidence to all expected outputs of functional tests.

coveralls · 2021-11-21T12:15:59Z

Pull Request Test Coverage Report for Build 1499777334

27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.009%) to 93.499%

Totals
Change from base Build 1499735092:	0.009%
Covered Lines:	13995
Relevant Lines:	14968

💛 - Coveralls

Pierre-Sassoulas

Approving the first part, thank you ! :) Let's add the end line / end column change on top of it.

Pierre-Sassoulas · 2021-11-21T12:15:13Z

pylint/testutils/output_line.py

@@ -106,6 +107,11 @@ def from_csv(cls, row: Union[Sequence[str], str]) -> "OutputLine":
            if isinstance(row, Sequence):
                column = cls._get_column(row[2])
                if len(row) == 5:
+                    warnings.warn(
+                        "In pylint 3.0 tests output should always include the expected confidence"


tests/functional/a/async_functions.txt

tests/functional/b/bad_open_mode.txt

DanielNoord · 2021-11-21T14:08:50Z

Used the new updater! Should be better now!

DanielNoord · 2021-11-21T14:47:50Z

Used the old updater! Shouldn't fail now 😄

DanielNoord · 2021-11-22T17:02:18Z

@Pierre-Sassoulas @cdce8p I'm working on updating OutputLine
https://github.com/PyCQA/pylint/blob/16a4e30da9ed738161372c5d12b5fd8f100d0f78/pylint/testutils/output_line.py#L76-L82

It makes sense to have end_line and end_column go in between column and object` in the attributes list. Not only does it makes more sense consistency-wise, but it would also allow the following code to be kept relatively the same:
https://github.com/PyCQA/pylint/blob/16a4e30da9ed738161372c5d12b5fd8f100d0f78/pylint/testutils/output_line.py#L116-L117

Adding end_line and end_column to the end of OutputLine's attributes would allow using a default value and keeps compatibility with previous versions. However, I wonder if direct access to OutputLine is considered part of the public API. I don't think anybody would normally need access to it directly: it should be called either from from_csv() or from_msg(). We can easily keep compatibility through those methods, even when inserting the attributes in between the others.

TLDR: Is OutputLine public or private? And can we insert end_line and end_column in between the other attributes?

Pierre-Sassoulas · 2021-11-22T17:38:10Z

Is OutputLine public or private? And can we insert end_line and end_column in between the other attributes?

It's semi public (as maybe used by plugin developer for their tests ?), but as we provide a quick way to upgrade the expected output let's consider that this is a "breaking change" we can do in a minor.

DanielNoord · 2021-11-22T18:44:52Z

It's semi public (as maybe used by plugin developer for their tests ?),

Yeah that was what I was wondering. However, looking at the code I don't think they should every instantiate an OutputClass directly and only do so via from_csv() or from_msg(), which makes it less risky to change this!

but as we provide a quick way to upgrade the expected output let's consider that this is a "breaking change" we can do in a minor.

Fair enough!

DanielNoord · 2021-11-22T19:50:03Z

Latest commit is WIP and blocked by #5343.

pylint/testutils/output_line.py

DanielNoord · 2021-11-23T00:20:16Z

I have added additional fixes to the functional test testutils as this seemed the right time to update them all at once.

Besides the updating of our own expected output files this PR now includes:

Allow and add end_lineno and end_column to the expected output of functional tests
DeprecationWarning for OutputLine without confidence, end_lineno and end_column
Standard confidence is now UNDEFINED mimicking PyLinter.add_message().
https://github.com/PyCQA/pylint/blob/9e32192fe7bf77281d0dfbc107984b751983e113/pylint/lint/pylinter.py#L1515-L1516
Removed the note saying that functional test consider UNDEFINED and HIGH to be the same. See tests/test_functional.py
Made the functional test update run even when the expected output equals the actual output. This makes it easier to update half-correct files and when you're supplying the flag you probably don't care about pytest taking 10 secs extra.

Pierre-Sassoulas

This look amazing. Checking that the column match the expected would take month. I don't think we have a choice but to release as is and wait for issues to get opened.

cdce8p

A few notes:

What is the value in adding UNDEFINED basically everywhere? Do we even use the confidence level somewhere?
Checking that the column match the expected would take month. I don't think we have a choice but to release as is and wait for issues to get opened.

It should be much easier once vscode-python supports error ranges. Then it's basically just opening the test file and checking that the ranges make sense.

cdce8p · 2021-11-23T16:36:18Z

pylint/testutils/output_line.py

+    @staticmethod
+    def _get_py38_none_value(value: T) -> Optional[T]:
+        """Handle attributes that are always None on pylint < 3.8 similar to _get_column."""
+        if not PY38_PLUS:
+            # We check the value only for the new better ast parser introduced in python 3.8
+            return None  # pragma: no cover
+        return value


That will likely not work for nodes that added end_lineno and end_col_offset later. E.g. keyword, Slice (both only in 3.9). Maybe only compare the values if end_lineno is not None or better a new field in [testoptions] to overwrite PY38_PLUS. I.e. if field is specified, ignore end_lineno check for all versions prior.

Do we ever create messages on those nodes though? Even invalid-slice-index does not pass an nodes.Slice as its node parameter. Could it be that we're fine if we ignore these?

I'm not sure how the testoptions stuff work so I'll need some guidance if want to add a new one.

I think only compare the values if end_lineno is not None is the simpler because I don't know what we could put in the test_option. A condition that needs to succeed or we return None ? Seems like we'll have to launch an eval for that (with all the implication for job launched from fork via github actions) ?

I looked into it a bit more and I think min_pyver_end_line could work? If we convert it to a boolean and pass it to _get_py38_none_value that should make it future proof while testing on versions that it should.

only compare the values if end_lineno is not None is what was previously done for confidence with the __eq__, I don't think that is the way to go as it allows to just never check it.

min_pyver_end_line

Ho, of course. I did not think of that. Very nice solution ! Should we add one option removing the check for line/end_line/column/end_column (min_pyver_position to check all 4 values properly only in the version that is the most accurate) or should we add 4 options (min_pyver_end_line, min_pyver_end_column ... in order to check line/column if line_end or column_end do not exists in other version) ?

@cdce8p Knows more about this than I do so I'll let him respond to this but I believe 3.6 has line and col_offset for all nodes that should have it. 3.8 adds end_line and end_col_offset for most and 3.9 adds it for some that were missing.

Thus I think we can create one min_pyver_end_position (?) which defaults to 3.8.0 and can be increased if needed.

Btw, I'm going to be quite busy tonight and tomorrow probably so if we want to release 2.12 today/tomorrow somebody else would likely need to start working on this. Sorry about that!

Don't worry 😄 min_pyver_end_position convinced me, do you agree @cdce8p ? By the way it's probably not strictly necessary to release 2.12.0 as we don't have trouble with it right now. We're probably never emitting message for Slice/Keyword nodes ?

Found some time, see #5386

DanielNoord · 2021-11-23T17:00:47Z

What is the value in adding UNDEFINED basically everywhere? Do we even use the confidence level somewhere?

It allows us to remove the weird __eq__ method. confidence is used by the confidence= setting so it makes sense to actually include it in our tests. It is a bit dumb since if its None we add UNDEFINED in PyLinter.add_message but I think it makes sense to update this while we're at it.
The fact that we handled HIGH as if it is similar to UNDEFINED is weird anyway. That is also fixed by this.

Pierre-Sassoulas · 2021-11-23T18:40:29Z

It should be much easier once vscode-python supports error ranges.

👍 although checking the whole functional test directory might still take a considerable amount of time. The visual result in vscode is motivating though.

* Fix checking of ``confidence`` in the unittests Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

Pierre-Sassoulas · 2021-11-24T14:43:15Z

@DanielNoord should we close #5336 when merging this or should we move #5336 to 2.13 ? I'm planning on finally releasing today :)

DanielNoord · 2021-11-24T15:38:29Z

We shouldn't close. end_column can't be tested in the unittests right now. That will be the next PR I'm working on.

DanielNoord · 2021-11-24T15:54:13Z

@Pierre-Sassoulas #5349 (comment) This was not resolved and might become problematic in the future.

I think we shouldn't release this before fixing this!

Pierre-Sassoulas · 2021-11-24T16:22:19Z

Ha, right. I was just about to release, thank you for bringing this up 😄

DanielNoord added the task label Nov 21, 2021

DanielNoord requested a review from Pierre-Sassoulas November 21, 2021 12:05

DanielNoord changed the title ~~Tests~~ Update functional test expected output Nov 21, 2021

Pierre-Sassoulas requested changes Nov 21, 2021

View reviewed changes

Pierre-Sassoulas added this to the 2.12.0 milestone Nov 21, 2021

DanielNoord force-pushed the tests branch from 053eb6c to 4ff6e60 Compare November 21, 2021 14:08

DanielNoord force-pushed the tests branch from 4ff6e60 to 4bc56a6 Compare November 21, 2021 14:46

DanielNoord force-pushed the tests branch from 1e2d1cd to 45223e0 Compare November 21, 2021 21:41

This was referenced Nov 22, 2021

Add end_lineno and end_col_offset to MessageLocationTuple #5343

Merged

Migrate test for extension to functional tests #5374

Merged

Include end column / range in message output #5336

Closed

DanielNoord commented Nov 23, 2021

View reviewed changes

pylint/testutils/output_line.py Outdated Show resolved Hide resolved

DanielNoord added Maintenance Discussion or action around maintaining pylint or the dev workflow Enhancement ✨ Improvement to a component labels Nov 23, 2021

Pierre-Sassoulas approved these changes Nov 23, 2021

View reviewed changes

cdce8p reviewed Nov 23, 2021

View reviewed changes

Pierre-Sassoulas mentioned this pull request Nov 24, 2021

Update reporters to (allow) use of end_line and end_column #5372

Merged

4 tasks

Pierre-Sassoulas force-pushed the tests branch from 963b16d to 9ba6921 Compare November 24, 2021 14:24

DanielNoord and others added 2 commits November 24, 2021 15:25

Fix checking of confidence in the unittests (pylint-dev#5376)

7f6229b

* Fix checking of ``confidence`` in the unittests Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

Add confidence to all expected functional test outputs

83c264c

DanielNoord added 10 commits November 24, 2021 15:25

Make OutputLine accept end_line and end_column

b472213

Update tests

f2ba3fb

Update changelog

d7f6353

Update tests

68a2205

Fix tests

35222c8

Updates tests

b430fc6

Fix tests

ebb31e5

Update pylint/testutils/output_line.py

6aaa0f2

It's getting late (fix test)

a6f998b

Update new extensions tests

867ba79

Pierre-Sassoulas force-pushed the tests branch from 9ba6921 to 867ba79 Compare November 24, 2021 14:26

Pierre-Sassoulas merged commit be149db into pylint-dev:main Nov 24, 2021

DanielNoord deleted the tests branch November 24, 2021 15:53

Pierre-Sassoulas mentioned this pull request Nov 24, 2021

To-do list for release 2.12.0 #5250

Closed

9 tasks

DanielNoord mentioned this pull request Nov 24, 2021

Add min_pyver_end_position option #5386

Merged

4 tasks

Pierre-Sassoulas removed the task label Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update functional test expected output #5349

Update functional test expected output #5349

DanielNoord commented Nov 21, 2021

coveralls commented Nov 21, 2021 •

edited

Pierre-Sassoulas left a comment

Pierre-Sassoulas Nov 21, 2021

DanielNoord commented Nov 21, 2021

DanielNoord commented Nov 21, 2021

DanielNoord commented Nov 22, 2021 •

edited

Pierre-Sassoulas commented Nov 22, 2021

DanielNoord commented Nov 22, 2021

DanielNoord commented Nov 22, 2021

DanielNoord commented Nov 23, 2021

Pierre-Sassoulas left a comment

cdce8p left a comment

cdce8p Nov 23, 2021

DanielNoord Nov 23, 2021

Pierre-Sassoulas Nov 24, 2021

DanielNoord Nov 24, 2021

Pierre-Sassoulas Nov 24, 2021

DanielNoord Nov 24, 2021 •

edited

Pierre-Sassoulas Nov 24, 2021

DanielNoord Nov 24, 2021

DanielNoord commented Nov 23, 2021

Pierre-Sassoulas commented Nov 23, 2021

Pierre-Sassoulas commented Nov 24, 2021

DanielNoord commented Nov 24, 2021

DanielNoord commented Nov 24, 2021

Pierre-Sassoulas commented Nov 24, 2021

Update functional test expected output #5349

Update functional test expected output #5349

Conversation

DanielNoord commented Nov 21, 2021

Type of Changes

Description

coveralls commented Nov 21, 2021 • edited

Pull Request Test Coverage Report for Build 1499777334

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Nov 21, 2021

DanielNoord commented Nov 21, 2021

DanielNoord commented Nov 22, 2021 • edited

Pierre-Sassoulas commented Nov 22, 2021

DanielNoord commented Nov 22, 2021

DanielNoord commented Nov 22, 2021

DanielNoord commented Nov 23, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

cdce8p 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

DanielNoord Nov 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Nov 23, 2021

Pierre-Sassoulas commented Nov 23, 2021

Pierre-Sassoulas commented Nov 24, 2021

DanielNoord commented Nov 24, 2021

DanielNoord commented Nov 24, 2021

Pierre-Sassoulas commented Nov 24, 2021

coveralls commented Nov 21, 2021 •

edited

DanielNoord commented Nov 22, 2021 •

edited

DanielNoord Nov 24, 2021 •

edited