Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#39219 closed enhancement (fixed)

Add `assertNotFalse` method to `WP_UnitTestCase`.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.8
Component: Build/Test Tools Keywords: has-patch fixed-major
Focuses: Cc:

Description

The version of phpunit WP uses on older versions of PHP doesn't include the assertNotFalse method.

Attachments (2)

39219.diff (752 bytes) - added by peterwilsoncc 7 years ago.
39219-unit-tests-to-replace.diff (2.4 KB) - added by peterwilsoncc 7 years ago.

Download all attachments as: .zip

Change History (11)

@peterwilsoncc
7 years ago

#1 @peterwilsoncc
7 years ago

  • Keywords has-patch added; needs-patch removed
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

In 39219.diff.

I've tested locally that it passes with all falsy values except false. I'm not sure where I should put these.

$this->assertNotFalse( 0 );
$this->assertNotFalse( null );
$this->assertNotFalse( '' );
$this->assertNotFalse( array() );
$this->assertNotFalse( 0.0 );
$this->assertNotFalse( '0' );

#2 follow-up: @dd32
7 years ago

@peterwilsoncc Confirmed that the logic matches what is in PHPUnit: https://github.com/sebastianbergmann/phpunit/blob/9758eee6421efd997a42fe850f74292a490e9c70/src/Framework/Assert.php#L1185-L1196 so this seems fine.

Is it possible to find the occurrences in the current unit tests where this was to be used, but avoided?

#3 in reply to: ↑ 2 @peterwilsoncc
7 years ago

Replying to dd32:

Is it possible to find the occurrences in the current unit tests where this was to be used, but avoided?

Uploaded in 39219-unit-tests-to-replace.diff, it replaces assertNotSame( false, $actual ) with assertNotFalse( $actual ). There are a few assertNotEquals() calls that look like they could be replaced but I'd prefer to be conservative.

I've kept the tests seperate, do you think they can be committed as part of this ticket or should it be a follow up?

#4 @swissspidy
7 years ago

In addition to assertNotSame() there are a few instances of assertTrue( false === … ) instead of assertFalse() and assertTrue( false !== ) instead of assertNotFalse(). They should be updated as well.

#6 @SergeyBiryukov
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39919:

Build/Test Tools: Add assertNotFalse() method to WP_UnitTestCase and use it where appropriate.

Props peterwilsoncc.
Fixes #39219.

#7 @swissspidy
7 years ago

  • Milestone changed from 4.8 to 4.7.4

#8 @swissspidy
7 years ago

  • Keywords fixed-major added

#9 @swissspidy
7 years ago

In 40388:

Build/Test Tools: Add assertNotFalse() method to WP_UnitTestCase and use it where appropriate.

Props peterwilsoncc.
Fixes #39219.

Merges [39919] to the 4.7 branch.

Note: See TracTickets for help on using tickets.