Better assert message when dealing with functional tests messages

[Pierre-Sassoulas]

Use sets here to show full diffs in functional tests

Co-authored-by: Ethan Leba [email protected]

Description

This change was hidden in #3821 and was not related to the MR that is taking some time because I must handle function and it's hard to do it properly. This was detrimental because tjhe error message was modified again in #3906 although a clearer error message already existed.

Type of Changes

	Type
✓	✨ New feature

[coveralls]

Coverage decreased (-0.09%) to 90.749% when pulling cfee05e on Pierre-Sassoulas:better-assert-message-in-functional-tests into fed4438 on PyCQA:master.

[coveralls]

Coverage decreased (-0.1%) to 90.75% when pulling 923745c on Pierre-Sassoulas:better-assert-message-in-functional-tests into fb6fa45 on PyCQA:master.

[Pierre-Sassoulas]

Changed to work in progress, because I'm going to refactor testutil.py a bit and #3919 need to be merged first.

[hippo91]

@Pierre-Sassoulas let me know when you want me to do the review.

[Pierre-Sassoulas]

Hey @hippo91 , so I worked on this MR as it's the next one for #3821. The idea was to create unittest for testutils but frankly I don't think I'll have the time to do that properly in a reasonable time. But I have some good news : I fixed a bug in the functional tests and the check we were doing on the txt output. Also made the error message clearer when a functional test fail. But the bad news is, ...as you will surely see once the test run in CI... that it revealed a dozen existing errors or checkers that never worked and that we'll have to fix those before being able to merge the fix.

[hippo91]

Hey @hippo91 , so I worked on this MR as it's the next one for #3821. The idea was to create unittest for testutils but frankly I don't think I'll have the time to do that properly in a reasonable time. But I have some good news : I fixed a bug in the functional tests and the check we were doing on the txt output. Also made the error message clearer when a functional test fail. But the bad news is, ...as you will surely see once the test run in CI... that it revealed a dozen existing errors or checkers that never worked and that we'll have to fix those before being able to merge the fix.

Thanks @Pierre-Sassoulas for your investigations! What was exactly the origin of the bug (didn't have time to investigate myself)?
I think we have to correct all the failing tests ASAP and before releasing the future version.
What do you think about it?

[Pierre-Sassoulas]

It was possible to add garbage lines that were not taken into account in the test in the .txt output file, I think it was signaled by the "TODO exhaustivity tests" (?). So for example if the checker wasn't doing anything and the output text had 5 lines of good looking result, the test is green but nothing is done.

For example we have this in the functional test:

use-symbolic-message-instead:2::"Id 'C0111' is used to disable 'missing-docstring' message emission"
use-symbolic-message-instead:3::"Id 'C0102' is used to disable 'blacklisted-name' message emission"
use-symbolic-message-instead:6::"Id 'C0102' is used to disable 'blacklisted-name' message emission"
use-symbolic-message-instead:6::"Id 'R1711' is used to disable 'useless-return' message emission"
use-symbolic-message-instead:10::"Id 'C0111' is used to enable 'missing-docstring' message emission"
missing-function-docstring:10:test_enabled_by_id_msg:"Missing function or method docstring"

But only the missing-function docstring is really reported, the other one are not. So if we want to merge the fix and not regress, we need to retroactively implement all those checkers that we thought were working but are not.

[hippo91]

@Pierre-Sassoulas thanks for your explanation. I think, indeed, that we have to fix all the false negative before merging.
I'll try to look at them ASAP but i'm a but busy right now.

[Pierre-Sassoulas]

Maybe a reasonable thing to do would be to fix functional tests and to remove failing tests on the master branch while creating a feature branch to add them back by reverting the suppression ? That way no new problems get introduced.

[hippo91]

@Pierre-Sassoulas seems to be a good idea. Let's do this!

[Pierre-Sassoulas]

@hippo91 the tests are passing again, I created #3981 in order to follow the reintegration of removed test. Could you check that the revert are really required, maybe some of those are actually not wanted results ? (That would be in commits from "Remove functional test for message manager by id" to the latest)

[hippo91]

@Pierre-Sassoulas sorry i totally forgot to answer your last comment.