From 45f3b36981653619d29b2fc691688ddb2cb6553a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Ro=CC=88=C3=9Fner?= Date: Fri, 6 Jun 2025 15:21:34 +0200 Subject: [PATCH 1/3] Refactor exception handling while destructing the client. Bump php-webdriver/webdriver to 1.14.0, because they introduced a common interface to identify all exceptions thrown in php-webdriver. --- composer.json | 2 +- src/Client.php | 16 ++++++++-------- tests/ClientTest.php | 40 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/composer.json b/composer.json index 41801fec..a26a3f15 100644 --- a/composer.json +++ b/composer.json @@ -27,7 +27,7 @@ "php": ">=8.0", "ext-dom": "*", "ext-libxml": "*", - "php-webdriver/webdriver": "^1.8.2", + "php-webdriver/webdriver": "^1.14.0", "symfony/browser-kit": "^5.4 || ^6.4 || ^7.0", "symfony/dependency-injection": "^5.4 || ^6.4 || ^7.0", "symfony/deprecation-contracts": "^2.4 || ^3", diff --git a/src/Client.php b/src/Client.php index a1c53e51..062ce62d 100644 --- a/src/Client.php +++ b/src/Client.php @@ -14,8 +14,8 @@ namespace Symfony\Component\Panther; use Facebook\WebDriver\Exception\NoSuchElementException; +use Facebook\WebDriver\Exception\PhpWebDriverExceptionInterface; use Facebook\WebDriver\Exception\TimeoutException; -use Facebook\WebDriver\Exception\WebDriverException; use Facebook\WebDriver\JavaScriptExecutor; use Facebook\WebDriver\Remote\RemoteWebDriver; use Facebook\WebDriver\WebDriver; @@ -108,11 +108,7 @@ public function __wakeup(): void public function __destruct() { - try { - $this->quit(); - } catch (WebDriverException) { - // ignore - } + $this->quit(); } public function start(): void @@ -602,8 +598,12 @@ public function getWindowHandles(): array public function quit(bool $quitBrowserManager = true): void { if (null !== $this->webDriver) { - $this->webDriver->quit(); - $this->webDriver = null; + try { + $this->webDriver->quit(); + $this->webDriver = null; + } catch (PhpWebDriverExceptionInterface) { + // ignore + } } if ($quitBrowserManager) { diff --git a/tests/ClientTest.php b/tests/ClientTest.php index edda41dd..b3641fd9 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -14,9 +14,11 @@ namespace Symfony\Component\Panther\Tests; use Facebook\WebDriver\Exception\ElementClickInterceptedException; +use Facebook\WebDriver\Exception\Internal\UnexpectedResponseException; use Facebook\WebDriver\Exception\InvalidSelectorException; use Facebook\WebDriver\Exception\StaleElementReferenceException; use Facebook\WebDriver\Exception\TimeoutException; +use Facebook\WebDriver\Exception\WebDriverException; use Facebook\WebDriver\JavaScriptExecutor; use Facebook\WebDriver\WebDriver; use Facebook\WebDriver\WebDriverExpectedCondition; @@ -30,6 +32,7 @@ use Symfony\Component\Panther\DomCrawler\Crawler; use Symfony\Component\Panther\Exception\LogicException; use Symfony\Component\Panther\PantherTestCase; +use Symfony\Component\Panther\ProcessManager\BrowserManagerInterface; use Symfony\Component\Panther\ProcessManager\ChromeManager; use Symfony\Contracts\HttpClient\HttpClientInterface; @@ -627,4 +630,41 @@ public static function providePrefersReducedMotion(): iterable yield 'Chrome' => [PantherTestCase::CHROME]; yield 'Firefox' => [PantherTestCase::FIREFOX]; } + + /** + * In order to test if destructing is fine, even when WebDriver throws exceptions, we test Client directly and use + * mocks to simulate wrong webdriver behaviour. + * + * @dataProvider provideSupportedWebDriverExceptions + */ + public function testDestructingIgnoringWebDriverException(\Throwable $webDriverException): void + { + $webDriver = $this->createMock(WebDriver::class); + $browserManager = $this->createMock(BrowserManagerInterface::class); + + $browserManager->expects(self::once()) + ->method('start') + ->willReturn($webDriver); + $browserManager->expects(self::once()) + ->method('quit'); + + $webDriver->expects(self::once()) + ->method('quit') + ->willThrowException($webDriverException); + + $client = new Client($browserManager); + $client->start(); + try { + unset($client); + } catch (\Throwable) { + // if any unexpected exception is thrown, this test will fail! + self::fail('This must not throw an exception.'); + } + } + + public static function provideSupportedWebDriverExceptions(): iterable + { + yield 'WebDriverException' => [new WebDriverException('something went wrong')]; + yield 'UnexpectedResponseException' => [UnexpectedResponseException::forError('something went wrong')];; + } } From aed289ac4316d85f4b5f85d7908ec2ccbac8649a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Ro=CC=88=C3=9Fner?= Date: Fri, 6 Jun 2025 15:43:16 +0200 Subject: [PATCH 2/3] Rename and refactor test to validate browser manager quitting during destruction despite WebDriver exceptions. --- tests/ClientTest.php | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index b3641fd9..018ed40f 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -632,39 +632,35 @@ public static function providePrefersReducedMotion(): iterable } /** - * In order to test if destructing is fine, even when WebDriver throws exceptions, we test Client directly and use + * In order to test if destructing is fine, even when WebDriver throws exceptions, we test Client directly and use * mocks to simulate wrong webdriver behaviour. - * + * * @dataProvider provideSupportedWebDriverExceptions */ - public function testDestructingIgnoringWebDriverException(\Throwable $webDriverException): void + public function testQuitBrowserManagerDuringDeconstructionEvenIfWebDriverThrowsExceptions(\Throwable $exception): void { $webDriver = $this->createMock(WebDriver::class); $browserManager = $this->createMock(BrowserManagerInterface::class); - + $browserManager->expects(self::once()) ->method('start') ->willReturn($webDriver); $browserManager->expects(self::once()) ->method('quit'); - + $webDriver->expects(self::once()) ->method('quit') - ->willThrowException($webDriverException); - + ->willThrowException($exception); + $client = new Client($browserManager); $client->start(); - try { - unset($client); - } catch (\Throwable) { - // if any unexpected exception is thrown, this test will fail! - self::fail('This must not throw an exception.'); - } + + unset($client); } public static function provideSupportedWebDriverExceptions(): iterable { yield 'WebDriverException' => [new WebDriverException('something went wrong')]; - yield 'UnexpectedResponseException' => [UnexpectedResponseException::forError('something went wrong')];; + yield 'UnexpectedResponseException' => [UnexpectedResponseException::forError('something went wrong')]; } } From 3cc9549c8fc178ee7bb920bf29b642698bdea55c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Ro=CC=88=C3=9Fner?= Date: Fri, 6 Jun 2025 15:57:16 +0200 Subject: [PATCH 3/3] name mocks as mocks. --- tests/ClientTest.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/ClientTest.php b/tests/ClientTest.php index 018ed40f..4db1ab93 100644 --- a/tests/ClientTest.php +++ b/tests/ClientTest.php @@ -639,20 +639,20 @@ public static function providePrefersReducedMotion(): iterable */ public function testQuitBrowserManagerDuringDeconstructionEvenIfWebDriverThrowsExceptions(\Throwable $exception): void { - $webDriver = $this->createMock(WebDriver::class); - $browserManager = $this->createMock(BrowserManagerInterface::class); + $webDriverMock = $this->createMock(WebDriver::class); + $browserManagerMock = $this->createMock(BrowserManagerInterface::class); - $browserManager->expects(self::once()) + $browserManagerMock->expects(self::once()) ->method('start') - ->willReturn($webDriver); - $browserManager->expects(self::once()) + ->willReturn($webDriverMock); + $browserManagerMock->expects(self::once()) ->method('quit'); - $webDriver->expects(self::once()) + $webDriverMock->expects(self::once()) ->method('quit') ->willThrowException($exception); - $client = new Client($browserManager); + $client = new Client($browserManagerMock); $client->start(); unset($client);