From: Aryeh Gregor Date: Sun, 18 Mar 2018 18:48:51 +0000 (+0200) Subject: Improve test coverage for ApiBlock.php to 100% X-Git-Tag: 1.31.0-rc.0~290^2 X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=6e93b3bfaa0bfd78c05f5025d53ae558fae7a824 Improve test coverage for ApiBlock.php to 100% The code coverage tool still reports that line 50 ("$status,") is not covered, but this is just a parameter to a function that is in fact called and does show up as covered, so I'm pretty sure it's a bug. I replaced a couple of sanity checks that reported "incomplete" on failure with actual assertions so that the test would fail properly if the sanity check failed. The testing could still probably be expanded considerably. Change-Id: Ib6ba7227af23bcb50c7e3bafb0b51395e8acf03f --- diff --git a/includes/api/ApiBlock.php b/includes/api/ApiBlock.php index f4aea986f0..8f4028310a 100644 --- a/includes/api/ApiBlock.php +++ b/includes/api/ApiBlock.php @@ -124,8 +124,8 @@ class ApiBlock extends ApiBase { $res['id'] = $block->getId(); } else { # should be unreachable - $res['expiry'] = ''; - $res['id'] = ''; + $res['expiry'] = ''; // @codeCoverageIgnore + $res['id'] = ''; // @codeCoverageIgnore } $res['reason'] = $params['reason']; diff --git a/tests/phpunit/includes/api/ApiBlockTest.php b/tests/phpunit/includes/api/ApiBlockTest.php index 832a113f0e..c0cf87eedf 100644 --- a/tests/phpunit/includes/api/ApiBlockTest.php +++ b/tests/phpunit/includes/api/ApiBlockTest.php @@ -8,13 +8,17 @@ * @covers ApiBlock */ class ApiBlockTest extends ApiTestCase { + protected $mUser = null; + protected function setUp() { parent::setUp(); $this->doLogin(); + + $this->mUser = $this->getMutableTestUser()->getUser(); } protected function tearDown() { - $block = Block::newFromTarget( 'UTApiBlockee' ); + $block = Block::newFromTarget( $this->mUser->getName() ); if ( !is_null( $block ) ) { $block->delete(); } @@ -25,80 +29,192 @@ class ApiBlockTest extends ApiTestCase { return $this->getTokenList( self::$users['sysop'] ); } - function addDBDataOnce() { - $user = User::newFromName( 'UTApiBlockee' ); - - if ( $user->getId() == 0 ) { - $user->addToDatabase(); - TestUser::setPasswordForUser( $user, 'UTApiBlockeePassword' ); - - $user->saveSettings(); - } - } - /** - * This test has probably always been broken and use an invalid token - * Bug tracking brokenness is https://phabricator.wikimedia.org/T37646 - * - * Root cause is https://gerrit.wikimedia.org/r/3434 - * Which made the Block/Unblock API to actually verify the token - * previously always considered valid (T36212). + * @param array $extraParams Extra API parameters to pass to doApiRequest + * @param User $blocker User to do the blocking, null to pick + * arbitrarily */ - public function testMakeNormalBlock() { - $tokens = $this->getTokens(); + private function doBlock( array $extraParams = [], User $blocker = null ) { + if ( $blocker === null ) { + $blocker = self::$users['sysop']->getUser(); + } - $user = User::newFromName( 'UTApiBlockee' ); + $tokens = $this->getTokens(); - if ( !$user->getId() ) { - $this->markTestIncomplete( "The user UTApiBlockee does not exist" ); - } + $this->assertNotNull( $this->mUser, 'Sanity check' ); + $this->assertNotSame( 0, $this->mUser->getId(), 'Sanity check' ); - if ( !array_key_exists( 'blocktoken', $tokens ) ) { - $this->markTestIncomplete( "No block token found" ); - } + $this->assertArrayHasKey( 'blocktoken', $tokens, 'Sanity check' ); - $this->doApiRequest( [ + $params = [ 'action' => 'block', - 'user' => 'UTApiBlockee', + 'user' => $this->mUser->getName(), 'reason' => 'Some reason', - 'token' => $tokens['blocktoken'] ], null, false, self::$users['sysop']->getUser() ); + 'token' => $tokens['blocktoken'], + ]; + if ( array_key_exists( 'userid', $extraParams ) ) { + // Make sure we don't have both user and userid + unset( $params['user'] ); + } + $ret = $this->doApiRequest( array_merge( $params, $extraParams ), null, + false, $blocker ); - $block = Block::newFromTarget( 'UTApiBlockee' ); + $block = Block::newFromTarget( $this->mUser->getName() ); $this->assertTrue( !is_null( $block ), 'Block is valid' ); - $this->assertEquals( 'UTApiBlockee', (string)$block->getTarget() ); - $this->assertEquals( 'Some reason', $block->mReason ); - $this->assertEquals( 'infinity', $block->mExpiry ); + $this->assertSame( $this->mUser->getName(), (string)$block->getTarget() ); + $this->assertSame( 'Some reason', $block->mReason ); + + return $ret; + } + + /** + * Block by username + */ + public function testNormalBlock() { + $this->doBlock(); } /** * Block by user ID */ - public function testMakeNormalBlockId() { - $tokens = $this->getTokens(); - $user = User::newFromName( 'UTApiBlockee' ); + public function testBlockById() { + $this->doBlock( [ 'userid' => $this->mUser->getId() ] ); + } - if ( !$user->getId() ) { - $this->markTestIncomplete( "The user UTApiBlockee does not exist." ); - } + /** + * A blocked user can't block + */ + public function testBlockByBlockedUser() { + $this->setExpectedException( ApiUsageException::class, + 'You cannot block or unblock other users because you are yourself blocked.' ); + + $blocked = $this->getMutableTestUser( [ 'sysop' ] )->getUser(); + $block = new Block( [ + 'address' => $blocked->getName(), + 'by' => self::$users['sysop']->getUser()->getId(), + 'reason' => 'Capriciousness', + 'timestamp' => '19370101000000', + 'expiry' => 'infinity', + ] ); + $block->insert(); + + $this->doBlock( [], $blocked ); + } - if ( !array_key_exists( 'blocktoken', $tokens ) ) { - $this->markTestIncomplete( "No block token found" ); - } + public function testBlockOfNonexistentUser() { + $this->setExpectedException( ApiUsageException::class, + 'There is no user by the name "Nonexistent". Check your spelling.' ); - $data = $this->doApiRequest( [ - 'action' => 'block', - 'userid' => $user->getId(), - 'reason' => 'Some reason', - 'token' => $tokens['blocktoken'] ], null, false, self::$users['sysop']->getUser() ); + $this->doBlock( [ 'user' => 'Nonexistent' ] ); + } + + public function testBlockOfNonexistentUserId() { + $id = 948206325; + $this->setExpectedException( ApiUsageException::class, + "There is no user with ID $id." ); + + $this->assertFalse( User::whoIs( $id ), 'Sanity check' ); + + $this->doBlock( [ 'userid' => $id ] ); + } + + public function testBlockWithTag() { + ChangeTags::defineTag( 'custom tag' ); + + $this->doBlock( [ 'tags' => 'custom tag' ] ); + + $dbw = wfGetDB( DB_MASTER ); + $this->assertSame( 'custom tag', $dbw->selectField( + [ 'change_tag', 'logging' ], + 'ct_tag', + [ 'log_type' => 'block' ], + __METHOD__, + [], + [ 'change_tag' => [ 'INNER JOIN', 'ct_log_id = log_id' ] ] + ) ); + } + + public function testBlockWithProhibitedTag() { + $this->setExpectedException( ApiUsageException::class, + 'You do not have permission to apply change tags along with your changes.' ); + + ChangeTags::defineTag( 'custom tag' ); + + $this->setMwGlobals( 'wgRevokePermissions', + [ 'user' => [ 'applychangetags' => true ] ] ); + + $this->doBlock( [ 'tags' => 'custom tag' ] ); + } + + public function testBlockWithHide() { + global $wgGroupPermissions; + $newPermissions = $wgGroupPermissions['sysop']; + $newPermissions['hideuser'] = true; + $this->mergeMwGlobalArrayValue( 'wgGroupPermissions', + [ 'sysop' => $newPermissions ] ); + + $res = $this->doBlock( [ 'hidename' => '' ] ); + + $dbw = wfGetDB( DB_MASTER ); + $this->assertSame( '1', $dbw->selectField( + 'ipblocks', + 'ipb_deleted', + [ 'ipb_id' => $res[0]['block']['id'] ], + __METHOD__ + ) ); + } + + public function testBlockWithProhibitedHide() { + $this->setExpectedException( ApiUsageException::class, + "You don't have permission to hide user names from the block log." ); + + $this->doBlock( [ 'hidename' => '' ] ); + } + + public function testBlockWithEmailBlock() { + $res = $this->doBlock( [ 'noemail' => '' ] ); + + $dbw = wfGetDB( DB_MASTER ); + $this->assertSame( '1', $dbw->selectField( + 'ipblocks', + 'ipb_block_email', + [ 'ipb_id' => $res[0]['block']['id'] ], + __METHOD__ + ) ); + } + + public function testBlockWithProhibitedEmailBlock() { + $this->setExpectedException( ApiUsageException::class, + "You don't have permission to block users from sending email through the wiki." ); + + $this->setMwGlobals( 'wgRevokePermissions', + [ 'sysop' => [ 'blockemail' => true ] ] ); + + $this->doBlock( [ 'noemail' => '' ] ); + } + + public function testBlockWithExpiry() { + $res = $this->doBlock( [ 'expiry' => '1 day' ] ); + + $dbw = wfGetDB( DB_MASTER ); + $expiry = $dbw->selectField( + 'ipblocks', + 'ipb_expiry', + [ 'ipb_id' => $res[0]['block']['id'] ], + __METHOD__ + ); + + // Allow flakiness up to one second + $this->assertLessThan( 1, + abs( wfTimestamp( TS_UNIX, $expiry ) - ( time() + 86400 ) ) ); + } - $block = Block::newFromTarget( 'UTApiBlockee' ); + public function testBlockWithInvalidExpiry() { + $this->setExpectedException( ApiUsageException::class, "Expiry time invalid." ); - $this->assertTrue( !is_null( $block ), 'Block is valid.' ); - $this->assertEquals( 'UTApiBlockee', (string)$block->getTarget() ); - $this->assertEquals( 'Some reason', $block->mReason ); - $this->assertEquals( 'infinity', $block->mExpiry ); + $this->doBlock( [ 'expiry' => '' ] ); } /** @@ -109,7 +225,7 @@ class ApiBlockTest extends ApiTestCase { $this->doApiRequest( [ 'action' => 'block', - 'user' => 'UTApiBlockee', + 'user' => $this->mUser->getName(), 'reason' => 'Some reason', ], null,