pylint icon indicating copy to clipboard operation
pylint copied to clipboard

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

Open StephenYu2018 opened this issue 1 year ago • 8 comments

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

StephenYu2018 avatar May 05 '24 14:05 StephenYu2018

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.

Pierre-Sassoulas avatar May 05 '24 17:05 Pierre-Sassoulas

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

jacobtylerwalls avatar May 05 '24 21:05 jacobtylerwalls

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?

badsketch avatar Jun 12 '24 02:06 badsketch

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

Pierre-Sassoulas avatar Jun 12 '24 05:06 Pierre-Sassoulas

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.

badsketch avatar Jun 16 '24 21:06 badsketch

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:

  • expected raised / actual raised
  • expected raised / actual not raised
  • expected raised / actual not raised but another one raised
  • expected not raised / actual raised
  • expected not raised / actual not raised
  • expected not raised / actual not raised but another one raised

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.

Pierre-Sassoulas avatar Jun 17 '24 08:06 Pierre-Sassoulas

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"

badsketch avatar Jun 23 '24 04:06 badsketch

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.

Pierre-Sassoulas avatar Jun 23 '24 20:06 Pierre-Sassoulas