Merge pull request php-webdriver#1032 from OndraM/feature/reuse-session-capabilities

OndraM · web-flow · commit 9e4b70313ef7 · 2022-11-15T14:51:36.000+01:00
Improve session reusing, require capabilities to be set
diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml
@@ -162,6 +162,7 @@ jobs:
           CHROMEDRIVER_PATH: "${{ (matrix.browser == 'chrome' && !matrix.selenium-server) && '/usr/local/share/chrome_driver/chromedriver' || '' }}"
           GECKODRIVER_PATH: "${{ (matrix.browser == 'firefox' && !matrix.selenium-server) && '/usr/local/share/gecko_driver/geckodriver' || '' }}"
           DISABLE_W3C_PROTOCOL: "${{ matrix.w3c && '0' || '1' }}"
+          SELENIUM_SERVER: "${{ matrix.selenium-server && '1' || '0' }}"
         run: |
           if [ "$BROWSER_NAME" = "chrome" ]; then EXCLUDE_GROUP+="exclude-chrome,"; fi
           if [ "$BROWSER_NAME" = "firefox" ]; then EXCLUDE_GROUP+="exclude-firefox,"; fi
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,9 +7,10 @@ This project versioning adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 - Require PHP ^7.3.
+- Capabilities must be either explicitly provided or retrievable from Selenium Grid when resuing session with `createBySessionID()`.
 - Throw `UnexpectedResponseException` instead of `UnknownErrorException` in `findElement()` and `findElements()` methods.
 - Throw custom php-webdriver exceptions instead of native PHP SPL exceptions.
-- Do not mix internal non-W3C WebDriver exceptions, separate them into own namespace.
+- Do not mix internal non-W3C WebDriver exceptions, separate them into own namespaces.
 
 ## 1.13.0 - 2022-10-03
 ### Added
diff --git a/lib/Exception/Internal/UnexpectedResponseException.php b/lib/Exception/Internal/UnexpectedResponseException.php
@@ -26,4 +26,16 @@ public static function forJsonDecodingError(int $jsonLastError, string $rawResul
             )
         );
     }
+
+    public static function forCapabilitiesRetrievalError(\Exception $previousException): self
+    {
+        return new self(
+            sprintf(
+                'Existing Capabilities were not provided, and they also cannot be read from Selenium Grid'
+                . ' (error: "%s"). You are probably not using Selenium Grid, so to reuse the previous session,'
+                . ' Capabilities must be explicitly provided to createBySessionID() method.',
+                $previousException->getMessage()
+            )
+        );
+    }
 }
