MAGETWO-46837: Extracting metric tracking from test object and adding wait and comment to ignored action types

Author: pdohogne-magento

src/Magento/FunctionalTestingFramework/Extension/PageReadinessExtension.php

@@ -12,7 +12,7 @@
 use Codeception\Exception\ModuleRequireException;
 use Codeception\Extension;
 use Codeception\Module\WebDriver;
-use Codeception\TestInterface;
+use Codeception\Step;
 use Facebook\WebDriver\Exception\UnexpectedAlertOpenException;
 use Magento\FunctionalTestingFramework\Extension\ReadinessMetrics\AbstractMetricCheck;
 use Facebook\WebDriver\Exception\TimeOutException;
@@ -32,8 +32,18 @@ class PageReadinessExtension extends Extension
      */
     public static $events = [
         Events::TEST_BEFORE => 'beforeTest',
-        Events::STEP_BEFORE => 'beforeStep',
-        Events::STEP_AFTER => 'afterStep'
+        Events::STEP_BEFORE => 'beforeStep'
+    ];
+
+    /**
+     * List of action types that should bypass metric checks
+     * shouldSkipCheck() also checks for the 'Comment' step type, which doesn't follow the $step->getAction() pattern
+     *
+     * @var array
+     */
+    private static $ignoredActions = [
+        'saveScreenshot',
+        'wait'
     ];
 
     /**
@@ -56,11 +66,18 @@ class PageReadinessExtension extends Extension
     private $readinessMetrics;
 
     /**
-     * Active test object
+     * The name of the active test
      *
-     * @var TestInterface
+     * @var string
      */
