-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
@Pierre-Sassoulas Can I add a skip for the failing test on 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 |
There was a problem hiding this 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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
I did do that: I'm using |
cadcb1e
to
f4ff1ca
Compare
Pull Request Test Coverage Report for Build 1499581579
💛 - Coveralls |
Took me some tries to fix those test errors. Should have installed 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/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* Fix checking of ``confidence`` in the unittests Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
* Fix checking of ``confidence`` in the unittests Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
doc/whatsnew/<current release.rst>
.Type of Changes
Description
Preparation for the PR to update the unittests to use
end_line
andend_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
andcol_offset
is ignored completely. I think it makes more sense to update them all at once withend_line
andend_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).