Skip to content

Allow different test output for different Python versions #10382

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented May 14, 2025

Followup to #10381 (comment)

Turns out the test suite does have an option to specify different output messages for different Python version already. It just wasn't usable as the output message check would subsequently fail.

This could be a good option where ever only the output messages between version changed / a pinning py-version was previously necessary. This does not replace different test cases for added syntax or if the config files are different. E.g. a lot of the typing extension test cases look quite similar but use runtime-typing yes and no.

@cdce8p cdce8p added the Maintenance Discussion or action around maintaining pylint or the dev workflow label May 14, 2025
@cdce8p cdce8p added this to the 4.0.0 milestone May 14, 2025

This comment has been minimized.

This comment has been minimized.

@cdce8p cdce8p added the tests label May 15, 2025

This comment has been minimized.

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.

This looks amazing. Especially the doc ! The code is a little too terse and hard to tests imo, We had a lot of issue in the past with testutils not doing what we expected it to do. Then we trust the CI and bad things happens. I would use the following checker to test the changes:

class TestutilVersionChecker(BaseChecker):
    name = 'version-checker'
    msgs = {
        "E9009": ("Running on Python 3.9",),
        "E9010": ("Running on Python 3.10",),
        "E9011": ("Running on Python 3.11",),
        "E9012": ("Running on Python 3.12",),
        "E9013": ("Running on Python 3.13",),
        "E9014": ("Running on Python 3.14",),
    }

    def visit_module(self, node):
        major, minor = sys.version_info[:2]
        error_code = f"E90{minor:02d}"
        self.add_message(error_code, node=node)

Also I wonder how multiple messages on the same line with some conditional on the interpreter are going to be handled # <3.14:[undefined-variable] # <3.13:[syntax-error] ? (the reason why I put the interpreter info just after the message name in something = 1 # [unused-variable|>=3.13,missing-module-docstring])

Comment on lines 2 to 5
py-version=3.10
load-plugins=pylint.extensions.typing

[testoptions]
min_pyver=3.10
Copy link
Member

Choose a reason for hiding this comment

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

Isn't py-version better than min_pyver ? (i.e. it means we can handle this on all interpreters). Or is there another reason to change this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't look to closely into 3.10 before so just assumed there was some syntax reason when there wasn't any. collections.abc.Generator is subscriptable since 3.9. Remove the testoption here.

@cdce8p
Copy link
Member Author

cdce8p commented May 15, 2025

Also I wonder how multiple messages on the same line with some conditional on the interpreter are going to be handled # <3.14:[undefined-variable] # <3.13:[syntax-error] ? (the reason why I put the interpreter info just after the message name in something = 1 # [unused-variable|>=3.13,missing-module-docstring])

Not sure that's allowed with the current test syntax, though haven't tested it. What does work though is to prefix a message with an offset. E.g. to separate two messages on the same line, we could use something like this:

# +2:<3.13: [syntax-error]
# +1:<3.14: [undefined-variable]
var: x = 2  # some code which raised pylint warnings

In theory, I'd be open to explore other syntax forms. Just used the existing one for the initial implementation as it seems to work good enough and has been (partially) supported for some time now. The pattern is defined here

_EXPECTED_RE = re.compile(
r"\s*#\s*(?:(?P<line>[+-]?[0-9]+):)?" # pylint: disable=consider-using-f-string
r"(?:(?P<op>[><=]+) *(?P<version>[0-9.]+):)?"
r"\s*\[(?P<msgs>{msg}(?:,\s*{msg})*)]".format(**_MESSAGE)
)
_OPERATORS = {">": operator.gt, "<": operator.lt, ">=": operator.ge, "<=": operator.le}

The code is a little too terse and hard to tests imo, We had a lot of issue in the past with testutils not doing what we expected it to do. Then we trust the CI and bad things happens. I would use the following checker to test the changes:

I agree. Thanks for the example. Will add some tests for it.

Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 2fb1531

@Pierre-Sassoulas
Copy link
Member

What does work though is to prefix a message with an offset. E.g. to separate two messages on the same line, we could use something like this:

# +2:<3.13: [syntax-error]
# +1:<3.14: [undefined-variable]
var: x = 2  # some code which raised pylint warnings

Ha, right, not super elegant but it's not like its going to happen all the time anyway. Let's document it, and keep it as it is. Maybe we can use this for the doc:

# +2:>3.13: [something-else]
# +1:>=3.13: [undefined-variable]
var: x = 2  # <3.13: [syntax-error]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants