Moved JsTrait methods to ElementTrait. #370
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change migrates JavaScript-related step definitions and helper methods from the now-removed Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Behat Tester
participant ElementTrait as ElementTrait
participant Browser as Browser Session
Tester->>ElementTrait: Call step (e.g., "I accept all confirmations")
ElementTrait->>Browser: Execute JS (e.g., override window.confirm)
Browser-->>ElementTrait: JS execution result
ElementTrait-->>Tester: Step completes
Tester->>ElementTrait: Call step (e.g., "I click on element '.selector'")
ElementTrait->>Browser: Find and click element via JS
Browser-->>ElementTrait: JS execution result
ElementTrait-->>Tester: Step completes
Tester->>ElementTrait: Call step (e.g., "I trigger 'click' event on '.selector'")
ElementTrait->>Browser: Trigger event via JS
Browser-->>ElementTrait: JS execution result
ElementTrait-->>Tester: Step completes
Possibly related PRs
Suggested labels
🪧 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 full review 8000 |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
behat.yml(1 hunks)src/ElementTrait.php(1 hunks)src/JsTrait.php(0 hunks)steps.md(2 hunks)tests/behat/bootstrap/FeatureContext.php(0 hunks)tests/behat/features/element.feature(1 hunks)tests/behat/features/js.feature(0 hunks)tests/behat/fixtures/d10/composer.json(1 hunks)
💤 Files with no reviewable changes (3)
- tests/behat/bootstrap/FeatureContext.php
- tests/behat/features/js.feature
- src/JsTrait.php
🧰 Additional context used
🪛 YAMLlint (1.35.1)
behat.yml
[warning] 74-74: too few spaces before comment
(comments)
🪛 markdownlint-cli2 (0.17.2)
steps.md
594-594: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
596-596: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
604-604: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
606-606: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
614-614: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
616-616: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🔇 Additional comments (7)
tests/behat/fixtures/d10/composer.json (1)
48-48: Dependency version upgrade - appropriate for the expanded featuresThe update of
drevops/behat-screenshotfrom a previous version (not shown) to^2.1aligns with the enhanced screenshot configuration inbehat.yml, where new features like automatic capture on failure, fullscreen options, and additional metadata are being utilized.behat.yml (1)
72-79: Enhanced screenshot configuration improves test debuggingThese additions effectively enhance the screenshot capabilities by:
- Automatically capturing screenshots on test failures
- Ensuring fullscreen captures with a specified algorithm
- Adding rich contextual metadata (URL, feature, step info, timestamp)
All these improvements will help with debugging JavaScript interactions that were migrated from JsTrait to ElementTrait.
#!/bin/bash # Verify the DrevOps\BehatScreenshotExtension supports these new options ast-grep --pattern $'DrevOps\\\\BehatScreenshotExtension(.*)\{ $$$ (on_failed|always_fullscreen|fullscreen_algorithm|info_types) $$$ \}'🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 74-74: too few spaces before comment
(comments)
tests/behat/features/element.feature (5)
203-207: Click element test properly implements the migrated functionThis scenario effectively tests the element click functionality that was migrated from JsTrait to ElementTrait.
208-214: JS event trigger test provides good coverageGreat implementation of a test that both triggers a JS event and verifies the resulting DOM changes (overlay becoming visible), providing comprehensive testing of the migrated functionality.
215-223: Confirmation dialog accept test is well structuredThe test checks both the initial state (confirmation button exists but result doesn't), sets up the confirmation behavior, and then verifies the correct outcome after action.
224-232: Confirmation dialog decline test complements the accept testGood implementation of the negative case to ensure both confirmation dialog responses work correctly. Together with the previous test, this provides complete coverage of the dialog interaction functionality.
233-237: Scroll functionality test is comprehensiveThe test properly verifies both the scrolling action and the resulting element position, covering both parts of the functionality.
| @@ -69,6 +69,14 @@ default: | |||
| DrevOps\BehatScreenshotExtension: | |||
| dir: '%paths.base%/.logs/screenshots' | |||
| purge: false # Change to 'true' (no quotes) to purge screenshots on each run. | |||
| on_failed: true | |||
| always_fullscreen: true | |||
| fullscreen_algorithm: resize # 'stitch' (only if GD ext available) or 'resize' | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix spacing in comment
There's a minor formatting issue with the comment spacing.
- fullscreen_algorithm: resize # 'stitch' (only if GD ext available) or 'resize'
+ fullscreen_algorithm: resize # 'stitch' (only if GD ext available) or 'resize'📝 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.
| fullscreen_algorithm: resize # 'stitch' (only if GD ext available) or 'resize' | |
| fullscreen_algorithm: resize # 'stitch' (only if GD ext available) or 'resize' |
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 74-74: too few spaces before comment
(comments)
| #### Assert the element :selector should be at the top of the viewport | ||
|
|
||
| ```gherkin | ||
| @Then the element :selector should be at the top of the viewport | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Viewport position assertion step documentation needs an example
The documentation for asserting element position in viewport is missing an example unlike other steps.
Add an example for consistency with other step documentation:
#### Assert the element :selector should be at the top of the viewport
```gherkin
@Then the element :selector should be at the top of the viewport+Example:
+gherkin +Then the element "#header" should be at the top of the viewport +
<!-- This is an auto-generated comment by CodeRabbit -->
| #### Accept confirmation dialogs appearing on the page | ||
|
|
||
| ```gherkin | ||
| @Given I accept all confirmation dialogs | ||
| ``` | ||
| Example: | ||
| ```gherkin | ||
| Given I accept all confirmation dialogs | ||
| ``` | ||
|
|
||
| #### Do not accept confirmation dialogs appearing on the page | ||
|
|
||
| ```gherkin | ||
| @Given I do not accept any confirmation dialogs | ||
| ``` | ||
| Example: | ||
| ```gherkin | ||
| Given I do not accept any confirmation dialogs | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Documentation for confirmation dialog steps is well migrated
The steps for accepting and declining confirmation dialogs have been properly moved from JsTrait to ElementTrait, maintaining consistent documentation.
Add blank lines around the code blocks to improve formatting:
#### Accept confirmation dialogs appearing on the page
+
```gherkin
@Given I accept all confirmation dialogsExample:
+
Given I accept all confirmation dialogsDo not accept confirmation dialogs appearing on the page
@Given I do not accept any confirmation dialogsExample:
+
Given I do not accept any confirmation dialogs
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.17.2)</summary>
594-594: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
---
596-596: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
---
604-604: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
---
606-606: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
</details>
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| #### Click on the element defined by the selector | ||
|
|
||
| ```gherkin | ||
| @When I click on the element :selector | ||
| ``` | ||
| Example: | ||
| ```gherkin | ||
| When I click on the element ".button" | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Element click step documentation properly relocated
The documentation for clicking elements has been appropriately moved to ElementTrait.
Add blank lines around the code blocks to improve formatting:
#### Click on the element defined by the selector
+
```gherkin
@When I click on the element :selectorExample:
+
When I click on the element ".button"
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.17.2)</summary>
614-614: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
---
616-616: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
</details>
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| #### When I trigger the JS event :event on the element :selector | ||
|
|
||
| ```gherkin | ||
| @When I trigger the JS event :event on the element :selector | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
JS event trigger step documentation needs an example
The documentation for triggering JS events is missing an example unlike other steps.
Add an example for consistency with other step documentation:
#### When I trigger the JS event :event on the element :selector
```gherkin
@When I trigger the JS event :event on the element :selector+Example:
+gherkin +When I trigger the JS event "click" on the element "#my-button" +
<!-- This is an auto-generated comment by CodeRabbit -->
src/ElementTrait.php
Outdated
| /** | ||
| * Click on the element defined by the selector. | ||
| * | ||
| * @code | ||
| * When I click on the element ".button" | ||
| * @endcode | ||
| * | ||
| * @When I click on the element :selector | ||
| * | ||
| * @javascript | ||
| */ | ||
| public function elementClick(string $element): void { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve parameter naming consistency
The method signature uses $element as the parameter name, but the PHPDoc and step definition use $selector. Consider renaming the parameter for consistency.
- public function elementClick(string $element): void {
+ public function elementClick(string $selector): void {
$element = $this
->getSession()
->getPage()
->find('css', $element);
$element->click();
}Committable suggestion skipped: line range outside the PR's diff.
src/ElementTrait.php
Outdated
| /** | ||
| * Scroll to an element with ID. | ||
| * | ||
| * @When I scroll to the element :selector | ||
| */ | ||
| public function elementScrollTo(string $selector): void { | ||
| $this->getSession()->executeScript(" | ||
| var element = document.querySelector('" . $selector . "'); | ||
| element.scrollIntoView( true ); | ||
| "); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for element scrolling
Similar to the click method, this function does not verify that the element exists before attempting to scroll to it, which could lead to unexpected behavior.
public function elementScrollTo(string $selector): void {
+ $page = $this->getSession()->getPage();
+ $element = $page->find('css', $selector);
+
+ if (!$element) {
+ throw new \RuntimeException(sprintf('Cannot scroll to element "%s" as it was not found on the page', $selector));
+ }
+
$this->getSession()->executeScript("
var element = document.querySelector('" . $selector . "');
element.scrollIntoView( true );
");
}📝 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.
| /** | |
| * Scroll to an element with ID. | |
| * | |
| * @When I scroll to the element :selector | |
| */ | |
| public function elementScrollTo(string $selector): void { | |
| $this->getSession()->executeScript(" | |
| var element = document.querySelector('" . $selector . "'); | |
| element.scrollIntoView( true ); | |
| "); | |
| } | |
| /** | |
| * Scroll to an element with ID. | |
| * | |
| * @When I scroll to the element :selector | |
| */ | |
| public function elementScrollTo(string $selector): void { | |
| $page = $this->getSession()->getPage(); | |
| $element = $page->find('css', $selector); | |
| if (!$element) { | |
| throw new \RuntimeException(sprintf( | |
| 8000 | 'Cannot scroll to element "%s" as it was not found on the page', |
| $selector | |
| )); | |
| } | |
| $this->getSession()->executeScript(" | |
| var element = document.querySelector('" . $selector . "'); | |
| element.scrollIntoView( true ); | |
| "); | |
| } |
| /** | ||
| * Assert the element :selector should be at the top of the viewport. | ||
| * | ||
| * @Then the element :selector should be at the top of the viewport | ||
| */ | ||
| public function elementAssertElementAtTopOfViewport(string $selector): void { | ||
| $script = <<<JS | ||
| (function() { | ||
| var element = document.querySelector('{$selector}'); | ||
| var rect = element.getBoundingClientRect(); | ||
| return (rect.top >= 0 && rect.top <= window.innerHeight); | ||
| })(); | ||
| JS; | ||
| $result = $this->getSession()->evaluateScript($script); | ||
| if (!$result) { | ||
| throw new \Exception(sprintf("Element with selector '%s' is not at the top of the viewport.", $selector)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve viewport position check precision
The condition on line 123 checks if an element is anywhere within the viewport, not specifically at the top. Consider refining this to check for a smaller distance from the top if that's the intended behavior.
- return (rect.top >= 0 && rect.top <= window.innerHeight);
+ return (rect.top >= 0 && rect.top <= 10); // 10px tolerance for "top of viewport"
src/ElementTrait.php
Outdated
| protected function elementExectuteJs(string $selector, string $script) { | ||
| $driver = $this->getSession()->getDriver(); | ||
| $scriptWrapper = "return (function() { | ||
| {{SCRIPT}} | ||
| }());"; | ||
| $script = str_replace('{{ELEMENT}}', sprintf("document.querySelector('%s')", $selector), $script); | ||
| $script = str_replace('{{SCRIPT}}', $script, $scriptWrapper); | ||
|
|
||
| return $driver->evaluateScript($script); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix method name typo "Exectute" → "Execute"
There's a typo in the method name elementExectuteJs - it should be elementExecuteJs.
- protected function elementExectuteJs(string $selector, string $script) {
+ protected function elementExecuteJs(string $selector, string $script) {Don't forget to update the method call on line 200 as well:
- $result = $this->elementExectuteJs($selector, $script);
+ $result = $this->elementExecuteJs($selector, $script);
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #370 +/- ##
==========================================
- Coverage 81.63% 81.61% -0.03%
==========================================
Files 32 31 -1
Lines 1677 1675 -2
==========================================
- Hits 1369 1367 -2
Misses 308 308 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d006af2 to
2c49295
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores