From f1ca6b8ca9c2dc65014bc8ae79f02158d2dc52c3 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bartosz=20Dziewo=C5=84ski?= Date: Mon, 13 Nov 2017 19:42:33 +0100 Subject: [PATCH] ApiOptionsTest: Do not use ->at() Quoting PHPUnit docs: The $index parameter for the at() matcher refers to the index, starting at zero, in all method invocations for a given mock object. Exercise caution when using this matcher as it can lead to brittle tests which are too closely tied to specific implementation details. Indeed these test cases would break horribly with unintuitive error messages ("Mocked method does not exist") if anything in preferences or API code called any additional methods on the mocked user. For example, it relied on the caching in Preferences::getPreferences(), which is being removed in I92390120a16448383a25e9ba2dd35a434a2f21bf. I'm pretty sure all that matters here is that all the setOption() calls with different arguments happen, so let's test just that. Change-Id: I30a814151a006e5f147eebb918344049807b2b97 --- tests/phpunit/includes/api/ApiOptionsTest.php | 63 ++++++------------- 1 file changed, 18 insertions(+), 45 deletions(-) diff --git a/tests/phpunit/includes/api/ApiOptionsTest.php b/tests/phpunit/includes/api/ApiOptionsTest.php index ef70626120..7e45f4da12 100644 --- a/tests/phpunit/includes/api/ApiOptionsTest.php +++ b/tests/phpunit/includes/api/ApiOptionsTest.php @@ -278,26 +278,13 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->never() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 2 ) ) - ->method( 'getOptions' ); - - $this->mUserMock->expects( $this->at( 5 ) ) + $this->mUserMock->expects( $this->exactly( 3 ) ) ->method( 'setOption' ) - ->with( $this->equalTo( 'willBeNull' ), $this->identicalTo( null ) ); - - $this->mUserMock->expects( $this->at( 6 ) ) - ->method( 'getOptions' ); - - $this->mUserMock->expects( $this->at( 7 ) ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'willBeEmpty' ), $this->equalTo( '' ) ); - - $this->mUserMock->expects( $this->at( 8 ) ) - ->method( 'getOptions' ); - - $this->mUserMock->expects( $this->at( 9 ) ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ); + ->withConsecutive( + [ $this->equalTo( 'willBeNull' ), $this->identicalTo( null ) ], + [ $this->equalTo( 'willBeEmpty' ), $this->equalTo( '' ) ], + [ $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ] + ); $this->mUserMock->expects( $this->once() ) ->method( 'saveSettings' ); @@ -315,19 +302,12 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->once() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 5 ) ) - ->method( 'getOptions' ); - - $this->mUserMock->expects( $this->at( 6 ) ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ); - - $this->mUserMock->expects( $this->at( 7 ) ) - ->method( 'getOptions' ); - - $this->mUserMock->expects( $this->at( 8 ) ) + $this->mUserMock->expects( $this->exactly( 2 ) ) ->method( 'setOption' ) - ->with( $this->equalTo( 'name' ), $this->equalTo( 'value' ) ); + ->withConsecutive( + [ $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ], + [ $this->equalTo( 'name' ), $this->equalTo( 'value' ) ] + ); $this->mUserMock->expects( $this->once() ) ->method( 'saveSettings' ); @@ -348,21 +328,14 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->never() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 4 ) ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'testmultiselect-opt1' ), $this->identicalTo( true ) ); - - $this->mUserMock->expects( $this->at( 5 ) ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'testmultiselect-opt2' ), $this->identicalTo( null ) ); - - $this->mUserMock->expects( $this->at( 6 ) ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'testmultiselect-opt3' ), $this->identicalTo( false ) ); - - $this->mUserMock->expects( $this->at( 7 ) ) + $this->mUserMock->expects( $this->exactly( 4 ) ) ->method( 'setOption' ) - ->with( $this->equalTo( 'testmultiselect-opt4' ), $this->identicalTo( false ) ); + ->withConsecutive( + [ $this->equalTo( 'testmultiselect-opt1' ), $this->identicalTo( true ) ], + [ $this->equalTo( 'testmultiselect-opt2' ), $this->identicalTo( null ) ], + [ $this->equalTo( 'testmultiselect-opt3' ), $this->identicalTo( false ) ], + [ $this->equalTo( 'testmultiselect-opt4' ), $this->identicalTo( false ) ] + ); $this->mUserMock->expects( $this->once() ) ->method( 'saveSettings' ); -- 2.20.1