[#272]: Assert no JS errors. #405
Conversation
WalkthroughA new PHP trait, Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Behat Tester
participant FeatureContext
participant JavascriptTrait
participant Browser
Tester->>FeatureContext: Start scenario (@javascript)
FeatureContext->>JavascriptTrait: jsCollectorBeforeScenario()
JavascriptTrait->>Browser: Inject JS error collector (window.onerror, console.error, etc.)
Tester->>FeatureContext: Execute monitored step
FeatureContext->>JavascriptTrait: jsCollectorBeforeStep()
JavascriptTrait->>Browser: Inject JS error collector (if needed)
FeatureContext->>Browser: Perform step actions (e.g., click button)
Browser-->>JavascriptTrait: Collect JS errors after step
FeatureContext->>JavascriptTrait: jsCollectorAfterStep()
Tester->>FeatureContext: End scenario (@javascript @no-js-errors)
FeatureContext->>JavascriptTrait: jsCollectorAssertScenarioClean()
JavascriptTrait->>Browser: Retrieve and assert no JS errors
Tester->>FeatureContext: Assert no JS errors on current page (optional)
FeatureContext->>JavascriptTrait: assertNoJsErrorsOnCurrentPage()
JavascriptTrait->>Browser: Retrieve and assert no JS errors
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/JavascriptTrait.php(1 hunks)tests/behat/bootstrap/FeatureContext.php(2 hunks)tests/behat/features/javascript.feature(1 hunks)
🔇 Additional comments (12)
tests/behat/bootstrap/FeatureContext.php (1)
33-33: Well-integrated trait usageThe JavascriptTrait has been properly imported and included in the FeatureContext class, following the established pattern of trait usage in this file.
Also applies to: 74-74
tests/behat/features/javascript.feature (4)
1-5: Good feature descriptionThe feature description clearly explains the purpose of the JavascriptTrait functionality, focusing on tracking and verifying JavaScript errors.
6-10: Well-structured first scenarioThis scenario correctly tests for the absence of JavaScript errors, which is a common use case. The @javascript tag ensures the trait's BeforeScenario hook runs properly.
11-16: Good error detection testThis scenario properly tests the error detection capability using a thrown JavaScript error, which is a common source of JavaScript errors.
17-21: Comprehensive error detectionThis scenario nicely tests the console.error detection capability, ensuring that errors logged via console.error are also captured by the trait.
src/JavascriptTrait.php (7)
1-14: Well-structured trait declarationThe namespace, imports, and documentation are appropriate. The trait clearly states its purpose of tracking and verifying JavaScript errors in web pages.
15-26: Good initialization with BeforeScenario hookThe @BeforeScenario hook correctly targets @javascript scenarios and resets the error buffer at the beginning of each scenario, which prevents errors from previous scenarios affecting current ones.
28-48: Consider improving error reporting in javascriptExecuteCodeThe method correctly executes JavaScript code on the page, but it silently ignores JavaScript errors. This might be confusing if a developer expects the step to fail when JavaScript errors occur.
Consider if it would be better to log a warning message when JavaScript errors are caught but ignored, to help with debugging. For example:
try { $this->getSession()->getDriver()->executeScript($code); } catch (\Throwable $e) { // Ignore JavaScript errors if (stripos($e->getMessage(), 'javascript error') === false) { throw $e; + } else { + // Log that a JavaScript error was caught and ignored + error_log('JavaScript error was caught and ignored: ' . $e->getMessage()); } }
50-67: Effective error assertion methodThe javascriptAssertError method correctly checks for JavaScript capability, injects the collector, and asserts that at least one JavaScript error was detected.
69-86: Clear error message for assertionsThe javascriptAssertNoErrors method provides a helpful error message that includes the specific JavaScript errors found, which aids in debugging.
88-129: Efficient error collector injectionThe injectCollector method efficiently checks if the collector is already installed before installing it, avoiding redundant script execution. The JavaScript collector properly captures both window.onerror events and console.error calls.
131-146: Robust error retrievalThe getJavaScriptErrors method properly handles type checking and exceptions when retrieving errors from the page.
| @javascript | ||
| Scenario: Assert JavaScript error is detected when one exists | ||
| When I go to "/" | ||
| And I execute the JavaScript code "throw new Error('Test JS error');" |
There was a problem hiding this comment.
this should be added to the test suite of the behat-steps itself, not the consumer trait.
There was a problem hiding this comment.
also, you could creae a fixture html page that has errors or that could throw an error if you click a button and use that for tests
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/JavascriptTrait.php (1)
179-183: 👍 Flexible JS-capable driver checkThanks for switching to
supportsJavascript()– this removes the hard dependency onSelenium2Driverand makes the trait work with any Mink driver that advertises JS support.
No further action needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (3)
src/JavascriptTrait.php(1 hunks)tests/behat/features/javascript.feature(1 hunks)tests/behat/fixtures/relative.html(1 hunks)
🔇 Additional comments (1)
tests/behat/fixtures/relative.html (1)
444-446: Fixture looks goodAdding an explicit error trigger makes the test deterministic and self-contained.
No issues spotted here.
| if (!$this->driverIsJsCapable()) { | ||
| return; | ||
| } | ||
|
|
||
| $this->harvestErrorsIntoRegistry(); | ||
| $path = $this->currentPath(); | ||
| $errors = $this->jsErrorRegistry[$path] ?? []; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inject the collector before harvesting to avoid undefined global
harvestErrorsIntoRegistry() assumes that window.__behatJsErrors has already been declared.
If assertNoJsErrorsOnCurrentPage() is called on a page that has never gone through a monitored step, this variable will be undefined, triggering a JavaScript reference error in some drivers and masking real problems.
public function assertNoJsErrorsOnCurrentPage(): void
{
if (!$this->driverIsJsCapable()) {
return;
}
+ // Ensure the collector exists even if no monitored step ran on this page.
+ $this->injectJsCollector();
$this->harvestErrorsIntoRegistry();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!$this->driverIsJsCapable()) { | |
| return; | |
| } | |
| $this->harvestErrorsIntoRegistry(); | |
| $path = $this->currentPath(); | |
| $errors = $this->jsErrorRegistry[$path] ?? []; | |
| public function assertNoJsErrorsOnCurrentPage(): void | |
| { | |
| if (!$this->driverIsJsCapable()) { | |
| return; | |
| } | |
| // Ensure the collector exists even if no monitored step ran on this page. | |
| $this->injectJsCollector(); | |
| $this->harvestErrorsIntoRegistry(); | |
| $path = $this->currentPath(); | |
| $errors = $this->jsErrorRegistry[$path] ?? []; |
🤖 Prompt for AI Agents
In src/JavascriptTrait.php around lines 79 to 86, the method
harvestErrorsIntoRegistry() is called before ensuring the JavaScript error
collector is injected, which can cause a JavaScript reference error if
window.__behatJsErrors is undefined. To fix this, inject the collector script
before calling harvestErrorsIntoRegistry() so that window.__behatJsErrors is
always defined when accessed.
| private function safeExecute(string $script): void | ||
| { | ||
| try { | ||
| $this->getSession()->executeScript($script); | ||
| } catch (DriverException $e) { | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Swallowing driver exceptions hides valuable diagnostics
safeExecute() silently drops every DriverException. When troubleshooting flaky tests it is extremely helpful to know why a script could not be executed (e.g., cross-origin restriction, browser crashed, etc.).
Consider at least logging the message at NOTICE/DEBUG level:
} catch (DriverException $e) {
- }
+ // Do not break the test flow, but surface the problem for debugging.
+ error_log(sprintf('[Behat JS-collector] DriverException: %s', $e->getMessage()));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function safeExecute(string $script): void | |
| { | |
| try { | |
| $this->getSession()->executeScript($script); | |
| } catch (DriverException $e) { | |
| } | |
| private function safeExecute(string $script): void | |
| { | |
| try { | |
| $this->getSession()->executeScript($script); | |
| } catch (DriverException $e) { | |
| // Do not break the test flow, but surface the problem for debugging. | |
| error_log(sprintf('[Behat JS-collector] DriverException: %s', $e->getMessage())); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/JavascriptTrait.php around lines 185 to 190, the safeExecute method
currently catches DriverException but silently ignores it, which hides important
diagnostic information. Modify the catch block to log the exception message at
NOTICE or DEBUG level using the appropriate logger, so that failures in script
execution are recorded for troubleshooting without interrupting the flow.
| @javascript @no-js-errors | ||
| Scenario: Assert JavaScript error is detected when one exists | ||
| When I visit "/sites/default/files/relative.html" | ||
| And the element "#js-error-trigger" should be displayed | ||
| And I click on the element "#js-error-trigger" | ||
|
|
There was a problem hiding this comment.
@no-js-errors tag makes the “error expected” scenario fail
The second scenario intentionally triggers a JavaScript error, but the @no-js-errors tag instructs JavascriptTrait to assert zero errors after the scenario, resulting in an unavoidable failure.
Remove the tag (or replace it with an explicit step that asserts the presence of errors):
- @javascript @no-js-errors
+ @javascript
Scenario: Assert JavaScript error is detected when one exists🤖 Prompt for AI Agents
In tests/behat/features/javascript.feature around lines 11 to 16, the scenario
that intentionally triggers a JavaScript error is incorrectly tagged with
@no-js-errors, which causes the test to fail by asserting zero errors. Remove
the @no-js-errors tag from this scenario or replace it with a step that
explicitly asserts the presence of JavaScript errors to correctly reflect the
test's intent.
#272
Summary by CodeRabbit