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 several tests related to constants #10013

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

temyurchenko
Copy link
Contributor

I've added some tests that test regressions in the tests.

I've also made the message to include the number of times a certain
message is missing or extra. Without it, I had a very confusing
experience, when I seemed to have the message and yet the
test-suite reported that I don't. Turned out, I had to add four
more of the same!

Type of Changes

Type
🐛 Bug fix

Description

I'd be surprised if it passes the tests right now. This will need a new version of astroid to pass, specifically, pylint-dev/astroid#2602

I've added some tests that test regressions in the tests.

I've also made the message to include the number of times a certain
   message is missing or extra. Without it, I had a very confusing
   experience, when I seemed to have the message and yet the
   test-suite reported that I don't. Turned out, I had to add four
   more of the same!
Comment on lines 23 to 28
def test_all_elements_without_parent(self) -> None:
node = astroid.extract_node("__all__ = []")
node.value.elts.append(astroid.Const("test"))
node.value.elts.append(astroid.Const("test", parent=None))
root = node.root()
with self.assertNoMessages():
self.checker.visit_module(root)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's a good test. Why should adding a constant not result in messages? That's just asserting implementation quirks. We should test the opposite, we should test that synthetic constants result in the same messages.
IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants