Problem/Motivation

As title. We have approximately 100 assertions such as

$this->assertTrue(isset($list['front']) && isset($list['403']) && isset($list['404']), 'Got a list with the right properties for site page data.
');

It is cleaner to assert each of these parts separately.

Steps to reproduce

Proposed resolution

Grep for >assert.*&& (some false positives)
Split each assertion into multiple lines, improving the assertion type where possible
Remove redundant assertion messages

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

core/modules/comment/tests/src/Functional/CommentEntityTest.php:89:
    $this->assertFalse(isset($settings['ajaxPageState']['libraries']) && in_array('comment/drupal.comment-new-indicator', explode(',', $settings['ajaxPageState']['libraries'])), 'drupal.comment-new-indicator library is present.');
    $this->assertFalse(isset($settings['history']['lastReadTimestamps']) && in_array($term->id(), array_keys($settings['history']['lastReadTimestamps'])), 'history.lastReadTimestamps is present.');

Not sure what the intent of this is, as neither ajaxPageState nor history seem to be set, never mind the child items or the part after &&, so this is largely useless.

longwave’s picture

Status: Active » Needs review
FileSize
25.05 KB

Work in progress, there are many more to do.

Status: Needs review » Needs work

The last submitted patch, 3: 3166450-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mondrake’s picture

Title: Split assertions using && into multiple assertions » Split assertTrue using && into multiple assertions
Status: Needs work » Needs review
FileSize
29.34 KB
9.1 KB

Baby steps forward. We can only convert assertTrue, since assertFalse($a && $b); requires OR logic, not AND.

Status: Needs review » Needs work

The last submitted patch, 5: 3166450-5.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
43.57 KB
15.31 KB

Still about 40 to do.

mondrake’s picture

Status: Needs review » Needs work

The last submitted patch, 8: 3166450-8.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
71.45 KB
16.45 KB

This should do. There's 1 more instance searching with assertTrue\(.+&&, and that's in a deprecated method in AssertLegacyTrait, that I would leave to rest there.

Status: Needs review » Needs work

The last submitted patch, 10: 3166450-10.patch, failed testing. View results

mondrake’s picture

Status: Needs work » Needs review
FileSize
71.45 KB
5.37 KB

Fixes.

longwave’s picture

Status: Needs review » Needs work

Thanks for picking this up, I assume you're happy with what I did in #3 except for the test fails, then I can RTBC this once the review issues are fixed.

  1. +++ b/core/modules/search/tests/src/Functional/SearchAdvancedSearchFormTest.php
    @@ -101,12 +101,10 @@ public function testFormRefill() {
    -        $elements = $this->xpath('//input[@name=:name]', [':name' => $key]);
    -        $this->assertTrue(isset($elements[0]) && $elements[0]->getValue() == $value, "Field $key is set to $value");
    +        $this->assertSession()->fieldValueEquals($key, $value);
           }
           else {
    -        $elements = $this->xpath('//input[@name=:name]', [':name' => $key]);
    -        $this->assertTrue(isset($elements[0]) && !empty($elements[0]->getAttribute('checked')), "Field $key is checked");
    +        $this->assertSession()->checkboxChecked($key);
    

    So much nicer!

  2. +++ b/core/tests/Drupal/KernelTests/Core/Cache/GenericCacheBackendUnitTestBase.php
    @@ -138,7 +138,8 @@ public function testSetGet() {
    -    $this->assertTrue($cached->created >= REQUEST_TIME && $cached->created <= round(microtime(TRUE), 3), 'Created time is correct.');
    +    $this->assertLessThanOrEqual($cached->created, REQUEST_TIME);
    +    $this->assertGreaterThanOrEqual($cached->created, round(microtime(TRUE), 3));
    

    I think $expected and $actual are the wrong way round here (and therefore LessThan should be GreaterThan and vice versa), same for a number of other instances in this file.

mondrake’s picture

Status: Needs work » Needs review
FileSize
74.05 KB
5.37 KB

@longwave yes, I was happy with #3. Done #13.2.

mondrake’s picture

Forgot merging with HEAD.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
snehalgaikwad’s picture

Assigned: Unassigned » snehalgaikwad
snehalgaikwad’s picture

Assigned: snehalgaikwad » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
71.36 KB

Rerolled the patch for 9.1.x.

longwave’s picture

Status: Needs review » Needs work

I read through all changes in the patch and only have one comment that doesn't need changes, and one question:

  1. +++ b/core/modules/field/tests/src/Kernel/BulkDeleteTest.php
    @@ -397,7 +398,8 @@ public function testPurgeFieldStorage() {
    +    $this->assertArrayHasKey($field->uuid(), $fields);
    +    $this->assertTrue($fields[$field->uuid()]->isDeleted());
    

    I do wonder if it's overkill asserting that the key exists first, as the second line will fail anyway with an error if the key isn't set. I guess really it doesn't matter though, this and similar instances can be left as-is.

  2. +++ b/core/modules/locale/tests/src/Functional/LocaleTranslationUiTest.php
    @@ -141,13 +141,15 @@ public function testStringTranslation() {
    -    $this->assertTrue($name != $translation && t($name, [], ['langcode' => $langcode]) == $translation, 't() works for non-English.');
    +    $this->assertNotEquals($name, $translation);
    +    $this->assertEquals(t($name, [], ['langcode' => $langcode]), $translation, 't() works for non-English.');
    ...
    -    $this->assertTrue($name != $translation_to_en && t($name, [], ['langcode' => 'en']) == $translation_to_en, 't() works for English.');
    +    $this->assertNotEquals($name, $translation_to_en);
    +    $this->assertEquals(t($name, [], ['langcode' => 'en']), $translation_to_en, 't() works for English.');
    

    I think $expected and $actual are the wrong way round here?

Otherwise I think this is good to go.

mondrake’s picture

Status: Needs work » Needs review
FileSize
71.21 KB
1.62 KB

Thanks.

#19.1 I personally agree that it seems overkill, but here it's a one-to-one conversion of previous code. One way or another, I'd leave to core committers to give input here.

#19.2 Indeed, done.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks perfect now - RTBC assuming testbot agrees.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 3166450-20.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

catch’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed d834847 and pushed to 9.2.x. Thanks!

  • catch committed 7dbdbcb on 9.2.x
    Issue #3166450 by mondrake, longwave, snehalgaikwad: Split assertTrue...

  • catch committed d834847 on 9.1.x
    Issue #3166450 by mondrake, longwave, snehalgaikwad: Split assertTrue...
catch’s picture

Status: Reviewed & tested by the community » Fixed

And cherry-picked to 9.1.x

xjm’s picture

Version: 9.2.x-dev » 9.1.x-dev

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.