Merge pull request #4682 from nextcloud/try-to-start-browser-sessions-again-when-they-fail-in-acceptance-tests

Try to start browser sessions again when they fail in acceptance tests

5 files changed, 99 insertions, 14 deletions

diff --git a/.drone.yml b/.drone.yml
index f5776cd019e..e5fda39f4ee 100644
--- a/.drone.yml
+++ b/.drone.yml
@@ -467,21 +467,21 @@ pipeline:
   acceptance-access-levels:
       image: nextcloudci/php7.0:php7.0-7
       commands:
-        - tests/acceptance/run-local.sh allow-git-repository-modifications features/access-levels.feature
+        - tests/acceptance/run-local.sh --timeout-multiplier 10 allow-git-repository-modifications features/access-levels.feature
       when:
         matrix:
           TESTS-ACCEPTANCE: access-levels
   acceptance-app-files:
       image: nextcloudci/php7.0:php7.0-7
       commands:
-        - tests/acceptance/run-local.sh allow-git-repository-modifications features/app-files.feature
+        - tests/acceptance/run-local.sh --timeout-multiplier 10 allow-git-repository-modifications features/app-files.feature
       when:
         matrix:
           TESTS-ACCEPTANCE: app-files
   acceptance-login:
       image: nextcloudci/php7.0:php7.0-7
       commands:
-        - tests/acceptance/run-local.sh allow-git-repository-modifications features/login.feature
+        - tests/acceptance/run-local.sh --timeout-multiplier 10 allow-git-repository-modifications features/login.feature
       when:
         matrix:
           TESTS-ACCEPTANCE: login
