From 475c71d1078979d756703c52c710d7c2a3d40729 Mon Sep 17 00:00:00 2001
From: Mathias Kresin <m@kresin.me>
Date: Mon, 10 Aug 2015 23:46:32 +0200
Subject: [PATCH] fix race conditions in tests

Wait till the counter has the desired value, instead of assuming that
the counter already has the expected value. This fixes the tests on
slow browsers.

Furthermore, the wait isn't needed any more, now that the counter
queries are race ondition proof.

The waitForIconMarkReadInvisible was the wrong wait function here, since
the whole article will be hidden instead of the read icon. This could
lead into race condition related errors if the article is hidden before
the waitForIconMarkRead() functions runs. The article variable that is
used to address the child read icon can refer to an (DOM) object which
doesn't exist any longer => StaleElementReferenceException.

The correct wait function in such a case would be waitForArticleInvisible().
---
 docs/tests.markdown                    |  2 +-
 tests/integration/minifluxTestCase.php | 62 +++++++++++++++++---------
 tests/integration/pageHistoryTest.php  |  2 -
 3 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/docs/tests.markdown b/docs/tests.markdown
index 6070f3c..cc758cd 100644
--- a/docs/tests.markdown
+++ b/docs/tests.markdown
@@ -62,7 +62,7 @@ The following `phpunit.xml` is used to run phpunit on a linux system with apache
         <const name="PHPUNIT_TESTSUITE_EXTENSION_SELENIUM_BROWSERS" value='
             [
               {
-                "browserName": "iexplore",
+                "browserName": "internet explorer",
                 "host": "windows.example.org",
                 "port": 4444,
                 "sessionStrategy": "shared"
diff --git a/tests/integration/minifluxTestCase.php b/tests/integration/minifluxTestCase.php
index 056603c..97ec13b 100644
--- a/tests/integration/minifluxTestCase.php
+++ b/tests/integration/minifluxTestCase.php
@@ -64,8 +64,8 @@ abstract class minifluxTestCase extends PHPUnit_Extensions_Selenium2TestCase
     protected function assertPostConditions()
     {
         // counter exists on every page
-        $this->assertEquals($this->expectedCounterPage, $this->getCounterPage(), 'page-counter differ from expectation');
-        $this->assertEquals($this->expectedCounterUnread, $this->getCounterUnread(), 'unread counter differ from expectation');
+        $this->assertTrue($this->waitForElementByIdText('page-counter', $this->expectedCounterPage), 'page-counter differ from expected');
+        $this->assertTrue($this->waitForElementByIdText('nav-counter', $this->expectedCounterUnread), 'unread counter differ from expectation');
 
         // url has not been changed (its likely that everything was done via javascript then)
         $this->assertEquals($this->expectedPageUrl, $this->url(), 'URL has been changed.');
@@ -157,25 +157,6 @@ abstract class minifluxTestCase extends PHPUnit_Extensions_Selenium2TestCase
         return static::$databaseTester;
     }
 
-    private function getCounterUnread()
-    {
-        $value = $this->element($this->using('id')->value('nav-counter'))->text();
-        return $value;
-    }
-
-    private function getCounterPage()
-    {
-        $value = NULL;
-
-        $elements = $this->elements($this->using('id')->value('page-counter'));
-
-        if (count($elements) === 1) {
-            $value = $elements[0]->text();
-        }
-
-        return $value;
-    }
-
     // public to be accessible within an closure
     public function isElementVisible($element)
     {
@@ -258,6 +239,45 @@ abstract class minifluxTestCase extends PHPUnit_Extensions_Selenium2TestCase
         return $value;
     }
 
+   private function waitForElementByIdText($id, $text)
+    {
+        // return false in case of timeout
+        try {
+            // Workaround for PHP < 5.4
+            $CI = $this;
+
+            $value = $this->waitUntil(function() use($CI, $id, $text) {
+                try {
+                    $elements = $this->elements($this->using('id')->value($id));
+
+                    if (count($elements) === 1 && $elements[0]->text() == $text
+                        || count($elements) === 0 && is_null($text)) {
+                        return TRUE;
+                    }
+                }
+                catch (PHPUnit_Extensions_Selenium2TestCase_WebDriverException $e) {
+                    $noSuchElement = ($e->getCode() === PHPUnit_Extensions_Selenium2TestCase_WebDriverException::NoSuchElement
+                                   || $e->getCode() === PHPUnit_Extensions_Selenium2TestCase_WebDriverException::StaleElementReference);
+
+                    // everything else than "No such Element" or
+                    // "Stale Element Reference" is unexpected
+                    if (! $noSuchElement) {
+                        throw $e;
+                    }
+                }
+            }, $this->waitTimeout);
+        }
+        catch(PHPUnit_Extensions_Selenium2TestCase_WebDriverException $e) {
+            if ($e->getCode() === PHPUnit_Extensions_Selenium2TestCase_WebDriverException::Timeout) {
+                return FALSE;
+            } else {
+                throw $e;
+            }
+        }
+
+        return $value;
+    }
+
     private function waitForElementAttributeHasValue($element, $attribute, $attributeValue, $invertMatch = FALSE)
     {
         // return false in case of timeout
diff --git a/tests/integration/pageHistoryTest.php b/tests/integration/pageHistoryTest.php
index 629ecd2..dc085b6 100644
--- a/tests/integration/pageHistoryTest.php
+++ b/tests/integration/pageHistoryTest.php
@@ -246,8 +246,6 @@ class pageHistoryTest extends minifluxTestCase
         $link = $this->getLinkReadStatusToogle($article);
         $link->click();
 
-        $this->waitForIconMarkReadInvisible($article);
-
         $this->expectedCounterPage = static::DEFAULT_COUNTER_PAGE + static::DEFAULT_COUNTER_UNREAD - 1;
         $this->expectedCounterUnread = 1;
         $this->expectedDataSet = $this->getDataSet('fixture_OneUnreadArticle');