Fix tests - handle oauth consent skipped #43
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant TestScript
participant BrowserDriver
participant OAuthWindow
TestScript->>BrowserDriver: Click login button
BrowserDriver->>OAuthWindow: Open OAuth login window
TestScript->>OAuthWindow: Enter email and password, submit form
loop Wait for consent or window close (up to 5s)
OAuthWindow-->>TestScript: Is window still open?
alt Consent button present
TestScript->>OAuthWindow: Click "#allow" consent button
else
TestScript->>TestScript: Wait and retry
end
end
OAuthWindow-->>BrowserDriver: Window closes if consent granted
TestScript->>BrowserDriver: Switch back to original window
BrowserDriver->>TestScript: Wait for iframe to load
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/TestFiles/Modules/Web.php (2)
218-223: Consider more specific exception handling.While the current implementation works, it catches all exceptions rather than just element-not-found exceptions. This could potentially mask other issues.
-try { - $this->driver->find_element_and_click( '#allow' ); - break; -} catch ( \Exception $e ) { // phpcs:ignore - // consent button not found, do nothing. -} +try { + $this->driver->find_element_and_click( '#allow' ); + break; +} catch ( \Facebook\WebDriver\Exception\NoSuchElementException $e ) { + // consent button not found, do nothing. +}
209-210: Consider extracting magic numbers to constants.The 5-second timeout and 10ms sleep interval could be defined as constants or class properties for better maintainability.
+ private const OAUTH_CONSENT_TIMEOUT_SECONDS = 5; + private const OAUTH_CONSENT_POLL_INTERVAL_MICROSECONDS = 10000; // Then in the method: - $start = microtime( true ); - while ( ( microtime( true ) - $start ) < 5 ) { + $start = microtime( true ); + while ( ( microtime( true ) - $start ) < self::OAUTH_CONSENT_TIMEOUT_SECONDS ) { // And later: - usleep( 10000 ); + usleep( self::OAUTH_CONSENT_POLL_INTERVAL_MICROSECONDS );Also applies to: 225-226
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/TestFiles/Modules/Web.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/TestFiles/Modules/Web.php (1)
tests/TestFiles/Modules/Webdriver.php (3)
get_driver(43-45)find_element_and_input(77-79)find_element_and_click(73-75)
🔇 Additional comments (4)
tests/TestFiles/Modules/Web.php (4)
195-199: The window handle management looks good.Properly obtains the window handles and switches to the new OAuth window.
202-202: Helpful comment addition.Good clarification on why the logged-in page is not shown. This explains the behavior better than the previous comment.
209-226: Great implementation of robust OAuth consent handling.This is a solid improvement that addresses the core issue. The implementation now handles both cases:
- When consent is already granted and the window closes automatically
- When the consent page is shown and requires clicking the allow button
The timed loop with exception handling is a much more reliable approach than the previous implementation.
228-230: The window switching after consent looks good.Correctly switches back to the original window and waits for the iframe to be available.
Summary by CodeRabbit