diff --git a/tests/acceptance/features/core/Actor.php b/tests/acceptance/features/core/Actor.php
index 0c23b5f7a40..3a57b7e6054 100644
--- a/tests/acceptance/features/core/Actor.php
+++ b/tests/acceptance/features/core/Actor.php
@@ -165,6 +165,18 @@ class Actor {
 	public function find($elementLocator, $timeout = 0, $timeoutStep = 0.5) {
 		$timeout = $timeout * $this->findTimeoutMultiplier;
 
+		return $this->findInternal($elementLocator, $timeout, $timeoutStep);
+	}
+
+	/**
+	 * Finds an element in the Mink Session of this Actor.
+	 *
+	 * The timeout is not affected by the multiplier set using
+	 * setFindTimeoutMultiplier().
+	 *
+	 * @see find($elementLocator, $timeout, $timeoutStep)
+	 */
+	private function findInternal($elementLocator, $timeout, $timeoutStep) {
 		$element = null;
 		$selector = $elementLocator->getSelector();
 		$locator = $elementLocator->getLocator();
@@ -211,7 +223,7 @@ class Actor {
 		$ancestorElement = $elementLocator->getAncestor();
 		if ($ancestorElement instanceof Locator) {
 			try {
-				$ancestorElement = $this->find($ancestorElement, $timeout, $timeoutStep);
+				$ancestorElement = $this->findInternal($ancestorElement, $timeout, $timeoutStep);
 			} catch (NoSuchElementException $exception) {
 				// Little hack to show the stack of ancestor elements that could
 				// not be found, as Behat only shows the message of the last
diff --git a/tests/acceptance/features/core/ActorContext.php b/tests/acceptance/features/core/ActorContext.php
index 86fe3832f66..d6fb63694ec 100644
--- a/tests/acceptance/features/core/ActorContext.php
+++ b/tests/acceptance/features/core/ActorContext.php
@@ -39,8 +39,9 @@ use Behat\MinkExtension\Context\RawMinkContext;
  * propagates its inherited "base_url" Mink parameter to the Actors as needed.
  *
  * By default no multiplier for the find timeout is set in the Actors. However,
- * it can be customized using the "actorFindTimeoutMultiplier" parameter of the
- * ActorContext in "behat.yml".
+ * it can be customized using the "actorTimeoutMultiplier" parameter of the
+ * ActorContext in "behat.yml". This parameter also affects the overall timeout
+ * to start a session for an Actor before giving up.
  *
  * Every actor used in the scenarios must have a corresponding Mink session
  * declared in "behat.yml" with the same name as the actor. All used sessions
@@ -66,16 +67,16 @@ class ActorContext extends RawMinkContext {
 	/**
 	 * @var float
 	 */
-	private $actorFindTimeoutMultiplier;
+	private $actorTimeoutMultiplier;
 
 	/**
 	 * Creates a new ActorContext.
 	 *
-	 * @param float $actorFindTimeoutMultiplier the find timeout multiplier to
-	 *        set in the Actors.
+	 * @param float $actorTimeoutMultiplier the timeout multiplier for Actor
+	 *        related timeouts.
 	 */
-	public function __construct($actorFindTimeoutMultiplier = 1) {
-		$this->actorFindTimeoutMultiplier = $actorFindTimeoutMultiplier;
+	public function __construct($actorTimeoutMultiplier = 1) {
+		$this->actorTimeoutMultiplier = $actorTimeoutMultiplier;
 	}
 
 	/**
@@ -98,6 +99,31 @@ class ActorContext extends RawMinkContext {
 	}
 
 	/**
+	 * Returns the session with the given name.
+	 *
+	 * If the session is not started it is started before returning it; if the
+	 * session fails to start (typically due to a timeout connecting with the
+	 * web browser) it will be tried again up to $actorTimeoutMultiplier times
+	 * in total (rounded up to the next integer) before giving up.
+	 *
+	 * @param string|null $sname the name of the session to get, or null for the
+	 *        default session.
+	 * @return \Behat\Mink\Session the session.
+	 */
+	public function getSession($name = null) {
+		for ($i = 0; $i < ($this->actorTimeoutMultiplier - 1); $i++) {
+			try {
+				return parent::getSession($name);
+			} catch (\Behat\Mink\Exception\DriverException $exception) {
+				echo "Exception when getting " . ($name == null? "default session": "session '$name'") . ": " . $exception->getMessage() . "\n";
+				echo "Trying again\n";
+			}
+		}
+
+		return parent::getSession($name);
+	}
+
+	/**
 	 * @BeforeScenario
 	 *
 	 * Initializes the Actors for the new Scenario with the default Actor.
@@ -110,7 +136,7 @@ class ActorContext extends RawMinkContext {
 		$this->sharedNotebook = array();
 
 		$this->actors["default"] = new Actor($this->getSession(), $this->getMinkParameter("base_url"), $this->sharedNotebook);
-		$this->actors["default"]->setFindTimeoutMultiplier($this->actorFindTimeoutMultiplier);
+		$this->actors["default"]->setFindTimeoutMultiplier($this->actorTimeoutMultiplier);
 
 		$this->currentActor = $this->actors["default"];
 	}
@@ -134,7 +160,7 @@ class ActorContext extends RawMinkContext {
 	public function iActAs($actorName) {
 		if (!array_key_exists($actorName, $this->actors)) {
 			$this->actors[$actorName] = new Actor($this->getSession($actorName), $this->getMinkParameter("base_url"), $this->sharedNotebook);
-			$this->actors[$actorName]->setFindTimeoutMultiplier($this->actorFindTimeoutMultiplier);
+			$this->actors[$actorName]->setFindTimeoutMultiplier($this->actorTimeoutMultiplier);
 		}
 
 		$this->currentActor = $this->actors[$actorName];
diff --git a/tests/acceptance/run-local.sh b/tests/acceptance/run-local.sh
index ee7a4e6455c..93c11e810f8 100755
--- a/tests/acceptance/run-local.sh
+++ b/tests/acceptance/run-local.sh
@@ -39,6 +39,21 @@ set -o errexit
 # Behat through Composer or running Behat) expect that.
 cd "$(dirname $0)"
 
+# "--timeout-multiplier N" option can be provided before any other parameter to
+# set the timeout multiplier to be used in ActorContext.
+TIMEOUT_MULTIPLIER=""
+if [ "$1" = "--timeout-multiplier" ]; then
+	if [[ ! "$2" =~ ^[0-9]+$ ]]; then
+		echo "--timeout-multiplier must be followed by a positive integer"
+
+		exit 1
+	fi
+
+	TIMEOUT_MULTIPLIER=$2
+
+	shift 2
+fi
+
 # Safety parameter to prevent executing this script by mistake and messing with
 # the Git repository.
 if [ "$1" != "allow-git-repository-modifications" ]; then
@@ -49,6 +64,22 @@ fi
 
 SCENARIO_TO_RUN=$2
 
+if [ "$TIMEOUT_MULTIPLIER" != "" ]; then
+	# Although Behat documentation states that using the BEHAT_PARAMS
+	# environment variable "You can set any value for any option that is
+	# available in a behat.yml file" this is currently not true for the
+	# constructor parameters of contexts (see
+	# https://github.com/Behat/Behat/issues/983). Thus, the default "behat.yml"
+	# configuration file has to be adjusted to provide the appropriate
+	# parameters for ActorContext.
+	ORIGINAL="\
+        - ActorContext"
+	REPLACEMENT="\
+        - ActorContext:\n\
+            actorTimeoutMultiplier: $TIMEOUT_MULTIPLIER"
+	sed --in-place "s/$ORIGINAL/$REPLACEMENT/" config/behat.yml
+fi
+
 composer install
 
 cd ../../
diff --git a/tests/acceptance/run.sh b/tests/acceptance/run.sh
index 42a718d46e2..1b68f8655ae 100755
--- a/tests/acceptance/run.sh
+++ b/tests/acceptance/run.sh
@@ -197,6 +197,22 @@ trap cleanUp EXIT
 # the Git working directory to the container) expect that.
 cd "$(dirname $0)"
 
+# "--timeout-multiplier N" option can be provided before the specific scenario
+# to run, if any, to set the timeout multiplier to be used in the acceptance
+# tests.
+TIMEOUT_MULTIPLIER_OPTION=""
+if [ "$1" = "--timeout-multiplier" ]; then
+	if [[ ! "$2" =~ ^[0-9]+$ ]]; then
+		echo "--timeout-multiplier must be followed by a positive integer"
+
+		exit 1
+	fi
+
+	TIMEOUT_MULTIPLIER_OPTION="--timeout-multiplier $2"
+
+	shift 2
+fi
+
 # If no parameter is provided to this script all the acceptance tests are run.
 SCENARIO_TO_RUN=$1
 
@@ -206,4 +222,4 @@ prepareSelenium
 prepareDocker
 
 echo "Running tests"
-docker exec $NEXTCLOUD_LOCAL_CONTAINER bash -c "cd nextcloud && tests/acceptance/run-local.sh allow-git-repository-modifications $SCENARIO_TO_RUN"
+docker exec $NEXTCLOUD_LOCAL_CONTAINER bash -c "cd nextcloud && tests/acceptance/run-local.sh $TIMEOUT_MULTIPLIER_OPTION allow-git-repository-modifications $SCENARIO_TO_RUN"