pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.33k stars 1.14k forks source link

Allow CheckerTestCase to assertDoesNotAddMessages() to check that a specific message has not been added #9598

Open StephenYu2018 opened 6 months ago

StephenYu2018 commented 6 months ago

Current problem

I use CheckerTestCase in tests for linting scripts in a project. There are some tests where I want to assert that a specific message has not been added given a specific code snippet. While assertNoMessages() works for now, it is possible in the future that I want to assert that a different message occurs for the same code snippet. assertNoMessages() wouldn't work because we do want to check that the different message has been added, but replacing assertNoMessages() with assertAddsMessages(different_message, ...) would mean that we can no longer check that the original message is not being added.

Desired solution

If I could directly assert that a specific message has not been added, then I could have more control over which specific messages I'd want or would not want under various scenarios. For my given example, something like this:

assertDoesNotAddMessages(original_message, ...)
assertAddsMessages(different_message, ...)

Better represents which messages I want to occur and which messages I want to not occur.

Additional context

No response

Pierre-Sassoulas commented 6 months ago

What about adding one function like assertMessages(emitted:iterable[str] | None, not_emitted: iterable[str] | None) ? Emitted = None would means no message were emitted, not_emitted = None would means we don't care if unexpected messages are emitted.

jacobtylerwalls commented 6 months ago

The original proposal is more like this from the stdlib:

self.assertLogs(...
self.assertNoLogs(...

If we add a new assertMessages, we have "two ways to do it" unless we deprecate the old way (no reason to).

badsketch commented 5 months ago

I'd be interested in looking into this. If I understand correctly, we shouldn't be overloading existing methods, nor creating one that might overlap with assertAddsMessages(), so we'll have a new method assertDoesNotAddMessages() to support this new functionality?

Pierre-Sassoulas commented 5 months ago

Thank you @badsketch I assigned you to the issue. Yes I agree with Jacob.

badsketch commented 5 months ago

Any advice on good testing criteria for this 🤔? Correct me if I'm wrong, but I don't see unit tests for checker_test_case.py where the implementation will live. For the PR, I can show how it works with a sample checker in my local machine, but as for "concrete" tests that'll live in the repo, I'm not sure.

Only thing I can think of is choose some existing unit tests that use self.assertAddsMessages() (for ex. unittest_format.py) and addingself.assertDoesNotAddMessages() for messages that clearly won't be present. Basically sanity testing the inverse.

Pierre-Sassoulas commented 5 months ago

Yeah, sadly the testutils are really poorly tested at the moment (tested through being used in the other tests, basically).

I think we should create a test file from scratch in tests/testutil then test something like:

No need to overdo it with multiple raised. But funky meta messages like useless-suppression / suppressed message, might benefit from some special attention if you're feeling heroic.

badsketch commented 5 months ago

Could you tell me more about what you're thinking the tests could look like, or give an example?

expected raised / actual raised

Is this one test that uses assertAddsMessages() to test expected raised ?

expected not raised / actual raised

Would this be the same test as above, but we use assertDoesNotAddMessages() to test expected not raised / actual raised?

expected not raised / actual not raised

Having trouble figuring this out - how might I get the actual not raised? Isn't it everything that's not "actual raised"

Pierre-Sassoulas commented 5 months ago

expected raised / actual raised

Is this one test that uses assertAddsMessages() to test expected raised ?

Yes, testing ``assertAddsMessages(x) and x is raised so the assertion should be successful.

expected not raised / actual raised

Would this be the same test as above, but we use assertDoesNotAddMessages() to test expected not raised / actual raised?

Test using assertDoesNotAddMessages(x), and the assertion should fail because x is raised

expected not raised / actual not raised

Having trouble figuring this out - how might I get the actual not raised? Isn't it everything that's not "actual raised"> Is this one test that uses assertAddsMessages() to test expected raised ?

Yes for example there's a module docstring and we check that missing-module-docstring is not raised with assertDoesNotAddMessages(missing-module-docstring), the assert should be successful.