@thiemowmde added these in c7256b0c68708fbd2c950c4e1ce172fe27d87e15

• assertNotEquals() as well as assertNotSame() with a boolean value is not wrong, but highly confusing. Replace with assertTrue/False().

I agree that the replacement assertion is not exactly the same as the original one, but it normally is what you want (I think) and humans can fix the few wrong fixes

I can see how this might feel wrong in some situations. But I would like to argue that such situations are rare, and the replacement with a positive assertTrue/assertFalse is the more helpful one in the majority of cases.

The point of this sniff is not (only) to replace one syntax with another one that looks different, but is otherwise semantically identical. That wouldn't have much of a benefit. The idea is that assertions that read like "I don't really care about this value, as long as it's not something that would act as if it's false" are vague and nor really helpful at best, or misleading or even wrong in the worst case. In almost all cases you know what to expect. An object? Use assertIsObject. A non-empty array? Use assertIsArray and maybe assertNotEmpty, or assertSame( [], $value ). Do you really don't care about the value's type? Then go ahead and change it to e.g. assertTrue( (bool)$value ), assertNotTrue or whatever fits best.

This sniff's auto-fixes don't force us to accept what they suggest. Such a patch will be found by our CI and not be merged. The only thing the sniff asks us to do is to avoid particularly confusing assertions. What this means is constantly discussed, e.g in T246674, T295644, or right now in https://gerrit.wikimedia.org/r/770066.

I'd think it would be unlikely for someone to use assertNotEquals or assertNotSame with a boolean and not intend for the behavior of assertNotTrue/assertNotFalse, otherwise they'd most likely have written it with assertEquals or assertSame and the opposite boolean in the first place.

Making an incorrect fix with the excuse "oh, it'll make the test fail so people can then manually fix it" seems just that, an excuse.

Change 785377 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Use more specific assertions in HTMLTitleTextField test

https://gerrit.wikimedia.org/r/785377

Maybe it helps to look at a real-world example? At the moment I can find only one, and that looks like it could benefit from being more specific.

Change 785377 merged by jenkins-bot:

[mediawiki/core@master] Use more specific assertions in HTMLTitleTextField test

https://gerrit.wikimedia.org/r/785377

Perhaps you should search for instances of assertNotTrue or assertNotFalse as well, since your position is that those should never be used.

[…] your position is that those should never be used.

Uh, I'm sorry? That's not what the sniff does. I believe we have some kind of misunderstanding here, but I can't figure it out.

Correct, that's not what the sniff does. But this is what you said.

In T306568#7873416, @thiemowmde wrote:

The point of this sniff is not (only) to replace one syntax with another one that looks different, but is otherwise semantically identical. That wouldn't have much of a benefit. The idea is that assertions that read like "I don't really care about this value, as long as it's not something that would act as if it's false" are vague and nor really helpful at best, or misleading or even wrong in the worst case. In almost all cases you know what to expect. An object? Use assertIsObject. A non-empty array? Use assertIsArray and maybe assertNotEmpty, or assertSame( [], $value ). Do you really don't care about the value's type? Then go ahead and change it to e.g. assertTrue( (bool)$value ), assertNotTrue or whatever fits best.

This sniff's auto-fixes don't force us to accept what they suggest. Such a patch will be found by our CI and not be merged. The only thing the sniff asks us to do is to avoid particularly confusing assertions.

I paraphrase that as you believing that assertNotTrue and assertNotFalse should not be used, and you want the sniff to push people in that direction rather than have the sniff produce a more accurate replacement that would be less likely to need manual attention.

In T306568#7885858, @Anomie wrote:

I paraphrase that as you believing that assertNotTrue and assertNotFalse should not be used, and you want the sniff to push people in that direction rather than have the sniff produce a more accurate replacement that would be less likely to need manual attention.

That feels like a slightly unfair characterization, since @thiemowmde wrote (and you even quoted):

In T306568#7873416, @thiemowmde wrote:

Do you really don't care about the value's type? Then go ahead and change it to e.g. assertTrue( (bool)$value ), assertNotTrue or whatever fits best.

If one of the suggestions is to use assertNotTrue, then clearly the position being taken is not that assertNotTrue() should not be used.

The position seems to be that it makes sense, is helpful, or is semantically equivalent, to suggest rewriting assertEquals( true, $x ), assertNotEquals( false, $x), and assertNotSame( false, $x ), as assertTrue( $x ), and vice versa for the inverse forms, as those represent improvements over the code being rewritten.

Considering that, in the phpunit example test class/code for Email.php (at https://phpunit.de/getting-started/phpunit-9.html), all of these assertions pass:

$this->assertNotEquals(
    false,
    Email::fromString('user@example.com')
);
$this->assertNotSame(
    false,
    Email::fromString('user@example.com')
);
$this->assertNotFalse(
   Email::fromString('user@example.com')
);

But this assertion fails:

$this->assertTrue(
    Email::fromString('user@example.com')
);

I find myself questioning that argument as well.

It's true, though, that this assertion also passes:

$this->assertTrue(
    (bool)Email::fromString('user@example.com')
);

Perhaps that's what should be suggested instead, when rewriting assertions that compare unknown types to boolean values? That would still meet the stated goal (from the code comments), which is that the suggested rewrite:

Discourages the use of PHPUnit's relaxed, not type-safe assertEquals() in favor of strict alternatives like assertSame(), assertNull(), and such.

I get that assertNotTrue() and assertNotFalse() aren't really any more type-strict than assertEquals(), since you can call them with almost any value type as an argument. And that could be an argument against suggesting them as replacements, since it wouldn't actually improve type-safety. I am not blind to the intention or the goal here.

But suggesting a rewrite from e.g. assertNotSame( false, $x ) to assertTrue( $x ) is an assumption that $x is a boolean value, when we all know that, most of the time, it won't have been. (And in fact, the test was probably written that way specifically because it's not a boolean value, some or all of the time.) The fix is a clear non-equivalence. And while that may be the intent, it violates the model for auto-corrections. Auto-fixes should not break previously-working code; if they do, they're not "fixes"! (See Notes.)

So, it seems like if the sniff requires this disclaimer:

* Please note: The auto-fixes done by this
* sniff can make PHPUnit test cases fail. These should be fixed not by reverting the fix, but by
* finding better expected values or better assertions.

...then it's broken. And I would suggest that rather than asserting that a test-breaking autofix "should be fixed not by reverting the fix", the sniff should either make better suggestions, or better assertions.

(The user can still manually change an assertTrue( (bool)... ) to assertTrue() without the cast, assertNotFalse(), assertIsObject(), or whatever else makes (more) sense.)

Notes

Rubocop

Rubocop calls semantics-altering autocorrections "unsafe", and any cop that performs such corrections is marked SafeAutoCorrect false and will be disabled by default when running rubocop --auto-correct; it requires rubocop --auto-correct-all to activate those fixes. (https://docs.rubocop.org/rubocop/1.29/usage/auto_correct.html)

ESLint

ESLint's list of best practices for fixes (https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes) opens with:

1. Avoid any fixes that could change the runtime behavior of code and cause it to stop working.

It has a "suggest" feature for adjustments that can be suggested by runtimes (like code editors) for the user to activate on demand, but those are never auto-applied. (https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions)

PHP_CodeSniffer

PHP_CodeSniffer does not appear to have any standards or guidance related to auto-corrections, to its detriment IMHO. However, sniffs can be marked as phpcs-only="true" or phpcbf-only="true" in the ruleset. IMHO, any sniff that alters code semantics should be strictly phpcs-only="true". (https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset#selectively-applying-rules)