From 0a83eb9b5df59047fbf64a997e5e6ffcd1299f80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= Date: Thu, 13 Jun 2024 04:33:59 +0200 Subject: [PATCH] FilterValidatorTest: Use MediaWiki core status assertions Depends-On: Ie4b3ebc03abb0e352e82394ced6ab9e733c83fb4 Depends-On: I8718cf7890f05c09a6e5712ee3dc4d171a6637cf Change-Id: I6cb0cee65646b2b108319df6a9f862cbdd881691 --- tests/phpunit/unit/FilterValidatorTest.php | 135 +++++++++++---------- 1 file changed, 70 insertions(+), 65 deletions(-) diff --git a/tests/phpunit/unit/FilterValidatorTest.php b/tests/phpunit/unit/FilterValidatorTest.php index fbb1813d6..d1bb93875 100644 --- a/tests/phpunit/unit/FilterValidatorTest.php +++ b/tests/phpunit/unit/FilterValidatorTest.php @@ -86,64 +86,59 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { } /** - * Helper to check that $expected is null and $actual is good, or the messages match - * @param string|null $expected - * @param Status $actual - * @param array|null $params + * Helper to check that $expectedError is null and $actual is good, or the messages match. */ - private function assertStatusMessageParams( ?string $expected, Status $actual, array $params = null ): void { - $actualError = $actual->isGood() ? null : $actual->getMessages()[0]->getKey(); - $this->assertSame( $expected, $actualError, 'status message' ); - if ( $params !== null ) { - $this->assertSame( $params, $actual->getMessages()[0]->getParams() ); + private function assertFilterValidatorStatus( ?string $expectedError, Status $actual ): void { + if ( $expectedError ) { + // Most of the FilterValidator error statuses are marked as non-fatal errors, and + // assertStatusWarning() (despite its name) checks for non-fatal errors, not warnings. + // The name and the distinction is confusing. See also T309859. + $this->assertStatusWarning( $expectedError, $actual ); + } else { + $this->assertStatusGood( $actual ); } } /** * @param ExceptionBase|null $excep - * @param string|null $expected - * @param array|null $expParams + * @param Status $expected * @dataProvider provideSyntax */ - public function testCheckValidSyntax( ?ExceptionBase $excep, ?string $expected, ?array $expParams ) { + public function testCheckValidSyntax( ?ExceptionBase $excep, Status $expected ) { $ruleChecker = $this->createMock( FilterEvaluator::class ); $syntaxStatus = new ParserStatus( $excep, [], 1 ); $ruleChecker->method( 'checkSyntax' )->willReturn( $syntaxStatus ); $validator = $this->getFilterValidator( null, $ruleChecker ); - $this->assertStatusMessageParams( - $expected, - $validator->checkValidSyntax( $this->createMock( AbstractFilter::class ) ), - $expParams - ); + $actual = $validator->checkValidSyntax( $this->createMock( AbstractFilter::class ) ); + $this->assertStatusMessagesExactly( $expected, $actual ); } public function provideSyntax(): Generator { - yield 'valid' => [ null, null, null ]; + yield 'valid' => [ null, Status::newGood() ]; $excText = 'Internal error text'; yield 'invalid, internal error' => [ new InternalException( $excText ), - 'abusefilter-edit-badsyntax', - [ $excText ] + Status::newFatal( 'abusefilter-edit-badsyntax', $excText ) ]; $excMsg = $this->getMockMessage( $excText ); $excep = $this->createMock( UserVisibleException::class ); $excep->method( 'getMessageObj' )->willReturn( $excMsg ); - yield 'invalid, user error' => [ $excep, 'abusefilter-edit-badsyntax', [ $excMsg ] ]; + yield 'invalid, user error' => [ $excep, Status::newFatal( 'abusefilter-edit-badsyntax', $excMsg ) ]; } /** * @param string $rules * @param string $name - * @param string|null $expected + * @param string|null $expectedError * @dataProvider provideRequiredFields */ - public function testCheckRequiredFields( string $rules, string $name, ?string $expected ) { + public function testCheckRequiredFields( string $rules, string $name, ?string $expectedError ) { $filter = $this->createMock( AbstractFilter::class ); $filter->method( 'getRules' )->willReturn( $rules ); $filter->method( 'getName' )->willReturn( $name ); - $validator = $this->getFilterValidator(); - $this->assertStatusMessageParams( $expected, $validator->checkRequiredFields( $filter ) ); + $actual = $this->getFilterValidator()->checkRequiredFields( $filter ); + $this->assertFilterValidatorStatus( $expectedError, $actual ); } public static function provideRequiredFields(): array { @@ -157,12 +152,12 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { /** * @param array $actions - * @param string|null $expected + * @param string|null $expectedError * @dataProvider provideEmptyMessages */ - public function testCheckEmptyMessages( array $actions, ?string $expected ) { - $filter = $this->getFilterWithActions( $actions ); - $this->assertStatusMessageParams( $expected, $this->getFilterValidator()->checkEmptyMessages( $filter ) ); + public function testCheckEmptyMessages( array $actions, ?string $expectedError ) { + $actual = $this->getFilterValidator()->checkEmptyMessages( $this->getFilterWithActions( $actions ) ); + $this->assertFilterValidatorStatus( $expectedError, $actual ); } public static function provideEmptyMessages(): array { @@ -178,14 +173,15 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { /** * @param bool $enabled * @param bool $deleted - * @param string|null $expected + * @param string|null $expectedError * @dataProvider provideConflictingFields */ - public function testCheckConflictingFields( bool $enabled, bool $deleted, ?string $expected ) { + public function testCheckConflictingFields( bool $enabled, bool $deleted, ?string $expectedError ) { $filter = $this->createMock( AbstractFilter::class ); $filter->method( 'isEnabled' )->willReturn( $enabled ); $filter->method( 'isDeleted' )->willReturn( $deleted ); - $this->assertStatusMessageParams( $expected, $this->getFilterValidator()->checkConflictingFields( $filter ) ); + $actual = $this->getFilterValidator()->checkConflictingFields( $filter ); + $this->assertFilterValidatorStatus( $expectedError, $actual ); } public static function provideConflictingFields(): array { @@ -198,13 +194,13 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { /** * @param bool $canEditNew * @param bool $canEditOrig - * @param string|null $expected + * @param string|null $expectedError * @dataProvider provideCheckGlobalFilterEditPermission */ public function testCheckGlobalFilterEditPermission( bool $canEditNew, bool $canEditOrig, - ?string $expected + ?string $expectedError ) { $permManager = $this->createMock( AbuseFilterPermissionManager::class ); $permManager->method( 'canEditFilter' )->willReturnOnConsecutiveCalls( $canEditNew, $canEditOrig ); @@ -214,7 +210,11 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { $this->createMock( AbstractFilter::class ), $this->createMock( AbstractFilter::class ) ); - $this->assertStatusMessageParams( $expected, $actual ); + if ( $expectedError ) { + $this->assertStatusError( $expectedError, $actual ); + } else { + $this->assertStatusGood( $actual ); + } } public static function provideCheckGlobalFilterEditPermission(): array { @@ -229,16 +229,14 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { /** * @param array $actions * @param bool $isGlobal - * @param string|null $expected + * @param string|null $expectedError * @dataProvider provideMessagesOnGlobalFilters */ - public function testCheckMessagesOnGlobalFilters( array $actions, bool $isGlobal, ?string $expected ) { + public function testCheckMessagesOnGlobalFilters( array $actions, bool $isGlobal, ?string $expectedError ) { $filter = $this->getFilterWithActions( $actions ); $filter->method( 'isGlobal' )->willReturn( $isGlobal ); - $this->assertStatusMessageParams( - $expected, - $this->getFilterValidator()->checkMessagesOnGlobalFilters( $filter ) - ); + $actual = $this->getFilterValidator()->checkMessagesOnGlobalFilters( $filter ); + $this->assertFilterValidatorStatus( $expectedError, $actual ); } public static function provideMessagesOnGlobalFilters(): array { @@ -272,7 +270,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { * @param AbstractFilter $oldFilter * @param array $restrictions * @param AbuseFilterPermissionManager $permManager - * @param string|null $expected + * @param string|null $expectedError * @dataProvider provideRestrictedActions */ public function testCheckRestrictedActions( @@ -280,14 +278,12 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { AbstractFilter $oldFilter, array $restrictions, AbuseFilterPermissionManager $permManager, - ?string $expected + ?string $expectedError ) { $validator = $this->getFilterValidator( $permManager, null, $restrictions ); $performer = $this->createMock( Authority::class ); - $this->assertStatusMessageParams( - $expected, - $validator->checkRestrictedActions( $performer, $newFilter, $oldFilter ) - ); + $actual = $validator->checkRestrictedActions( $performer, $newFilter, $oldFilter ); + $this->assertFilterValidatorStatus( $expectedError, $actual ); } public function provideRestrictedActions(): Generator { @@ -358,7 +354,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { $filter = $this->createMock( AbstractFilter::class ); $filter->method( 'getRules' )->willReturn( 'user_unnamed_ip' ); $filter->method( 'isProtected' )->willReturn( false ); - $this->assertStatusMessageParams( + $this->assertFilterValidatorStatus( 'abusefilter-edit-protected-variable-not-protected', $this->getFilterValidator()->checkProtectedVariables( $filter ) ); @@ -387,7 +383,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { $permManager->method( 'shouldProtectFilter' )->willReturn( [ 'user_unnamed_ip' ] ); $filter = $this->createMock( AbstractFilter::class ); $filter->method( 'getRules' )->willReturn( $data[ 'rules' ] ); - $this->assertStatusMessageParams( + $this->assertFilterValidatorStatus( 'abusefilter-edit-protected-variable', $this->getFilterValidator( $permManager )->checkCanViewProtectedVariables( $performer, $filter ) ); @@ -431,7 +427,10 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { } public function testCheckAllTags_noTags() { - $this->assertStatusMessageParams( 'tags-create-no-name', $this->getFilterValidator()->checkAllTags( [] ) ); + $this->assertFilterValidatorStatus( + 'tags-create-no-name', + $this->getFilterValidator()->checkAllTags( [] ) + ); } /** @@ -440,8 +439,8 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { * @dataProvider provideThrottleParameters */ public function testCheckThrottleParameters( array $params, ?string $expectedError ) { - $result = $this->getFilterValidator()->checkThrottleParameters( $params ); - $this->assertStatusMessageParams( $expectedError, $result ); + $actual = $this->getFilterValidator()->checkThrottleParameters( $params ); + $this->assertFilterValidatorStatus( $expectedError, $actual ); } /** @@ -472,25 +471,32 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { /** * @param AbstractFilter $newFilter - * @param string|null $expected + * @param string|null $expectedError * @param AbuseFilterPermissionManager|null $permissionManager * @param FilterEvaluator|null $ruleChecker * @param array $restrictions + * @param bool $isFatalError * @dataProvider provideCheckAll */ public function testCheckAll( AbstractFilter $newFilter, - ?string $expected, + ?string $expectedError, AbuseFilterPermissionManager $permissionManager = null, FilterEvaluator $ruleChecker = null, - array $restrictions = [] + array $restrictions = [], + bool $isFatalError = false ) { $validator = $this->getFilterValidator( $permissionManager, $ruleChecker, $restrictions ); $origFilter = $this->createMock( AbstractFilter::class ); - $status = $validator->checkAll( $newFilter, $origFilter, $this->createMock( Authority::class ) ); - $actualError = $status->isGood() ? null : $status->getMessages()[0]->getKey(); - $this->assertSame( $expected, $actualError ); + $actual = $validator->checkAll( $newFilter, $origFilter, $this->createMock( Authority::class ) ); + if ( $expectedError && $isFatalError ) { + $this->assertStatusError( $expectedError, $actual ); + } elseif ( $expectedError ) { + $this->assertStatusWarning( $expectedError, $actual ); + } else { + $this->assertStatusGood( $actual ); + } } public function provideCheckAll(): Generator { @@ -528,7 +534,8 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { $permManager = $this->createMock( AbuseFilterPermissionManager::class ); $permManager->method( 'canEditFilter' )->willReturn( false ); - yield 'global filter, no modify-global' => [ $noopFilter, 'abusefilter-edit-notallowed-global', $permManager ]; + yield 'global filter, no modify-global' => [ $noopFilter, 'abusefilter-edit-notallowed-global', $permManager, + null, [], true ]; $customWarnFilter = $this->getFilterWithActions( [ 'warn' => [ 'foo' ] ] ); $customWarnFilter->method( 'isGlobal' )->willReturn( true ); @@ -562,16 +569,14 @@ class FilterValidatorTest extends MediaWikiUnitTestCase { /** * @param string $group * @param string[] $validGroups - * @param string|null $expected + * @param string|null $expectedError * @dataProvider provideGroups */ - public function testCheckGroup( string $group, array $validGroups, ?string $expected ) { + public function testCheckGroup( string $group, array $validGroups, ?string $expectedError ) { $filter = $this->createMock( AbstractFilter::class ); $filter->expects( $this->atLeastOnce() )->method( 'getGroup' )->willReturn( $group ); - $this->assertStatusMessageParams( - $expected, - $this->getFilterValidator( null, null, [], $validGroups )->checkGroup( $filter ) - ); + $actual = $this->getFilterValidator( null, null, [], $validGroups )->checkGroup( $filter ); + $this->assertFilterValidatorStatus( $expectedError, $actual ); } public static function provideGroups(): Generator {