Deprecate dangerous assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests

Fixing failing tests may be daunting, but for someone with sufficient PHP experience it may be a good task.

Me & @tomasnagy are working on this at DrupalCon Dublin 2016.

Went over all the failing test together with @jeroenbegyn. All tests should validate correctly.

Fixed almost all failed tests introduced with 8.3.x. FrontPageTest keeps failing for now, can't figure out why it doesn't know about assertNotEmpty (PHPUnit specific functions?). Any assistance would be appreciated.

Conceptually I totally support this change. One question though we could ask: How do announce that change, so contrib/custom code can adapt without breaking immediately.

For now, how do I do a hard assertFalse()? Calling parent::assertFalse() seems to put me inside of the trait.

Cool, thanks klausi!

Re-rolled the patch against 8.4.x. Let's see how this goes.

Made the change in a few other places also, and removed the change from core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php to fix the problem with FrontPageTest.

Great progress, thanks ZeiP

Some more tests changed to use more correct assert methods.

This should fix almost all of the remaining problems; only one test I was unable to run locally. Let's see how it goes.

Okay, so apparently we have two tests of which one works at a time: "Drupal\node\Tests\Views\FrontPageTest", which can't find assertNotEmpty(), and "Drupal\Tests\views\Kernel\RenderCacheIntegrationTest", which requires the change. Both of these use core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php.

Back to square one with an updated patch for 8.4.x. I'll try to continue with this in the coming days. Assigning to myself.

The status shouldn't have been changed.

Patch in #30 no longer applies so re-rolled.

I have to say, I'm somewhat surprised. There's way more failures than I'd have thought.

I'll try to fix some of them tonight, let's see where that gets us.

This should be much closer. I'm changing the version back to 8.4.x, because I think that making this kind of test cleanup might be a good fit for the RC stage.

This should be pretty close.

Retry with correct patch.

Perhaps this time it's the actually correct patch. Sorry for the spam.

No worries about the spamming. @ZeiP I'm curious, how do you produce the patch?

I'm making the changes by hand using the testing results, then adding the changed lines with git add -p (trying to catch any possible mistakes I might've made) and then output git diff --cached to the patch file. On the last round I forgot the git add before outputting the ”new” patch... :)

@ZeiP I'm wondering whether you could use a regex with sed or so to produce the same patch. I don't say that's better or worse, I'm just wondering :)

Probably not. I'm only replacing assertFalse/assertTrue calls that fail, since many of them probably are indeed comparing booleans and should be left alone. And the assertFalse calls are replaced with either assertEmpty or assertNull depending on the value etc. Also I think I should have most of them down again now, probably the same problematic ones to be solved after that – and that probably requires something completely else :)

Thanks for the suggestion, though! The previous issue I was meddling with was a more straightforward one and that one I managed to mostly do with just sed and related tools.

Few more. Perhaps now we have all?

Suspected as much. One more to go. I'm a bit surprised that the constant errors we got a few months ago seem to be gone – perhaps this one'll be a green one?

There were a few calls to assertTrue that didn't look quite right. This patch tries to fix those to work correctly.

Another try at fixing the assertTrue in CommentAdminViewUpdateTest.

Ok, this seems good to go. Attached is a interdiff for the last change that was more than just a simple assert function change. Please review.

Awesome work ZeiP!

