Add a rule to catch some incorrect uses of assertTrue()

[kappa]

assertTrue() calls with 2 arguments when the intention is clearly to
compare the values are surprisingly common. This is valid syntax, but
completely incorrect semantics as no comparison is done and the second
argument is used as the assertion failure message.

See some examples:
https://grep.app/search?q=self%5C.assertTrue%5C%28.%2B%2C%5Cs%2A%28%5B%2B-%5D%3F%280%5Bxb%5D%29%3F%5Cd%2B%28%5C.%5Cd%2B%29%3F%7CTrue%7C%5C%5B.%2A%5C%5D%29%5C%29%24&regexp=true&filter[lang][0]=Python

Some incorrect cases are not caught by this rule because there's no way to always reliably determine the role of the second argument. One example is:
https://github.com/vibora-io/vibora/blob/master/tests/schemas/schemas.py#L18

Summary

Test Plan

[isidentical]

If you'd like to implement a couple of more patterns, you can check out teyit. It is a tool where the only aim is formatting assertion calls like this, and I think it would be a great idea to slowly extract refactorings into fixit itself since it is a common place for all kinds of refactorings.

[kappa]

docs check failure should be fixed after #133.

[kappa]

@isidentical

If you'd like to implement a couple of more patterns, you can check out teyit. It is a tool where the only aim is formatting assertion calls like this, and I think it would be a great idea to slowly extract refactorings into fixit itself since it is a common place for all kinds of refactorings.

Thanks for the information on teyit! I really like the idea of making assertions use more specific methods. Correct me if I am wrong, but teyit tries to always keep the semantics of the tests unchanged. My pull request actually fixes incorrect tests. Some tests may start to fail after applying the fixes.

[jimmylai]

Thanks for contributing. The rule looks good to me and I was able to find two violations in our codebase (not a big number).

To make this rule more useful, would you like to add autofix to convert
self.assertTrue(a == b) as self.assertEqual(a, b)?
That falls into the scope of this rule and can make it a more frequently used autofix rule.

[kappa]

@jimmylai, I believe the addition you suggest is out of scope for this rule.

The reasons:

1. it looks for 1 arg calls to assertTrue() whereas all currently searched cases are 2 arg;
2. it is more of a quality improvement than correctness fix -- assertTrue(a == b) is still correct although the error message for assertEqual(a, b) is certainly better;
3. there are more similar improvements possible that could be implemented in a future rule and @isidentical's teyit project is an awesome collection of those: assertTrue(a > b) --> assertGreater(), assertTrue(a in list) --> assertIn() etc.

[jimmylai]

@jimmylai, I believe the addition you suggest is out of scope for this rule.

The reasons:

1. it looks for 1 arg calls to assertTrue() whereas all currently searched cases are 2 arg;
2. it is more of a quality improvement than correctness fix -- assertTrue(a == b) is still correct although the error message for assertEqual(a, b) is certainly better;
3. there are more similar improvements possible that could be implemented in a future rule and @isidentical's teyit project is an awesome collection of those: assertTrue(a > b) --> assertGreater(), assertTrue(a in list) --> assertIn() etc.

Considered the naming of NoAssertTrueForComparisonsRule, I found converting self.assertTrue(a == b) as self.assertEqual(a, b) falls to its scope.
Implementing it requires add another matches() for the single == comparison and build autofix for it.
The assertTrue to assertEqual conversion will be a more impactful autofixer since it's more widely used.
It's up to you to include it in this rule or leave for future. Just let me know.

[kappa]

@jimmylai, I believe the addition you suggest is out of scope for this rule.
The reasons:

1. it looks for 1 arg calls to assertTrue() whereas all currently searched cases are 2 arg;
2. it is more of a quality improvement than correctness fix -- assertTrue(a == b) is still correct although the error message for assertEqual(a, b) is certainly better;
3. there are more similar improvements possible that could be implemented in a future rule and @isidentical's teyit project is an awesome collection of those: assertTrue(a > b) --> assertGreater(), assertTrue(a in list) --> assertIn() etc.

Considered the naming of NoAssertTrueForComparisonsRule, I found converting self.assertTrue(a == b) as self.assertEqual(a, b) falls to its scope.
Implementing it requires add another matches() for the single == comparison and build autofix for it.
The assertTrue to assertEqual conversion will be a more impactful autofixer since it's more widely used.
It's up to you to include it in this rule or leave for future. Just let me know.

I agree that it will be impactful in terms of sheer number of triggers, thank you for pointing that out. I still believe that it should be implemented as a separate rule with a wider scope that will also include other conditions.

[isidentical]

I dont know whether this affects or not, but after the explanation, I agree with that this might be ensured by a different rule (and possibly a more broad one). I can take a look at this weekend, if you are OK with that idea?

[jimmylai]

I dont know whether this affects or not, but after the explanation, I agree with that this might be ensured by a different rule (and possibly a more broad one). I can take a look at this weekend, if you are OK with that idea?

Make the use cases covered by Teyit as another broader rule looks good to me.