-    private $test;
+    private $testName;
+
+    /**
+     * The current URI of the active page
+     *
+     * @var string
+     */
+    private $uri;
 
     /**
      * Initialize local vars
@@ -93,17 +110,18 @@ public function getDriver()
      */
     public function beforeTest(TestEvent $e)
     {
-        $this->test = $e->getTest();
-
         if (isset($this->config['resetFailureThreshold'])) {
             $failThreshold = intval($this->config['resetFailureThreshold']);
         } else {
             $failThreshold = 3;
         }
 
+        $this->testName = $e->getTest()->getMetadata()->getName();
+        $this->uri = null;
+
         $metrics = [];
         foreach ($this->config['readinessMetrics'] as $metricClass) {
-            $metrics[] = new $metricClass($this, $this->test, $failThreshold);
+            $metrics[] = new $metricClass($this, $failThreshold);
         }
 
         $this->readinessMetrics = $metrics;
@@ -119,16 +137,13 @@ public function beforeTest(TestEvent $e)
     public function beforeStep(StepEvent $e)
     {
         $step = $e->getStep();
-        if ($step->getAction() == 'saveScreenshot') {
+        if ($this->shouldSkipCheck($step)) {
             return;
         }
 
-        try {
-            $this->test->getMetadata()->setCurrent(['uri', $this->getDriver()->_getCurrentUri()]);
-        } catch (\Exception $exception) {
-            $this->logDebug('Could not retrieve current URI', ['action' => $e->getStep()->getAction()]);
-        }
+        $this->checkForNewPage($step);
 
+        // todo: Implement step parameter to override global timeout configuration
         if (isset($this->config['timeout'])) {
             $timeout = intval($this->config['timeout']);
         } else {
@@ -159,46 +174,75 @@ function () use ($metrics) {
 
         /** @var AbstractMetricCheck $metric */
         foreach ($metrics as $metric) {
-            $metric->finalize($step);
+            $metric->finalizeForStep($step);
         }
     }
 
     /**
-     * Checks to see if the step changed the uri and resets failure tracking if so
+     * Check if the URI has changed and reset metric tracking if so
      *
-     * @param StepEvent $e
+     * @param Step $step
      * @return void
      */
-    public function afterStep(StepEvent $e)
+    private function checkForNewPage($step)
     {
-        $step = $e->getStep();
-        if ($step->getAction() == 'saveScreenshot') {
-            return;
-        }
-
         try {
             $currentUri = $this->getDriver()->_getCurrentUri();
+
+            if ($this->uri !== $currentUri) {
+                $this->logDebug(
+                    'Page URI changed; resetting readiness metric failure tracking',
+                    [
+                        'step' => $step->__toString(),
+                        'newUri' => $currentUri
+                    ]
+                );
+
+                /** @var AbstractMetricCheck $metric */
+                foreach ($this->readinessMetrics as $metric) {
+                    $metric->resetTracker();
+                }
+
+                $this->uri = $currentUri;
+            }
         } catch (\Exception $e) {
-            // $this->debugLog('Could not retrieve current URI', ['action' => $step()->getAction()]);
-            return;
+            $this->logDebug('Could not retrieve current URI', ['step' => $step->__toString()]);
         }
+    }
 
-        $previousUri = $this->test->getMetadata()->getCurrent('uri');
+    /**
+     * Gets the active page URI from the start of the most recent step
+     *
+     * @return string
+     */
+    public function getUri()
+    {
+        return $this->uri;
+    }
 
-        if ($previousUri !== $currentUri) {
-            $this->logDebug(
-                'Page URI changed; resetting readiness metric failure tracking',
-                [
-                    'action' => $step->getAction(),
-                    'newUri' => $currentUri
-                ]
-            );
+    /**
+     * Gets the name of the active test
+     *
+     * @return string
+     */
+    public function getTestName()
+    {
+        return $this->testName;
+    }
 
-            /** @var AbstractMetricCheck $metric */
-            foreach ($this->readinessMetrics as $metric) {
-                $metric->setTracker();
-            }
+    /**
+     * Should the given step bypass the readiness checks
+     * todo: Implement step parameter to bypass specific metrics (or all) instead of basing on action type
+     *
+     * @param Step $step
+     * @return boolean
+     */
+    private function shouldSkipCheck($step)
+    {
+        if ($step instanceof Step\Comment || in_array($step->getAction(), PageReadinessExtension::$ignoredActions)) {
+            return true;
         }
+        return false;
     }
 
     /**
@@ -211,10 +255,9 @@ public function afterStep(StepEvent $e)
     private function logDebug($message, $context = [])
     {
         if ($this->verbose) {
-            $testMeta = $this->test->getMetadata();
             $logContext = [
-                'test' => $testMeta->getName(),
-                'uri' => $testMeta->getCurrent('uri')
+                'test' => $this->testName,
+                'uri' => $this->uri
             ];
             foreach ($this->readinessMetrics as $metric) {
                 $logContext[$metric->getName()] = $metric->getStoredValue();

src/Magento/FunctionalTestingFramework/Extension/ReadinessMetrics/AbstractMetricCheck.php

@@ -9,7 +9,6 @@
 use Codeception\Exception\ModuleRequireException;
 use Codeception\Module\WebDriver;
 use Codeception\Step;
-use Codeception\TestInterface;
 use Facebook\WebDriver\Exception\UnexpectedAlertOpenException;
 use Magento\FunctionalTestingFramework\Extension\PageReadinessExtension;
 use Magento\FunctionalTestingFramework\Util\Logger\LoggingUtil;
@@ -29,18 +28,26 @@ abstract class AbstractMetricCheck
     protected $extension;
 
     /**
-     * The active test object
+     * Current state of the value the metric tracks
      *
-     * @var TestInterface
+     * @var mixed;
      */
-    protected $test;
+    protected $currentValue;
 
     /**
-     * Current state of the value the metric tracks
+     * Most recent saved state of the value the metric tracks
+     * Updated when the metric passes or is finalized
      *
      * @var mixed;
      */
-    protected $currentValue;
+    protected $storedValue;
+
+    /**
+     * Current count of sequential identical failures
+     *
+     * @var integer;
+     */
+    protected $failCount;
 
     /**
      * Number of sequential identical failures before force-resetting the metric
@@ -63,14 +70,12 @@ abstract class AbstractMetricCheck
      * Constructor, called from the beforeTest event
      *
      * @param PageReadinessExtension $extension
-     * @param TestInterface          $test
      * @param integer                $resetFailureThreshold
      * @throws \Exception
      */
-    public function __construct($extension, $test, $resetFailureThreshold)
+    public function __construct($extension, $resetFailureThreshold)
     {
         $this->extension = $extension;
-        $this->test = $test;
         $this->logger = LoggingUtil::getInstance()->getLogger(get_class($this));
         $this->verbose = MftfApplicationConfig::getConfig()->verboseEnabled();
 
@@ -83,7 +88,7 @@ public function __construct($extension, $test, $resetFailureThreshold)
             $this->resetFailureThreshold = -1;
         }
 
-        $this->setTracker();
+        $this->resetTracker();
     }
 
     /**
@@ -141,7 +146,7 @@ public function getName()
     public function runCheck()
     {
         if ($this->doesMetricPass($this->getCurrentValue(true))) {
-            $this->setTracker($this->getCurrentValue());
+            $this->setTracker($this->getCurrentValue(), 0);
             return true;
         }
 
@@ -157,35 +162,35 @@ public function runCheck()
      * @param Step $step
      * @return void
      */
-    public function finalize($step)
+    public function finalizeForStep($step)
     {
         try {
             $currentValue = $this->getCurrentValue();
         } catch (UnexpectedAlertOpenException $exception) {
             $this->debugLog(
                 'An alert is open, bypassing javascript-based metric check',
-                ['action' => $step->getAction()]
+                ['step' => $step->__toString()]
             );
             return;
         }
 
         if ($this->doesMetricPass($currentValue)) {
-            $this->setTracker($currentValue);
+            $this->setTracker($currentValue, 0);
         } else {
             // If failure happened on the same value as before, increment the fail count, otherwise set at 1
-            if ($currentValue !== $this->getStoredValue()) {
+            if (!isset($this->storedValue) || $currentValue !== $this->getStoredValue()) {
                 $failCount = 1;
             } else {
                 $failCount = $this->getFailureCount() + 1;
             }
             $this->setTracker($currentValue, $failCount);
 
-            $this->errorLog('Failed readiness check', ['action' => $step->getAction()]);
+            $this->errorLog('Failed readiness check', ['step' => $step->__toString()]);
 
             if ($this->resetFailureThreshold >= 0 && $failCount >= $this->resetFailureThreshold) {
                 $this->debugLog(
                     'Too many failures, assuming metric is stuck and resetting state',
-                    ['action' => $step->getAction()]
+                    ['step' => $step->__toString()]
                 );
                 $this->resetMetric();
             }
@@ -214,7 +219,7 @@ protected function getDriver()
      */
     protected function executeJs($script, $arguments = [])
     {
-        return $this->getDriver()->executeJS($script, $arguments);
+        return $this->extension->getDriver()->executeJS($script, $arguments);
     }
 
     /**
@@ -243,7 +248,7 @@ private function getCurrentValue($refresh = false)
      */
     public function getStoredValue()
     {
-        return $this->test->getMetadata()->getCurrent($this->getName());
+        return $this->storedValue;
     }
 
     /**
@@ -254,7 +259,7 @@ public function getStoredValue()
      */
     public function getFailureCount()
     {
-        return $this->test->getMetadata()->getCurrent($this->getName() . '.failCount');
+        return $this->failCount;
     }
 
     /**
@@ -266,7 +271,7 @@ public function getFailureCount()
     private function resetMetric()
     {
         $this->clearFailureOnPage();
-        $this->setTracker();
+        $this->resetTracker();
     }
 
     /**
@@ -276,13 +281,18 @@ private function resetMetric()
      * @param integer $failCount
      * @return void
      */
-    public function setTracker($value = null, $failCount = 0)
+    public function setTracker($value, $failCount)
+    {
+        unset($this->currentValue);
+        $this->storedValue = $value;
+        $this->failCount = $failCount;
+    }
+
+    public function resetTracker()
     {
-        $this->test->getMetadata()->setCurrent([
-            $this->getName() => $value,
-            $this->getName() . '.failCount' => $failCount
-        ]);
         unset($this->currentValue);
+        unset($this->storedValue);
+        $this->failCount = 0;
     }
 
     /**
@@ -334,10 +344,9 @@ protected function debugLog($message, $context = [])
      */
     private function getLogContext()
     {
-        $testMeta = $this->test->getMetadata();
         return [
-            'test' => $testMeta->getName(),
-            'uri' => $testMeta->getCurrent('uri'),
+            'test' => $this->extension->getTestName(),
+            'uri' => $this->extension->getUri(),
             $this->getName() => $this->getStoredValue(),
             $this->getName() . '.failCount' => $this->getFailureCount()
         ];