Ensure --fail-on always returns a non-zero exit code when issue is matched

[MarkCBell]

Steps

• 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.

Description

This fixes a bug in which pylint can still return a zero exit code when using --fail-on even if the specified issue occurs when the issues matched are all of info type.

Without this patch, running: pylint --fail-on=useless-suppression a.py when a.py contains:

"""Test file to trigger an useless-suppression error msg"""
# pylint: disable=broad-except
pass

returns exit code 0 despite matching the useless-suppression (info) issue.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

This is related to Issue #4296 and #3363. However although it will provides some other options for these issues, I don't think it will fully resolve them.

[coveralls]

Coverage increased (+0.001%) to 92.056% when pulling b750b8b on MarkCBell:issue-4296 into 6c29598 on PyCQA:main.

[Pierre-Sassoulas]

Thank you for the changes, thjis look reasonable ! Could you add integration tests for this, please ? I'm thinking of something like:

@pytest.mark.parametrize("args,expected", [["fail-under", 0], ...])
def test_exit_code(args, expected):
    with pytest.raises(SystemExit as e:
        with patch("sys.argv", ["pylint", "tests/functional/...",] + args):
             ...

[MarkCBell]

@Pierre-Sassoulas I've added some tests and I think what I've done works (the tests implement the correct behaviour, fail without the functionality patch and pass with the functionality patch) but if you have any suggestions for how to refine the structure of the tests or change where they should be located then please let me know.

[Pierre-Sassoulas]

Nice jobs, the tests are on point, I don't see any forgotten use cases, but if they exists it will be really easy to add them later on. I think this fix the issues you linked, we're not going to add an option for failing on information messages, it makes more sense has the default behavior. Do yo agree ?

[Pierre-Sassoulas]

I'm putting this in 2.10 because it'll feel like a breaking change is a pipeline start to fail because there were informational messages. Could you also add an explaining text in 2.10's what's new, please ?

[MarkCBell]

So I understand there is a need for caution around changes that it may change the behaviour of pipeline. However in order to have a pipeline that is currently succeeding become failing because of this patch, I think the following conditions would need to be met:

• pylint is run without the --exit-zero flag
• pylint generates only informational messages
• at least one of the informational messages that is generated is provided as an argument to the --fail-on flag

So this means that although a developer has explicitly set --fail-on=i0011 (for example) and that message is being generated, their pipeline is still (I would say incorrectly) passing.

I would suggest that this should just be treated as a bug in the implementation of fail-on which despite it's documentation:

    --fail-on=<msg ids>
                        Return non-zero exit code if any of these
                        messages/categories are detected, even if score is
                        above --fail-under value. Syntax same as enable.

can currently return a zero exit code even when a specified (info) message is detected. In which case I don't think that this would need to wait for 2.10. Does that sound reasonable to you or have I missed something?

[Pierre-Sassoulas]

Ho, I did not notice the fail-on requirement. Shouldn't pylint exit with an error code if there is an enabled informational message even without the fail-on option ? If I understand correctly we could merge this as it is, and make the exit code change for 2.10 later ?

[MarkCBell]

I'm sorry but I don't think I understand your last comment. Suppose we were to merge this PR exactly as it is right now into 2.9.4, what exactly is "the exit code change" that you are proposing could be done separately / later for 2.10?

[Pierre-Sassoulas]

Sorry for being unclear. Considering that we now have 3 conditions:

• pylint is run without the --exit-zero flag
• pylint generates only informational messages
• at least one of the informational messages that is generated is provided as an argument to the --fail-on flag

I'd want to remove that last condition regarding the --fail-on flag later on.

[MarkCBell]

Ok, I think I understand now. In which case I suggest that the right thing to do is to merge this PR into 2.9.4 as a bug fix to make the implementation of --fail-on match how it is described in the documentation. As you suggest, breaking changes can be made as part of 2.10 later and hopefully that should be very straightforward based on the structure and tests provided by the PR.