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

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Nov 22, 2021

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • 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.

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring

Description

Preparation for the PR to update the unittests to use end_line and end_column > the third PR as mentioned in #5343 (comment).

Updating the unittests is going to take some time and perhaps might be best to do in a later release.
Basically the unittests currently don't really check confidence and col_offset is ignored completely. I think it makes more sense to update them all at once with end_line and end_col_offset as we're doing for the functional tests.

However, I think for this it might be better to not do one large PR but do them in steps. There are some difficult changes here which I would like good review on before merging. Having one large PR that updates all four attributes might nog work well.

Another consideration here is whether this is part of the public API. This might be more "breaking" than the functional test as there is no easy way to update the expected output.
If we do consider this to be public I will need to redo the work here. Just let me know what you guys think. (We might consider making some aspects of our testutils private/unstable as to not limit ourselves).

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow labels Nov 22, 2021
@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Nov 22, 2021

@Pierre-Sassoulas Can I add a skip for the failing test on pypy or do we want to investigate?

Never mind, this is an actually failing test that is skipped on Python 3.10. The test is correctly failing now that we are somewhat checking col_offset.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM

might be best to do in a later release.

Strongly agree with that, right now there is nothing but we can make it better progressively.

as there is no easy way to update the expected output.

Could we add a default value to the expected output so the script keeps working until we put the right column later on ? What did I not understand ?

"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

@DanielNoord
Copy link
Collaborator Author

as there is no easy way to update the expected output.

Could we add a default value to the expected output so the script keeps working until we put the right column later on ? What did I not understand ?

I did do that: I'm using UNDEFINED. However, previously if the confidence level didn't match we just compared the tuple with [:-1]. See the removed __eq__ method.
So, if a plugin has a warning with a different level than UNDEFINED but hasn't added that to the MessageTest used in its test, their tests will now fail. I think this is a correct fail: the test is not specific enough and adding UNDEFINED as a confidence when it is not provided is standard behaviour in Pylinter.add_message(). We're just (finally) mimicking that in the tests.
This is technically breaking though.

@coveralls
Copy link

coveralls commented Nov 22, 2021

Pull Request Test Coverage Report for Build 1499581579

  • 11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 93.49%

Totals Coverage Status
Change from base Build 1499577185: 0.007%
Covered Lines: 13974
Relevant Lines: 14947

💛 - Coveralls

@DanielNoord
Copy link
Collaborator Author

Took me some tries to fix those test errors. Should have installed 3.6 from the start 😄

I also added a changelog entry.

If we agree that we find this to be just under the breaking-limit this is good to go for review/2.12.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

👍

@Pierre-Sassoulas Pierre-Sassoulas merged commit c056248 into pylint-dev:main Nov 24, 2021
@DanielNoord DanielNoord deleted the unittests branch November 24, 2021 14:17
Pierre-Sassoulas added a commit to DanielNoord/pylint that referenced this pull request Nov 24, 2021
* Fix checking of ``confidence`` in the unittests

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Pierre-Sassoulas added a commit to DanielNoord/pylint that referenced this pull request Nov 24, 2021
* Fix checking of ``confidence`` in the unittests

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants