tests: code review of acceptance tests

tests/acceptance/features/apiAsyncUpload/delayPostprocessing.feature

@@ -6,7 +6,7 @@ Feature: delay post-processing of uploaded files
 
   Background:
     Given user "Alice" has been created with default attributes and without skeleton files
-    And async upload has been enabled with post processing delayed to "30" seconds
+    And async upload has been enabled with post-processing delayed to "30" seconds
 
   @issue-5326
   Scenario Outline: user sends GET request to the file while it's still being processed

tests/acceptance/features/apiNotification/spaceNotification.feature

@@ -285,7 +285,7 @@ Feature: Notification
     And user "Alice" has disabled a space "notification checking"
     When user "Brian" lists all notifications
     Then the HTTP status code should be "200"
-    And user "Brian" should have "2" notifications
+    And there should be "2" notifications
     And the JSON response should contain a notification message with the subject "Space disabled" and the message-details should match
       """
       {
@@ -462,4 +462,4 @@ Feature: Notification
     And user "Alice" has disabled a space "notification checking"
     When user "Brian" lists all notifications
     Then the HTTP status code should be "200"
-    And user "Brian" should have "2" notifications
+    And there should be "2" notifications

tests/acceptance/features/bootstrap/ArchiverContext.php

@@ -27,7 +27,7 @@
 use TestHelpers\HttpRequestHelper;
 use TestHelpers\SetupHelper;
 use PHPUnit\Framework\Assert;
-use \Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ResponseInterface;
 
 require_once 'bootstrap.php';
 

tests/acceptance/features/bootstrap/AuthContext.php

@@ -24,16 +24,13 @@
 use Behat\Gherkin\Node\TableNode;
 use Behat\Behat\Context\Context;
 use TestHelpers\SetupHelper;
-use \Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ResponseInterface;
 use TestHelpers\WebDavHelper;
 
 /**
  * Authentication functions
  */
 class AuthContext implements Context {
-	private string $clientToken;
-	private string $appToken;
-	private array $appTokens;
 	private FeatureContext $featureContext;
 
 	/**
@@ -548,12 +545,12 @@ public function userRequestsTheseEndpointsWithoutBodyUsingThePasswordOfUser(stri
 	 *
 	 * @param string $asUser
 	 * @param string $method
-	 * @param string $body
+	 * @param string|null $body
 	 * @param string $ofUser
 	 * @param TableNode $table
 	 *
 	 * @return void
-	 * @throws Exception
+	 * @throws JsonException
 	 */
 	public function userRequestsTheseEndpointsIncludingBodyUsingPasswordOfUser(string $asUser, string $method, ?string $body, string $ofUser, TableNode $table):void {
 		$asUser = $this->featureContext->getActualUsername($asUser);

tests/acceptance/features/bootstrap/ChecksumContext.php

@@ -398,7 +398,7 @@ public function theOcChecksumHeaderShouldNotBeThere():void {
 		Assert::assertFalse(
 			$isHeader,
 			"Expected no checksum header but got "
-			. $this->featureContext->getResponse()->getHeader('OC-Checksum')
+			. print_r($this->featureContext->getResponse()->getHeader('OC-Checksum'), true)
 		);
 	}
 

tests/acceptance/features/bootstrap/FeatureContext.php

@@ -30,7 +30,6 @@
 use Behat\Gherkin\Node\PyStringNode;
 use Behat\Gherkin\Node\TableNode;
 use Behat\Testwork\Hook\Scope\BeforeSuiteScope;
-use Behat\Testwork\Hook\Scope\AfterSuiteScope;
 use GuzzleHttp\Cookie\CookieJar;
 use Psr\Http\Message\ResponseInterface;
 use PHPUnit\Framework\Assert;
@@ -60,17 +59,17 @@ class FeatureContext extends BehatVariablesContext {
 	 * json schema validator keywords
 	 * See: https://json-schema.org/draft-06/draft-wright-json-schema-validation-01#rfc.section.6
 	 */
-	private array $jsonSchemaValidators = [];
+	private array $jsonSchemaValidators;
 
 	/**
 	 * Unix timestamp seconds
 	 */
 	private int $scenarioStartTime;
-	private string $adminUsername = '';
-	private string $adminPassword = '';
+	private string $adminUsername;
+	private string $adminPassword;
 	private string $adminDisplayName = '';
 	private string $adminEmailAddress = '';
-	private string $originalAdminPassword = '';
+	private string $originalAdminPassword;
 
 	/**
 	 * An array of values of replacement values of user attributes.
@@ -80,55 +79,55 @@ class FeatureContext extends BehatVariablesContext {
 	 * Key is the username, value is an array of user attributes
 	 */
 	private ?array $userReplacements = null;
-	private string $regularUserPassword = '';
-	private string $alt1UserPassword = '';
-	private string $alt2UserPassword = '';
-	private string $alt3UserPassword = '';
-	private string $alt4UserPassword = '';
+	private string $regularUserPassword;
+	private string $alt1UserPassword;
+	private string $alt2UserPassword;
+	private string $alt3UserPassword;
+	private string $alt4UserPassword;
 
 	/**
 	 * The password to use in tests that create a sub-admin user
 	 */
-	private string $subAdminPassword = '';
+	private string $subAdminPassword;
 
 	/**
 	 * The password to use in tests that create another admin user
 	 */
-	private string $alternateAdminPassword = '';
+	private string $alternateAdminPassword;
 
 	/**
 	 * The password to use in tests that create public link shares
 	 */
-	private string $publicLinkSharePassword = '';
-	private string $ocPath = '';
+	private string $publicLinkSharePassword;
+	private string $ocPath;
 
 	/**
 	 * Location of the root folder of ownCloud on the local server under test
 	 */
 	private ?string $localServerRoot = null;
 	private string $currentUser = '';
-	private string $currentServer = '';
+	private string $currentServer;
 
 	/**
 	 * The base URL of the current server under test,
 	 * without any terminating slash
 	 * e.g. http://localhost:8080
 	 */
-	private string $baseUrl = '';
+	private string $baseUrl;
 
 	/**
 	 * The base URL of the local server under test,
 	 * without any terminating slash
 	 * e.g. http://localhost:8080
 	 */
-	private string $localBaseUrl = '';
+	private string $localBaseUrl;
 
 	/**
 	 * The base URL of the remote (federated) server under test,
 	 * without any terminating slash
 	 * e.g. http://localhost:8180
 	 */
-	private string $remoteBaseUrl = '';
+	private string $remoteBaseUrl;
 
 	/**
 	 * The suite name, feature name and scenario line number.
@@ -149,7 +148,7 @@ class FeatureContext extends BehatVariablesContext {
 	/**
 	 * @var boolean true if TEST_SERVER_FED_URL is defined
 	 */
-	private bool $federatedServerExists = false;
+	private bool $federatedServerExists;
 	private int $ocsApiVersion = 1;
 	private ?ResponseInterface $response = null;
 	private string $responseUser = '';
@@ -1407,9 +1406,11 @@ public function userSendsHTTPMethodToUrl(string $user, string $verb, string $url
 	 * @param string $user
 	 * @param string $verb
 	 * @param string $url
-	 * @param string $headersTable
+	 * @param TableNode $headersTable
 	 *
 	 * @return void
+	 * @throws GuzzleException
+	 * @throws JsonException
 	 */
 	public function userSendsHTTPMethodToUrlWithHeaders(string $user, string $verb, string $url, TableNode $headersTable): void {
 		$this->verifyTableNodeColumns(
@@ -3599,18 +3600,6 @@ public function clearSpaceId(): void {
 		WebDavHelper::$SPACE_ID_FROM_OCIS = '';
 	}
 
-	/**
-	 * @BeforeSuite
-	 *
-	 * @param BeforeSuiteScope $scope
-	 *
-	 * @return void
-	 * @throws Exception
-	 */
-	public static function useBigFileIDs(BeforeSuiteScope $scope): void {
-		return;
-	}
-
 	/**
 	 * Verify that the tableNode contains expected headers
 	 *

tests/acceptance/features/bootstrap/FilesVersionsContext.php

@@ -462,7 +462,7 @@ public function userGetMetaInfo(string $user, string $fileOrFileId, string $path
 	}
 
 	/**
-	 * returns the result parsed into an SimpleXMLElement
+	 * returns the result parsed into a SimpleXMLElement
 	 * with a registered namespace with 'd' as prefix and 'DAV:' as namespace
 	 *
 	 * @param string $user

tests/acceptance/features/bootstrap/GraphContext.php

@@ -140,7 +140,7 @@ public function theUserChangesTheDisplayNameOfUserToUsingTheGraphApi(string $byU
 	public function theUserChangesTheUserNameOfUserToUsingTheGraphApi(string $byUser, string $user, string $userName): void {
 		$response = $this->editUserUsingTheGraphApi($byUser, $user, $userName);
 		$this->featureContext->setResponse($response);
-		// need add user to list to delete him after test
+		// need to add user to list to delete him after test
 		if (!empty($userName)) {
 			$this->featureContext->addUserToCreatedUsersList($userName, $this->featureContext->getUserPassword($user));
 		}
@@ -324,7 +324,7 @@ public function adminDeletesUserUsingTheGraphApi(string $user, ?string $byUser =
 	}
 
 	/**
-	 * remove user from group
+	 * remove user from a group
 	 *
 	 * @param string $group
 	 * @param string $user
@@ -675,7 +675,7 @@ public function getArrayOfUsersResponded(ResponseInterface $response): array {
 	}
 
 	/**
-	 * returns a list of members in group
+	 * returns a list of members in a group
 	 *
 	 * @param string $group
 	 *
@@ -761,16 +761,14 @@ public function addUserToGroup(string $group, string $user, ?string $byUser = nu
 	 *
 	 * @param string $user
 	 * @param string $group
-	 * @param bool $checkResult
 	 *
 	 * @return void
 	 * @throws Exception
 	 * @throws GuzzleException
 	 */
 	public function adminHasAddedUserToGroupUsingTheGraphApi(
 		string $user,
-		string $group,
-		bool $checkResult = true
+		string $group
 	): void {
 		$response = $this->addUserToGroup($group, $user);
 		$this->featureContext->theHTTPStatusCodeShouldBe(204, '', $response);
@@ -1887,7 +1885,7 @@ public function userGetsDetailsOfTheGroupUsingTheGraphApi(string $user, string $
 	 *
 	 * @return void
 	 */
-	public function userSearchesForGroupUsingGraphApi($user, $searchTerm):void {
+	public function userSearchesForGroupUsingGraphApi(string $user, string $searchTerm):void {
 		$credentials = $this->getAdminOrUserCredentials($user);
 		$this->featureContext->setResponse(
 			GraphHelper::searchGroup(
@@ -2242,7 +2240,7 @@ public function userGeneratesGDPRReportOfOwnDataToPath(string $user, string $pat
 	 * @param PyStringNode|null $schemaString
 	 *
 	 * @return void
-	 * @throws GuzzleException
+	 * @throws Exception
 	 *
 	 */
 	public function downloadedJsonContentShouldContainEventTypeInItemAndShouldMatch(string $eventType, ?string $spaceType=null, PyStringNode $schemaString=null): void {
@@ -2324,7 +2322,7 @@ public function userTriesToExportGdprReportOfAnotherUserUsingGraphApi(string $us
 	 * @return ResponseInterface
 	 * @throws GuzzleException
 	 */
-	public function getAssignedRole(string $user) {
+	public function getAssignedRole(string $user): ResponseInterface {
 		$userId = $this->featureContext->getAttributeOfCreatedUser($user, 'id') ?? $this->featureContext->getUserIdByUserName($user);
 		return (
 			GraphHelper::getAssignedRole(

tests/acceptance/features/bootstrap/NotificationContext.php

@@ -247,15 +247,14 @@ public function userShouldNotHaveAnyNotification($user): void {
 	}
 
 	/**
-	 * @Then /^user "([^"]*)" should have "([^"]*)" notifications$/
+	 * @Then /^there should be "([^"]*)" notifications$/
 	 *
-	 * @param string $user
 	 * @param int $numberOfNotification
 	 *
 	 * @return void
 	 * @throws Exception
 	 */
-	public function userShouldHaveNotifications(string $user, int $numberOfNotification): void {
+	public function userShouldHaveNotifications(int $numberOfNotification): void {
 		if (!isset($this->featureContext->getJsonDecodedResponseBodyContent()->ocs->data)) {
 			throw new Exception("Notification is empty");
 		}
@@ -264,7 +263,7 @@ public function userShouldHaveNotifications(string $user, int $numberOfNotificat
 		Assert::assertEquals(
 			$numberOfNotification,
 			$actualNumber,
-			"Expected number of notification was '$numberOfNotification', but got '$actualNumber'"
+			"Expected number of notifications was '$numberOfNotification', but got '$actualNumber'"
 		);
 	}
 
@@ -351,7 +350,7 @@ public function filterResponseByNotificationSubjectAndResource(string $subject,
 			}
 		} else {
 			$responseBodyArray[] = $this->featureContext->getJsonDecodedResponseBodyContent($response);
-			Assert::fail("Response should contain notification but found: $responseBodyArray");
+			Assert::fail("Response should contain notification but found: " . print_r($responseBodyArray, true));
 		}
 		return $responseBodyArray;
 	}
@@ -368,8 +367,8 @@ public function filterResponseByNotificationSubjectAndResource(string $subject,
 	 */
 	public function userShouldGetANotificationWithMessage(string $user, string $subject, TableNode $table):void {
 		$count = 0;
-		// sometimes the test might try to get notification before the notification is created by the server
-		// in order to prevent test from failing because of that try to list the notifications again
+		// Sometimes the test might try to get the notifications before the server has created the notification.
+		// To prevent the test from failing because of that, try to list the notifications again
 		do {
 			if ($count > 0) {
 				\sleep(1);
@@ -418,9 +417,9 @@ public function userShouldGetNotificationForResourceWithMessage(string $user, st
 			$this->userDeletesNotificationOfResourceAndSubject($user, $resource, $subject);
 			$this->featureContext->theHTTPStatusCodeShouldBe(200);
 		} elseif (\count($notification) === 0) {
-			throw new \Exception("Response doesn't contain any notification with resource '$resource' and subject '$subject'.\n$notification");
+			throw new \Exception("Response doesn't contain any notification with resource '$resource' and subject '$subject'.\n" . print_r($notification, true));
 		} else {
-			throw new \Exception("Response contains more than one notification with resource '$resource' and subject '$subject'.\n$notification");
+			throw new \Exception("Response contains more than one notification with resource '$resource' and subject '$subject'.\n" . print_r($notification, true));
 		}
 	}
 
@@ -472,9 +471,7 @@ public function userShouldHaveReceivedTheFollowingEmailFromUserAboutTheShareOfPr
 						[$this->spacesContext, "getSpaceIdByName"],
 					"parameter" => [$sender, $spaceName]
 				],
-			],
-			null,
-			null
+			]
 		);
 		$this->assertEmailContains($user, $expectedEmailBodyContent);
 	}
@@ -527,11 +524,11 @@ public function assertEmailContains(string $user, string $expectedEmailBodyConte
 	public function clearInbucketMessages():void {
 		try {
 			if (!empty($this->featureContext->emailRecipients)) {
-				foreach ($this->featureContext->emailRecipients as $emailRecipent) {
+				foreach ($this->featureContext->emailRecipients as $emailRecipient) {
 					EmailHelper::deleteAllEmailsForAMailbox(
 						EmailHelper::getLocalEmailUrl(),
 						$this->featureContext->getStepLineRef(),
-						$emailRecipent
+						$emailRecipient
 					);
 				}
 			}
@@ -568,8 +565,7 @@ public function userCreatesDeprovisioningNotification(
 			'POST',
 			$this->globalNotificationEndpointPath,
 			$this->featureContext->getStepLineRef(),
-			json_encode($payload),
-			2
+			json_encode($payload)
 		);
 	}
 
@@ -635,8 +631,7 @@ public function userDeletesDeprovisioningNotification(?string $user = null):void
 			'DELETE',
 			$this->globalNotificationEndpointPath,
 			$this->featureContext->getStepLineRef(),
-			json_encode($payload),
-			2
+			json_encode($payload)
 		);
 		$this->featureContext->setResponse($response);
 	}