diff --git a/lib/Remote/RemoteWebDriver.php b/lib/Remote/RemoteWebDriver.php
@@ -56,22 +56,19 @@ class RemoteWebDriver implements WebDriver, JavaScriptExecutor, WebDriverHasInpu
     /**
      * @param HttpCommandExecutor $commandExecutor
      * @param string $sessionId
-     * @param WebDriverCapabilities|null $capabilities
+     * @param WebDriverCapabilities $capabilities
      * @param bool $isW3cCompliant false to use the legacy JsonWire protocol, true for the W3C WebDriver spec
      */
     protected function __construct(
         HttpCommandExecutor $commandExecutor,
         $sessionId,
-        WebDriverCapabilities $capabilities = null,
+        WebDriverCapabilities $capabilities,
         $isW3cCompliant = false
     ) {
         $this->executor = $commandExecutor;
         $this->sessionID = $sessionId;
         $this->isW3cCompliant = $isW3cCompliant;
-
-        if ($capabilities !== null) {
-            $this->capabilities = $capabilities;
-        }
+        $this->capabilities = $capabilities;
     }
 
     /**
@@ -139,14 +136,22 @@ public static function create(
     /**
      * [Experimental] Construct the RemoteWebDriver by an existing session.
      *
-     * This constructor can boost the performance a lot by reusing the same browser for the whole test suite.
-     * You cannot pass the desired capabilities because the session was created before.
+     * This constructor can boost the performance by reusing the same browser for the whole test suite. On the other
+     * hand, because the browser is not pristine, this may lead to flaky and dependent tests. So carefully
+     * consider the tradeoffs.
+     *
+     * To create the instance, we need to know Capabilities of the previously created session. You can either
+     * pass them in $existingCapabilities parameter, or we will attempt to receive them from the Selenium Grid server.
+     * However, if Capabilities were not provided and the attempt to get them was not successful,
+     * exception will be thrown.
      *
      * @param string $session_id The existing session id
      * @param string $selenium_server_url The url of the remote Selenium WebDriver server
      * @param int|null $connection_timeout_in_ms Set timeout for the connect phase to remote Selenium WebDriver server
      * @param int|null $request_timeout_in_ms Set the maximum time of a request to remote Selenium WebDriver server
      * @param bool $isW3cCompliant True to use W3C WebDriver (default), false to use the legacy JsonWire protocol
+     * @param WebDriverCapabilities|null $existingCapabilities Provide capabilities of the existing previously created
+     *  session. If not provided, we will attempt to read them, but this will only work when using Selenium Grid.
      * @return static
      */
     public static function createBySessionID(
@@ -157,6 +162,7 @@ public static function createBySessionID(
     ) {
         // BC layer to not break the method signature
         $isW3cCompliant = func_num_args() > 4 ? func_get_arg(4) : true;
+        $existingCapabilities = func_num_args() > 5 ? func_get_arg(5) : null;
 
         $executor = new HttpCommandExecutor($selenium_server_url, null, null);
         if ($connection_timeout_in_ms !== null) {
@@ -170,7 +176,12 @@ public static function createBySessionID(
             $executor->disableW3cCompliance();
         }
 
-        return new static($executor, $session_id, null, $isW3cCompliant);
+        // if  capabilities were not provided, attempt to read them from the Selenium Grid API
+        if ($existingCapabilities === null) {
+            $existingCapabilities = self::readExistingCapabilitiesFromSeleniumGrid($session_id, $executor);
+        }
+
+        return new static($executor, $session_id, $existingCapabilities, $isW3cCompliant);
     }
 
     /**
@@ -727,4 +738,26 @@ protected static function castToDesiredCapabilitiesObject($desired_capabilities
 
         return $desired_capabilities;
     }
+
+    protected static function readExistingCapabilitiesFromSeleniumGrid(
+        string $session_id,
+        HttpCommandExecutor $executor
+    ): DesiredCapabilities {
+        $getCapabilitiesCommand = new CustomWebDriverCommand($session_id, '/se/grid/session/:sessionId', 'GET', []);
+
+        try {
+            $capabilitiesResponse = $executor->execute($getCapabilitiesCommand);
+
+            $existingCapabilities = DesiredCapabilities::createFromW3cCapabilities(
+                $capabilitiesResponse->getValue()['capabilities']
+            );
+            if ($existingCapabilities === null) {
+                throw UnexpectedResponseException::forError('Empty capabilities received');
+            }
+        } catch (\Exception $e) {
+            throw UnexpectedResponseException::forCapabilitiesRetrievalError($e);
+        }
+
+        return $existingCapabilities;
+    }
 }
diff --git a/phpstan.neon b/phpstan.neon
@@ -23,5 +23,7 @@ parameters:
         # Parameter is intentionally not part of signature to not break BC
         - message: '#PHPDoc tag \@param references unknown parameter: \$isW3cCompliant#'
           path: 'lib/Remote/RemoteWebDriver.php'
+        - message: '#PHPDoc tag \@param references unknown parameter: \$existingCapabilities#'
+          path: 'lib/Remote/RemoteWebDriver.php'
 
     inferPrivatePropertyTypeFromConstructor: true
diff --git a/tests/functional/RemoteWebDriverCreateTest.php b/tests/functional/RemoteWebDriverCreateTest.php
@@ -2,12 +2,14 @@
 
 namespace Facebook\WebDriver;
 
+use Facebook\WebDriver\Exception\Internal\UnexpectedResponseException;
 use Facebook\WebDriver\Remote\DesiredCapabilities;
 use Facebook\WebDriver\Remote\HttpCommandExecutor;
 use Facebook\WebDriver\Remote\RemoteWebDriver;
 use Facebook\WebDriver\Remote\WebDriverBrowserType;
 
 /**
+ * @covers \Facebook\WebDriver\Exception\Internal\UnexpectedResponseException
  * @covers \Facebook\WebDriver\Remote\HttpCommandExecutor
  * @covers \Facebook\WebDriver\Remote\RemoteWebDriver
  */
@@ -111,12 +113,41 @@ public function testShouldCreateInstanceFromExistingSessionId()
         $originalDriver->get($this->getTestPageUrl(TestPage::INDEX));
         $this->compatAssertStringContainsString('/index.html', $originalDriver->getCurrentURL());
 
-        // Store session ID
+        // Store session attributes
         $sessionId = $originalDriver->getSessionID();
         $isW3cCompliant = $originalDriver->isW3cCompliant();
+        $originalCapabilities = $originalDriver->getCapabilities();
+
+        $capabilitiesForSessionReuse = $originalCapabilities;
+        if ($this->isSeleniumServerUsed()) {
+            // do not provide capabilities when selenium server is used, to test they are read from selenium server
+            $capabilitiesForSessionReuse = null;
+        }
 
         // Create new RemoteWebDriver instance based on the session ID
-        $this->driver = RemoteWebDriver::createBySessionID($sessionId, $this->serverUrl, null, null, $isW3cCompliant);
+        $this->driver = RemoteWebDriver::createBySessionID(
+            $sessionId,
+            $this->serverUrl,
+            null,
+            null,
+            $isW3cCompliant,
+            $capabilitiesForSessionReuse
+        );
+
+        // Capabilities should be retrieved and be set to the driver instance
+        $returnedCapabilities = $this->driver->getCapabilities();
+        $this->assertInstanceOf(WebDriverCapabilities::class, $returnedCapabilities);
+
+        $expectedBrowserName = $this->desiredCapabilities->getBrowserName();
+
+        if ($this->isSauceLabsBuild() && $expectedBrowserName === 'MicrosoftEdge') {
+            $expectedBrowserName = 'msedge'; // SauceLabs for some reason reports MicrosoftEdge as msedge
+        }
+        $this->assertEqualsIgnoringCase(
+            $expectedBrowserName,
+            $returnedCapabilities->getBrowserName()
+        );
+        $this->assertEqualsCanonicalizing($originalCapabilities, $this->driver->getCapabilities());
 
         // Check we reused the previous instance (window) and it has the same URL
         $this->compatAssertStringContainsString('/index.html', $this->driver->getCurrentURL());
@@ -125,6 +156,22 @@ public function testShouldCreateInstanceFromExistingSessionId()
         $this->assertNotEmpty($this->driver->findElement(WebDriverBy::id('id_test'))->getText());
     }
 
+    public function testShouldRequireCapabilitiesToBeSetToReuseExistingSession()
+    {
+        $this->expectException(UnexpectedResponseException::class);
+        $this->expectExceptionMessage(
+            'Existing Capabilities were not provided, and they also cannot be read from Selenium Grid'
+        );
+
+        // Do not provide capabilities, they also cannot be retrieved from the Selenium Grid
+        RemoteWebDriver::createBySessionID(
+            'sessionId',
+            'http://localhost:332', // nothing should be running there
+            null,
+            null
+        );
+    }
+
     protected function createWebDriver()
     {
     }
diff --git a/tests/functional/WebDriverTestCase.php b/tests/functional/WebDriverTestCase.php
@@ -110,6 +110,11 @@ public static function isW3cProtocolBuild()
         return getenv('DISABLE_W3C_PROTOCOL') !== '1';
     }
 
