Fix many issues with the tests

- Return type were not correct
- willReturn and with confusion

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

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'],