Problem/Motivation

There is a wrong assertion in taxonomy.test that always evaluates to TRUE.

$this->assertTrue(count($terms4 == 4), 'Correct number of terms were loaded.');

This is always TRUE. It should at least be count($terms4) == 4. However, it would be better to replace the assertion with assertEqual(): $this->assertEqual(count($terms4), 4, ...).

Proposed resolution

  • Change the mentioned line of code to assertEqual() and fix the count().
  • Look for other places in the file that could be done using assertEqual() instead of assertTrue() to simplify the code.

Remaining tasks

This should only be done once both #1160566: entity_load() is actually entity_load_multiple() and #1195254: Taxonomy test cleanup have been commited (the the second already has been commited to 8.x, only needs to be commited to 7.x, the first will not be backported) because both patches currently touch that line or come too close so that it will need a re-roll.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue tags: +Needs backport to D7

Updated the summary.

damiankloip’s picture

Status: Active » Needs review
FileSize
684 bytes

Here is a D7 patch for this.

Status: Needs review » Needs work

The last submitted patch, 1542674-d7.patch, failed testing.

damiankloip’s picture

Status: Needs work » Active

Ignore the last patch, too premature.

xjm’s picture

Looks like it was just rolled against D7. :) It needs to be fixed in D8 first.

damiankloip’s picture

Yep, I got it all wrong on this one I'm afraid :)

bas.hr’s picture

FileSize
3.59 KB

Here is a D8 patch with assertEqual instead of assertTrue where appropriate.

bas.hr’s picture

Status: Active » Needs review
damiankloip’s picture

Isn't this issue still waiting on other issues above?

Berdir’s picture

Issue tags: -Novice, -Needs backport to D7

#7: 1542674-7.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs backport to D7

The last submitted patch, 1542674-7.patch, failed testing.

bas.hr’s picture

Status: Needs work » Needs review
FileSize
3.59 KB

Re-rolled to apply after #1160566: entity_load() is actually entity_load_multiple() has been commited

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Dries’s picture

Version: 8.x-dev » 7.x-dev

Nice catch, and good clean-up. Committed to 8.x. Moving to 7.x.

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev

It doesn't look like this was pushed to D8 yet.

tim.plunkett’s picture

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
3.58 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, drupal-1542674-17.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.58 KB

I mismatched the parenthesis on that weird count() line.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Looks correct.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

wow, nice catch. committed and pushed to 7.x. thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.