+    public static function isSeleniumServerUsed(): bool
+    {
+        return getenv('SELENIUM_SERVER') === '1';
+    }
+
     public static function skipForW3cProtocol($message = 'Not supported by W3C specification')
     {
         if (static::isW3cProtocolBuild()) {
diff --git a/tests/unit/Remote/RemoteWebDriverTest.php b/tests/unit/Remote/RemoteWebDriverTest.php
@@ -11,7 +11,7 @@
 use PHPUnit\Framework\TestCase;
 
 /**
- * Unit part of RemoteWebDriver tests. Ie. tests for behavior which do not interact with the real remote server.
+ * Unit part of RemoteWebDriver tests. I.e. tests for behavior which do not interact with the real remote server.
  *
  * @coversDefaultClass \Facebook\WebDriver\Remote\RemoteWebDriver
  */
@@ -22,7 +22,15 @@ class RemoteWebDriverTest extends TestCase
 
     protected function setUp(): void
     {
-        $this->driver = RemoteWebDriver::createBySessionID('session-id', 'http://foo.bar:4444');
+        // `createBySessionID()` is used because it is the simplest way to instantiate real RemoteWebDriver
+        $this->driver = RemoteWebDriver::createBySessionID(
+            'session-id',
+            'http://foo.bar:4444',
+            null,
+            null,
+            true,
+            new DesiredCapabilities([])
+        );
     }
 
     /**

Original file line number	Diff line number	Diff line change
`@@ -26,4 +26,16 @@ public static function forJsonDecodingError(int $jsonLastError, string $rawResul`
`26`	`26`	`)`
`27`	`27`	`);`
`28`	`28`	`}`
	`29`	`+`
	`30`	`+ public static function forCapabilitiesRetrievalError(\Exception $previousException): self`
	`31`	`+ {`
	`32`	`+ return new self(`
	`33`	`+ sprintf(`
	`34`	`+ 'Existing Capabilities were not provided, and they also cannot be read from Selenium Grid'`
	`35`	`+ . ' (error: "%s"). You are probably not using Selenium Grid, so to reuse the previous session,'`
	`36`	`+ . ' Capabilities must be explicitly provided to createBySessionID() method.',`
	`37`	`+ $previousException->getMessage()`
	`38`	`+ )`
	`39`	`+ );`
	`40`	`+ }`
`29`	`41`	`}`
Original file line number	Diff line number	Diff line change
`@@ -110,6 +110,11 @@ public static function isW3cProtocolBuild()`
`110`	`110`	`return getenv('DISABLE_W3C_PROTOCOL') !== '1';`
`111`	`111`	`}`
`112`	`112`
	`113`	`+ public static function isSeleniumServerUsed(): bool`
	`114`	`+ {`
	`115`	`+ return getenv('SELENIUM_SERVER') === '1';`
	`116`	`+ }`
	`117`	`+`
`113`	`118`	`public static function skipForW3cProtocol($message = 'Not supported by W3C specification')`
`114`	`119`	`{`
`115`	`120`	`if (static::isW3cProtocolBuild()) {`