From f38e0600900ddecf9a3eeef86f66099ea9c3d88b Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jun 2022 11:26:30 +0200 Subject: [PATCH] Fix many issues with the tests - Return type were not correct - willReturn and with confusion Signed-off-by: Carl Schwan --- .../lib/Controller/SettingsController.php | 4 +- apps/federation/lib/DbHandler.php | 3 +- .../Controller/OCSAuthAPIControllerTest.php | 26 +--- .../Controller/SettingsControllerTest.php | 7 +- .../Middleware/AddServerMiddlewareTest.php | 13 +- .../tests/SyncFederationAddressbooksTest.php | 14 +- apps/federation/tests/TrustedServersTest.php | 126 ++++++++---------- 7 files changed, 85 insertions(+), 108 deletions(-) diff --git a/apps/federation/lib/Controller/SettingsController.php b/apps/federation/lib/Controller/SettingsController.php index 8abc2f8af57..8bcdc769de9 100644 --- a/apps/federation/lib/Controller/SettingsController.php +++ b/apps/federation/lib/Controller/SettingsController.php @@ -78,7 +78,7 @@ class SettingsController extends Controller { * @AuthorizedAdminSetting(settings=OCA\Federation\Settings\Admin) * @throws HintException */ - protected function checkServer(string $url): void { + protected function checkServer(string $url): bool { if ($this->trustedServers->isTrustedServer($url) === true) { $message = 'Server is already in the list of trusted servers.'; $hint = $this->l->t('Server is already in the list of trusted servers.'); @@ -90,5 +90,7 @@ class SettingsController extends Controller { $hint = $this->l->t('No server to federate with found'); throw new HintException($message, $hint); } + + return true; } } diff --git a/apps/federation/lib/DbHandler.php b/apps/federation/lib/DbHandler.php index 3142cfd353d..b91c9963f80 100644 --- a/apps/federation/lib/DbHandler.php +++ b/apps/federation/lib/DbHandler.php @@ -83,6 +83,7 @@ class DbHandler { $message = 'Internal failure, Could not add trusted server: ' . $url; $message_t = $this->IL10N->t('Could not add server'); throw new HintException($message, $message_t); + return -1; } /** @@ -215,7 +216,7 @@ class DbHandler { $statement = $query->executeQuery(); $result = $statement->fetch(); $statement->closeCursor(); - return $result['shared_secret']; + return (string)$result['shared_secret']; } /** diff --git a/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php index f48c8352ae1..02e82880f9b 100644 --- a/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php +++ b/apps/federation/tests/Controller/OCSAuthAPIControllerTest.php @@ -32,9 +32,9 @@ use OCA\Federation\DbHandler; use OCA\Federation\TrustedServers; use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\AppFramework\Utility\ITimeFactory; -use OCP\ILogger; use OCP\IRequest; use OCP\Security\ISecureRandom; +use Psr\Log\LoggerInterface; use Test\TestCase; class OCSAuthAPIControllerTest extends TestCase { @@ -60,12 +60,10 @@ class OCSAuthAPIControllerTest extends TestCase { /** @var \PHPUnit\Framework\MockObject\MockObject|ITimeFactory */ private $timeFactory; - - /** @var OCSAuthAPIController */ - private $ocsAuthApi; + private OCSAuthAPIController $ocsAuthApi; /** @var int simulated timestamp */ - private $currentTime = 1234567; + private int $currentTime = 1234567; protected function setUp(): void { parent::setUp(); @@ -75,10 +73,9 @@ class OCSAuthAPIControllerTest extends TestCase { $this->trustedServers = $this->createMock(TrustedServers::class); $this->dbHandler = $this->createMock(DbHandler::class); $this->jobList = $this->createMock(JobList::class); - $this->logger = $this->createMock(ILogger::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->ocsAuthApi = new OCSAuthAPIController( 'federation', $this->request, @@ -96,13 +93,8 @@ class OCSAuthAPIControllerTest extends TestCase { /** * @dataProvider dataTestRequestSharedSecret - * - * @param string $token - * @param string $localToken - * @param bool $isTrustedServer - * @param bool $ok */ - public function testRequestSharedSecret($token, $localToken, $isTrustedServer, $ok) { + public function testRequestSharedSecret(string $token, string $localToken, bool $isTrustedServer, bool $ok): void { $url = 'url'; $this->trustedServers @@ -137,12 +129,8 @@ class OCSAuthAPIControllerTest extends TestCase { /** * @dataProvider dataTestGetSharedSecret - * - * @param bool $isTrustedServer - * @param bool $isValidToken - * @param bool $ok */ - public function testGetSharedSecret($isTrustedServer, $isValidToken, $ok) { + public function testGetSharedSecret(bool $isTrustedServer, bool $isValidToken, bool $ok): void { $url = 'url'; $token = 'token'; @@ -171,7 +159,7 @@ class OCSAuthAPIControllerTest extends TestCase { $this->secureRandom->expects($this->once())->method('generate')->with(32) ->willReturn('secret'); $this->trustedServers->expects($this->once()) - ->method('addSharedSecret')->willReturn($url, 'secret'); + ->method('addSharedSecret')->with($url, 'secret'); } else { $this->secureRandom->expects($this->never())->method('generate'); $this->trustedServers->expects($this->never())->method('addSharedSecret'); diff --git a/apps/federation/tests/Controller/SettingsControllerTest.php b/apps/federation/tests/Controller/SettingsControllerTest.php index c33ee6feb87..a3c66159147 100644 --- a/apps/federation/tests/Controller/SettingsControllerTest.php +++ b/apps/federation/tests/Controller/SettingsControllerTest.php @@ -101,9 +101,10 @@ class SettingsControllerTest extends TestCase { } public function testRemoveServer(): void { - $this->trustedServers->expects($this->once())->method('removeServer') - ->with('url'); - $result = $this->controller->removeServer('url'); + $this->trustedServers->expects($this->once()) + ->method('removeServer') + ->with(1); + $result = $this->controller->removeServer(1); $this->assertTrue($result instanceof DataResponse); $this->assertSame(200, $result->getStatus()); } diff --git a/apps/federation/tests/Middleware/AddServerMiddlewareTest.php b/apps/federation/tests/Middleware/AddServerMiddlewareTest.php index c3370cdbe90..dd1ad500384 100644 --- a/apps/federation/tests/Middleware/AddServerMiddlewareTest.php +++ b/apps/federation/tests/Middleware/AddServerMiddlewareTest.php @@ -31,19 +31,18 @@ use OCA\Federation\Middleware\AddServerMiddleware; use OCP\AppFramework\Http; use OCP\HintException; use OCP\IL10N; -use OCP\ILogger; use Test\TestCase; +use Psr\Log\LoggerInterface; class AddServerMiddlewareTest extends TestCase { - /** @var \PHPUnit\Framework\MockObject\MockObject | ILogger */ + /** @var \PHPUnit\Framework\MockObject\MockObject | LoggerInterface */ private $logger; /** @var \PHPUnit\Framework\MockObject\MockObject | \OCP\IL10N */ private $l10n; - /** @var AddServerMiddleware */ - private $middleware; + private AddServerMiddleware $middleware; /** @var \PHPUnit\Framework\MockObject\MockObject | SettingsController */ private $controller; @@ -51,7 +50,7 @@ class AddServerMiddlewareTest extends TestCase { protected function setUp(): void { parent::setUp(); - $this->logger = $this->getMockBuilder(ILogger::class)->getMock(); + $this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $this->l10n = $this->getMockBuilder(IL10N::class)->getMock(); $this->controller = $this->getMockBuilder(SettingsController::class) ->disableOriginalConstructor()->getMock(); @@ -70,11 +69,11 @@ class AddServerMiddlewareTest extends TestCase { * @param string $hint */ public function testAfterException($exception, $hint) { - $this->logger->expects($this->once())->method('logException'); + $this->logger->expects($this->once())->method('error'); $this->l10n->expects($this->any())->method('t') ->willReturnCallback( - function ($message) { + function (string $message): string { return $message; } ); diff --git a/apps/federation/tests/SyncFederationAddressbooksTest.php b/apps/federation/tests/SyncFederationAddressbooksTest.php index 36dd43e7cd2..73c44c72399 100644 --- a/apps/federation/tests/SyncFederationAddressbooksTest.php +++ b/apps/federation/tests/SyncFederationAddressbooksTest.php @@ -50,11 +50,11 @@ class SyncFederationAddressbooksTest extends \Test\TestCase { public function testSync() { /** @var DbHandler | \PHPUnit\Framework\MockObject\MockObject $dbHandler */ - $dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler')-> - disableOriginalConstructor()-> - getMock(); - $dbHandler->method('getAllServer')-> - willReturn([ + $dbHandler = $this->getMockBuilder('OCA\Federation\DbHandler') + ->disableOriginalConstructor() + ->getMock(); + $dbHandler->method('getAllServer') + ->willReturn([ [ 'url' => 'https://cloud.drop.box', 'url_hash' => 'sha1', @@ -68,14 +68,14 @@ class SyncFederationAddressbooksTest extends \Test\TestCase { ->disableOriginalConstructor() ->getMock(); $syncService->expects($this->once())->method('syncRemoteAddressBook') - ->willReturn(1); + ->willReturn('1'); /** @var \OCA\DAV\CardDAV\SyncService $syncService */ $s = new SyncFederationAddressBooks($dbHandler, $syncService, $this->discoveryService); $s->syncThemAll(function ($url, $ex) { $this->callBacks[] = [$url, $ex]; }); - $this->assertEquals(1, count($this->callBacks)); + $this->assertEquals('1', count($this->callBacks)); } public function testException() { diff --git a/apps/federation/tests/TrustedServersTest.php b/apps/federation/tests/TrustedServersTest.php index 0dc155f0b8d..49ee021c028 100644 --- a/apps/federation/tests/TrustedServersTest.php +++ b/apps/federation/tests/TrustedServersTest.php @@ -35,10 +35,10 @@ use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\Http\Client\IResponse; use OCP\IConfig; -use OCP\ILogger; use OCP\Security\ISecureRandom; use OCP\EventDispatcher\IEventDispatcher; use Test\TestCase; +use Psr\Log\LoggerInterface; class TrustedServersTest extends TestCase { @@ -85,7 +85,7 @@ class TrustedServersTest extends TestCase { $this->httpClientService = $this->getMockBuilder(IClientService::class)->getMock(); $this->httpClient = $this->getMockBuilder(IClient::class)->getMock(); $this->response = $this->getMockBuilder(IResponse::class)->getMock(); - $this->logger = $this->getMockBuilder(ILogger::class)->getMock(); + $this->logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $this->jobList = $this->getMockBuilder(IJobList::class)->getMock(); $this->secureRandom = $this->getMockBuilder(ISecureRandom::class)->getMock(); $this->config = $this->getMockBuilder(IConfig::class)->getMock(); @@ -103,12 +103,7 @@ class TrustedServersTest extends TestCase { ); } - /** - * @dataProvider dataTrueFalse - * - * @param bool $success - */ - public function testAddServer($success) { + public function testAddServer(): void { /** @var \PHPUnit\Framework\MockObject\MockObject|TrustedServers $trustedServers */ $trustedServers = $this->getMockBuilder('OCA\Federation\TrustedServers') ->setConstructorArgs( @@ -130,46 +125,39 @@ class TrustedServersTest extends TestCase { $this->timeFactory->method('getTime') ->willReturn(1234567); $this->dbHandler->expects($this->once())->method('addServer')->with('https://url') - ->willReturn($success); - - if ($success) { - $this->secureRandom->expects($this->once())->method('generate') - ->willReturn('token'); - $this->dbHandler->expects($this->once())->method('addToken')->with('https://url', 'token'); - $this->jobList->expects($this->once())->method('add') - ->with('OCA\Federation\BackgroundJob\RequestSharedSecret', - ['url' => 'https://url', 'token' => 'token', 'created' => 1234567]); - } else { - $this->jobList->expects($this->never())->method('add'); - } - - $this->assertSame($success, - $trustedServers->addServer('url') + ->willReturn(1); + + $this->secureRandom->expects($this->once())->method('generate') + ->willReturn('token'); + $this->dbHandler->expects($this->once())->method('addToken')->with('https://url', 'token'); + $this->jobList->expects($this->once())->method('add') + ->with('OCA\Federation\BackgroundJob\RequestSharedSecret', + ['url' => 'https://url', 'token' => 'token', 'created' => 1234567]); + + $this->assertSame( + $trustedServers->addServer('url'), + 1 ); } - public function dataTrueFalse() { - return [ - [true], - [false] - ]; - } - - public function testAddSharedSecret() { + public function testAddSharedSecret(): void { $this->dbHandler->expects($this->once())->method('addSharedSecret') ->with('url', 'secret'); $this->trustedServers->addSharedSecret('url', 'secret'); } - public function testGetSharedSecret() { - $this->dbHandler->expects($this->once())->method('getSharedSecret') - ->with('url')->willReturn(true); - $this->assertTrue( - $this->trustedServers->getSharedSecret('url') + public function testGetSharedSecret(): void { + $this->dbHandler->expects($this->once()) + ->method('getSharedSecret') + ->with('url') + ->willReturn('secret'); + $this->assertSame( + $this->trustedServers->getSharedSecret('url'), + 'secret' ); } - public function testRemoveServer() { + public function testRemoveServer(): void { $id = 42; $server = ['url_hash' => 'url_hash']; $this->dbHandler->expects($this->once())->method('removeServer')->with($id); @@ -178,7 +166,7 @@ class TrustedServersTest extends TestCase { $this->dispatcher->expects($this->once())->method('dispatchTyped') ->willReturnCallback( function ($event) { - $this->assertTrue($event instanceof \OCP\Federated\Events\TrustedServerRemovedEvent); + $this->assertSame(get_class($event), \OCP\Federation\Events\TrustedServerRemovedEvent::class); /** @var \OCP\Federated\Events\TrustedServerRemovedEvent $event */ $this->assertSame('url_hash', $event->getUrlHash()); } @@ -186,7 +174,7 @@ class TrustedServersTest extends TestCase { $this->trustedServers->removeServer($id); } - public function testGetServers() { + public function testGetServers(): void { $this->dbHandler->expects($this->once())->method('getAllServer')->willReturn(['servers']); $this->assertEquals( @@ -196,8 +184,9 @@ class TrustedServersTest extends TestCase { } - public function testIsTrustedServer() { - $this->dbHandler->expects($this->once())->method('serverExists')->with('url') + public function testIsTrustedServer(): void { + $this->dbHandler->expects($this->once()) + ->method('serverExists')->with('url') ->willReturn(true); $this->assertTrue( @@ -207,26 +196,23 @@ class TrustedServersTest extends TestCase { public function testSetServerStatus() { $this->dbHandler->expects($this->once())->method('setServerStatus') - ->with('url', 'status'); - $this->trustedServers->setServerStatus('url', 'status'); + ->with('url', 1); + $this->trustedServers->setServerStatus('url', 1); } public function testGetServerStatus() { $this->dbHandler->expects($this->once())->method('getServerStatus') - ->with('url')->willReturn(true); - $this->assertTrue( - $this->trustedServers->getServerStatus('url') + ->with('url')->willReturn(1); + $this->assertSame( + $this->trustedServers->getServerStatus('url'), + 1 ); } /** * @dataProvider dataTestIsNextcloudServer - * - * @param int $statusCode - * @param bool $isValidOwnCloudVersion - * @param bool $expected */ - public function testIsNextcloudServer($statusCode, $isValidOwnCloudVersion, $expected) { + public function testIsNextcloudServer(int $statusCode, bool $isValidNextcloudVersion, bool $expected): void { $server = 'server1'; /** @var \PHPUnit\Framework\MockObject\MockObject | TrustedServers $trustedServers */ @@ -243,7 +229,7 @@ class TrustedServersTest extends TestCase { $this->timeFactory ] ) - ->setMethods(['checkOwnCloudVersion']) + ->setMethods(['checkNextcloudVersion']) ->getMock(); $this->httpClientService->expects($this->once())->method('newClient') @@ -256,10 +242,12 @@ class TrustedServersTest extends TestCase { ->willReturn($statusCode); if ($statusCode === 200) { - $trustedServers->expects($this->once())->method('checkOwnCloudVersion') - ->willReturn($isValidOwnCloudVersion); + $this->response->expects($this->once())->method('getBody') + ->willReturn(''); + $trustedServers->expects($this->once())->method('checkNextcloudVersion') + ->willReturn($isValidNextcloudVersion); } else { - $trustedServers->expects($this->never())->method('checkOwnCloudVersion'); + $trustedServers->expects($this->never())->method('checkNextcloudVersion'); } $this->assertSame($expected, @@ -267,7 +255,7 @@ class TrustedServersTest extends TestCase { ); } - public function dataTestIsOwnCloudServer() { + public function dataTestIsNextcloudServer(): array { return [ [200, true, true], [200, false, false], @@ -278,7 +266,7 @@ class TrustedServersTest extends TestCase { /** * @expectedExceptionMessage simulated exception */ - public function testIsOwnCloudServerFail() { + public function testIsNextcloudServerFail(): void { $server = 'server1'; $this->httpClientService->expects($this->once())->method('newClient') @@ -289,17 +277,17 @@ class TrustedServersTest extends TestCase { throw new \Exception('simulated exception'); }); - $this->assertFalse($this->trustedServers->isOwnCloudServer($server)); + $this->assertFalse($this->trustedServers->isNextcloudServer($server)); } /** - * @dataProvider dataTestCheckOwnCloudVersion + * @dataProvider dataTestCheckNextcloudVersion */ - public function testCheckOwnCloudVersion($status) { - $this->assertTrue($this->invokePrivate($this->trustedServers, 'checkOwnCloudVersion', [$status])); + public function testCheckNextcloudVersion($status): void { + $this->assertTrue($this->invokePrivate($this->trustedServers, 'checkNextcloudVersion', [$status])); } - public function dataTestCheckOwnCloudVersion() { + public function dataTestCheckNextcloudVersion(): array { return [ ['{"version":"9.0.0"}'], ['{"version":"9.1.0"}'] @@ -307,16 +295,16 @@ class TrustedServersTest extends TestCase { } /** - * @dataProvider dataTestCheckOwnCloudVersionTooLow + * @dataProvider dataTestCheckNextcloudVersionTooLow */ - public function testCheckOwnCloudVersionTooLow($status) { + public function testCheckNextcloudVersionTooLow(string $status): void { $this->expectException(\OCP\HintException::class); $this->expectExceptionMessage('Remote server version is too low. 9.0 is required.'); - $this->invokePrivate($this->trustedServers, 'checkOwnCloudVersion', [$status]); + $this->invokePrivate($this->trustedServers, 'checkNextcloudVersion', [$status]); } - public function dataTestCheckOwnCloudVersionTooLow() { + public function dataTestCheckNextcloudVersionTooLow(): array { return [ ['{"version":"8.2.3"}'], ]; @@ -324,16 +312,14 @@ class TrustedServersTest extends TestCase { /** * @dataProvider dataTestUpdateProtocol - * @param string $url - * @param string $expected */ - public function testUpdateProtocol($url, $expected) { + public function testUpdateProtocol(string $url, string $expected): void { $this->assertSame($expected, $this->invokePrivate($this->trustedServers, 'updateProtocol', [$url]) ); } - public function dataTestUpdateProtocol() { + public function dataTestUpdateProtocol(): array { return [ ['http://owncloud.org', 'http://owncloud.org'], ['https://owncloud.org', 'https://owncloud.org'], -- 2.39.5