Not to be a pain, but there are also instances of $this->assertSame(FALSE, ... and $this->assertSame(TRUE, ... that could perhaps be changed while you're at it.

Conceptually I totally support this change. One question though we could ask: How do announce that change, so contrib/custom code can adapt without breaking immediately.

@kristiaanvandeneynde, I could change those, but the patch is quite large as is, so I think we should make that a separate issue. I'm not even sure about the few changes in #56, since even those were perhaps slightly out of scope. Keeping the issue straightforward it's probably easier to get in.

Announcing the change is a good question. This also relates to the version question: Should we only do this in 8.5.x?

8.5.x only with a change record should make sure people have time to update their code? If people were doing a hard check for TRUE or FALSE, they would use assertSame() already. If people were getting (un)lucky because they meant to use assertFalse and an empty value actually passed their test, their tests will now rightfully fail.

Or am I missing something?

Mostly yes. The main point is that apparently assertFalse/assertTrue was in some point actually meant to check for any kind of empty – FALSE, NULL, etc. So I'm not sure if all the tests would then rightfully fail, but they obviously would be now incorrect and therefore should be fixed. Basically IMO announcing this with the brand-new 8.5.x seems fair enough.

Changing the version back to 8.5.x. Attached is a patch for 8.5.x, seems that the previous patch didn't apply cleanly.

Apparently there had been added one instance to 8.5.x :) This should be ready for review.

We certainly need to write a change notice so contrib module authors can fix their tests ... I think its okay to

1. +++ b/core/modules/aggregator/tests/src/Functional/AggregatorTestBase.php
   @@ -210,7 +210,7 @@ public function deleteFeedItems(FeedInterface $feed) {
        $count = db_query('SELECT COUNT(*) FROM {aggregator_item} WHERE fid = :fid', [':fid' => $feed->id()])->fetchField();
   -    $this->assertTrue($count);
   +    $this->assertNotEmpty($count);
   

   I think this should actually be $this->assertSame(0, $count) don't you think so?

2. +++ b/core/modules/aggregator/tests/src/Kernel/Migrate/d7/MigrateAggregatorFeedTest.php
   @@ -47,7 +47,7 @@ public function testAggregatorFeedImport() {
   -    $this->assertTrue(preg_match('/^"[a-z0-9]{32}"$/', $feed->getEtag()));
   +    $this->assertNotEmpty(preg_match('/^"[a-z0-9]{32}"$/', $feed->getEtag()));
   

   Given what preg_match returns I guess assertSame(1) would be better

3. +++ b/core/modules/comment/tests/src/Kernel/Migrate/d6/MigrateCommentVariableInstanceTest.php
   @@ -42,9 +42,9 @@ public function testCommentFieldInstance() {
   -    $this->assertFalse($settings['anonymous']);
   +    $this->assertEmpty($settings['anonymous']);
   

   I'm curious whether we can be more specific?

... note: I've not reviewed the entire patch but it feels like using assertEmpty everywhere doesn't really improve the situation.

I think this should actually be $this->assertSame(0, $count) don't you think so?

I don't think so. The code used to check for a "true-ish" count; i.e.: the query having results.

There are indeed quite a bit of instances that should perhaps actually do something else than use any of the assert{False,True,Empty,NotEmpty,Null}. It would be possible to fix these while we're at it, but I'm wondering if that goes a bit too much outside the issue scope and makes reviewing the patch needlessly difficult?

The current patch tries to stay faithful to the original meaning of the assert call fixing just the True/False-assertion, except for two instances which IMO were clearly buggy. I somewhat disagree with @dawehner in that the patch greatly improves the situation for most of the calls, since now we're actually separating booleans from other values. It's only this limited number of calls that are basically in the same situation as before.

If we want to do all of the fixes in this issue, I'm quite fine with it, but IMO it seems like we should maybe create a follow-up task for that and keep this one as simple as possible (which unfortunately still is far from simple).

I tend to agree with Daniel here. If we are going to fix it, might as well fix it now. Otherwise, how will we find those that need fixing in a follow-up?

Well, you are correct in that it would require going through the same changes again.

Attached is a patch trying to find the correct assert functions for each replacement. Optimally this should come out as green, but there's probably something I misinterpreted, so let's see.

Few typos fixed.

A few missing castings and for some reason some previously unproblematic assertTrue/False calls fixed.

One more error.

Some more fixes for the new changes.

Everything seems to be in order. Attached is an interdiff for the changes, please review.

Damn, named the interdiff incorrectly. Resetting to Needs review.

Interdiff generally looks great!

Just a minor nitpick:

-    $this->assertNotEmpty($page->isDefaultRevision());
+    $this->assertTrue((bool) $page->isDefaultRevision());

Typecasting return values with (bool) and then asserting their boolean value is essentially the same as checking for empty or not empty. Because 'foobar' would still pass the above test. So those typecasts need to go :/

I'm sorry if this discourages you, because I really feel your pain writing long patches like this, but... Needs work :(

Wow this is a tonne of work - thanks @ZeiP. So the good news is we do have a way of progressing here. We can deprecate instead of remove. I've re-rolled the patch and added deprecations instead of removing the old methods. I guess this will fail with new deprecations as we'll have added new usages. Also we need a change record to inform people about the change.

No interdiff because the conflicts on reroll were many!

Updated issue summary. Also this is not just about Kernel tests - it's about Functional and FunctionalJavascript tests too because they use the same trait via \Drupal\FunctionalTests\AssertLegacyTrait

Here's some of the tests fixed and also one bug in \Drupal\user\Plugin\views\access\Role::access() which is quite important because this is an access function which is not adhering to the interface. Should probably split that out into a separate issue but it kinda proves the point about the danger of these overrides.

I think this needs a CR, and maybe bumped to critical since this is a deprceation that will impact contrib a lot?

@mondrake yep,,, thanks for tagging...

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -551,7 +551,7 @@ public static function onDependencyRemoval(FieldDefinitionInterface $field_defin
                  // setting, disable the auto-creation feature completely.
                  $auto_create_bundle = !empty($handler_settings['auto_create_bundle']) ? $handler_settings['auto_create_bundle'] : FALSE;
                  if ($auto_create_bundle && $auto_create_bundle == $bundle->id()) {
   -                $handler_settings['auto_create'] = NULL;
   +                $handler_settings['auto_create'] = FALSE;
                    $handler_settings['auto_create_bundle'] = NULL;
                  }
    
   

   This is bug fix. NULL is not a valid value according to config schema. One interesting thing to work out is how come the schema is not being applied. Needs its own issue I think. This issue should be test changes only.

2. +++ b/core/modules/user/src/Plugin/views/access/Role.php
   @@ -69,7 +69,7 @@ public static function create(ContainerInterface $container, array $configuratio
       * {@inheritdoc}
       */
      public function access(AccountInterface $account) {
   -    return array_intersect(array_filter($this->options['role']), $account->getRoles());
   +    return !empty(array_intersect(array_filter($this->options['role']), $account->getRoles()));
      }
    
      /**
   

   This is bugfix too and needs an issue. This one feels quite serious as it's used in access and it's not returning the correct type. Bring on return typing!

3. +++ b/core/tests/Drupal/KernelTests/AssertLegacyTrait.php
   @@ -35,6 +35,7 @@ public static function assertTrue($actual, $message = '') {
          parent::assertTrue($actual, $message);
        }
        else {
   +      @trigger_error('Support for asserting against non-boolean values in ::assertTrue is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a different assert method, for example, ::assertNotEmpty(). See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
          parent::assertNotEmpty($actual, $message);
        }
      }
   @@ -47,6 +48,7 @@ public static function assertFalse($actual, $message = '') {
   
   @@ -47,6 +48,7 @@ public static function assertFalse($actual, $message = '') {
          parent::assertFalse($actual, $message);
        }
        else {
   +      @trigger_error('Support for asserting against non-boolean values in ::assertFalse is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a different assert method, for example, ::assertEmpty(). See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
          parent::assertEmpty($actual, $message);
        }
      }
   

   Hence the https://www.drupal.org/node/TODO

1. Needs title update. The title applies only to kernel tests.

Also this is not just about Kernel tests - it's about Functional and FunctionalJavascript tests too

2. Should we also improve tests on the simpletest module? The preg_match() function returns 1, 0 or FALSE.

--- a/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
+++ b/core/modules/simpletest/src/Tests/SimpleTestBrowserTest.php
@@ -90,7 +90,7 @@ public function testUserAgentValidation() {
     $http_path = $system_path . '/tests/http.php/user/login';
     $https_path = $system_path . '/tests/https.php/user/login';
     // Generate a valid simpletest User-Agent to pass validation.
-    $this->assertTrue(preg_match('/test\d+/', $this->databasePrefix, $matches), 'Database prefix contains test prefix.');
+    $this->assertEquals(1, preg_match('/test\d+/', $this->databasePrefix, $matches) === 1, 'Database prefix contains test prefix.');
     $test_ua = drupal_generate_test_ua($matches[0]);
     $this->additionalCurlOptions = [CURLOPT_USERAGENT => $test_ua];

per #91 and #92

I added change record: Removed assertTrue/False() compatibility overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests.

AFAICT it's not 'Remove' but 'Deprecate' - valid for both the issue title and the CR

Updated the issue title and will rework the change record.

I've fleshed out the change record and improved a couple of the new assertions - this task is pretty much unending because there many many places where we could use better assertions so I think we should just get the patch to pass and do our best to use the best semantic assertion in place but not quibble if the new assertion works. Whatever we replace it with it is likely to be more specific that the current duck-typing of HEAD.

@Krzysztof Domański what do you mean by #93.2 - the reason that can't use assertRegExp() is that we need $matches.

Opened #3082287: \Drupal\user\Plugin\views\access\Role::access() does not conform to the base class documentation and #3082289: \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::onDependencyRemoval() sets auto_create to an invalid data type to fix the bugs so this issue is only scoped to test changes.

How about deprecating also the assertEquals override in KernelTestBase and BrowserTestBase, so forcing tests to call explicitly castSafeStrings when needed? (I know, not here)

re #102 - sure but yeah not here and the gain with it is less obvious.One thing that is interesting is instead of the assertEquals hack we could register a Comparator to compare MarkupInterface objects... but as before definitely not as part of this issue :)

Filed #3082340: Replace assertEquals() overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests with a MarkupInterfaceComparator implementation

I created follow-up: #3082415: Replace assert(Not)Same/Identical() on booleans with assert(Not)True/False() in PHPUnit tests.

this task is pretty much unending because there many many places where we could use better assertions

I've created a issue to track this and similar problems #3082735: [META] Identify dangerous and problematic cases of casting data types.

Rerolled now the two bug fixes are in. So we should be good to go here unless some new assertTrue/False have been added.

Doing

$ git diff 8.8.x HEAD --name-only | grep -v "/tests/"
core/modules/migrate_drupal/src/Tests/StubTestTrait.php
core/modules/views/src/Tests/AssertViewsCacheTagsTrait.php

on a branch with this patch committed shows that all the changes in the this patch are to code contain in a /tests/ directory apart from the two test traits above. Those traits are test code. They probably should be moved to a better place but that is out-of-scope for this issue.

Nits:

1. +++ b/core/modules/config/tests/src/Functional/ConfigEntityTest.php
   @@ -37,7 +38,6 @@ public function testCRUD() {
   -    $this->assertTrue($empty->uuid());
   

   This is removed and not replaced

2. +++ b/core/modules/config/tests/src/Functional/ConfigEntityTest.php
   @@ -95,7 +95,6 @@ public function testCRUD() {
   -    $this->assertTrue($config_test->uuid());
   

   Same

3. +++ b/core/modules/content_moderation/tests/src/Kernel/EntityOperationsTest.php
   @@ -74,7 +74,7 @@ public function testPendingRevisions() {
   +    $this->assertTrue((bool) $page->isDefaultRevision());
   

   Looks like the return value of isDefaultRevision is mixed and not bool as documented. Follow up to correct the docs? Then assertNotFalse instead?

4. +++ b/core/modules/field/tests/src/Kernel/FieldCrudTest.php
   @@ -69,7 +69,7 @@ public function testCreateField() {
   +    $this->assertEquals('TRUE', $field->getSetting('field_setting_from_config_data'));
   

   assertSame?

5. +++ b/core/modules/field/tests/src/Kernel/FieldImportCreateTest.php
   @@ -41,15 +41,17 @@ public function testImportCreateDefault() {
   +    $this->assertEquals('entity_test', $field2a->getTargetBundle(), 'The second field was created on bundle entity_test.');
   ...
   +    $this->assertEquals('test_bundle', $field2b->getTargetBundle(), 'The second field was created on bundle test_bundle.');
   

   assertSame?

6. +++ b/core/modules/media/tests/src/FunctionalJavascript/MediaViewsWizardTest.php
   @@ -43,7 +43,7 @@ public function testMediaWizard() {
   +    $this->assertSame($view->filter['status']->value, '1');
   
   @@ -77,7 +77,7 @@ public function testMediaRevisionWizard() {
   +    $this->assertSame($view->filter['status']->value, '1');
   

   Swap the arguments

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/FormGroupingElementsTest.php
   @@ -126,8 +126,8 @@ public function testDetailsChildVisibility() {
   +    $this->assertEquals('true', $summary->getAttribute('aria-expanded'));
   +    $this->assertEquals('true', $summary->getAttribute('aria-pressed'));
   

   assertSame?

8. +++ b/core/tests/Drupal/KernelTests/Core/File/StreamWrapperTest.php
   @@ -85,8 +85,8 @@ public function testUriFunctions() {
   +    $this->assertSame($stream_wrapper_manager::getTarget('public://'), '');
   +    $this->assertSame($stream_wrapper_manager::getTarget('data:'), '');
   

   Swap arguments

@mondrake re #109
1. This is a duplicate assertion. This assertion is made twice in the test. We just remove one.
2. This is a duplicate assertion. This assertion is made twice in the test. We just remove one.
3. Nah - the cast is unnecessary here. Nice catch. ::isDefaultRevision() can only ever return a Boolean.
4, 5, 6, 7, 8: I don't think matter at all. The assertEquals on a text string is fine. It's not as if ->assertEquals('test', TRUE) results in a pass. And the argument order is wrong all over the place - we shouldn't fix that here we'd be here forever and not get the main win of this issue which is consistency with PHPUnit and catching the occasional bug when we've made assumptions about return types.

+++ b/core/modules/content_moderation/tests/src/Kernel/EntityOperationsTest.php
@@ -178,7 +178,7 @@ public function testArchive() {
     $storage = \Drupal::entityTypeManager()->getStorage('node');
     $new_revision = $storage->loadRevision($new_revision_id);
     $this->assertFalse($new_revision->isPublished());
-    $this->assertTrue($new_revision->isDefaultRevision());
+    $this->assertTrue((bool) $new_revision->isDefaultRevision());
   }
 
 }

#110.3 - One more left.

This can also be fixed on commit. Follow-up #3082415: Replace assert(Not)Same/Identical() on booleans with assert(Not)True/False() in PHPUnit tests filed. RTBC when it turns back green.

Review of #3082340: Replace assertEquals() overrides in PHPUnit Kernel, Functional and FunctionalJavascript tests with a MarkupInterfaceComparator implementation would be appreciated ;)

Mmm, maybe #111 cannot be fixed on commit after all, it's code change.

Fixing #111. Minor change aligned with the rest of #110.3, I think I can RTBC.

Needs a re-roll. This is always going to be a problem with a 400+kb patch but also can't really think of a good way to split it up.

Reroll.

Removed usage that was added since #113.

... and the patch ...

As this is a large and disruptive change no point committing it right before the alpha - hopefully this will get it right after.

needs reroll

Reroll. The @trigger_error for assertTrue and assertFalse in AssertLegacyTrait needed to be moved to the compatibility layer.

Reroll to 8.9, note that core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php was already fixed in #2233595

Fixing the last fail...

We now need 8.x.x and 9.0.x versions because of a change in core/modules/system/tests/src/Functional/Theme/TwigTransTest.php due to Twig 2.

The patch in #133 is for 8.x.x and the patch attached here is for 9.0.x.

Needs one more re-roll. I'm hoping to commit this today just before the beta is tagged:

error: patch failed: core/modules/path/tests/src/Functional/PathAliasTest.php:327
error: core/modules/path/tests/src/Functional/PathAliasTest.php: patch does not apply

Rerolled.

Adding commit credit..

Committed/pushed to 9.0.x, 8.9.x and 8.8.x, thanks!

Hotfix for the fail caused by this not retesting after #3082655: Specify the $defaultTheme property in all functional tests landed.

I committed the hotfix after @lauriii confirmed that it fixes the test. Didn't wait for QA because HEAD is broken and we need to have QA be green to do the beta release. So DrupalCI will give us our green test run in this case.

Thanks @tim.plunkett for the hotfix!