[#532] Added support for Functional JS tests.#2300
Merged
AlexSkrypnyk merged 1 commit intomainfrom Feb 18, 2026
Merged
Conversation
WalkthroughAdds end-to-end FunctionalJavascript support: new Ahoy command, PHPUnit suite and bootstrap, browser screenshot trait, example module/theme tests and bases, docs and config updates, and workflow test additions validating success and failure runs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor DevCLI as "Developer (Ahoy CLI)"
participant PHPUnit as "PHPUnit (testsuite=functional-javascript)"
participant Selenium as "Selenium Hub (Chrome)"
participant Browser as "Browser (Chrome)"
participant Drupal as "Drupal web server (nginx/PHP-FPM)"
participant DB as "Database"
DevCLI->>PHPUnit: run `ahoy test-functional-javascript`
PHPUnit->>Selenium: request WebDriver session (goog:chromeOptions)
Selenium->>Browser: launch Chrome with flags
Browser->>Drupal: HTTP requests to pages (BROWSERTEST_OUTPUT_BASE_URL)
Drupal->>DB: perform test DB operations
Browser->>PHPUnit: return JS evaluation & screenshots
PHPUnit->>DevCLI: emit exit code, XML report, and browser_output artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.vortex/tests/phpunit/Traits/Subtests/SubtestAhoyTrait.php:
- Around line 457-485: The hard-coded test path in method
subtestAhoyTestFunctionalJavascript is pointing to the wrong module; update the
$file variable so it references ys_base instead of sw_base (i.e. change the path
'/modules/custom/sw_base/tests/src/FunctionalJavascript/ExampleTest.php' to use
'/modules/custom/ys_base/...'), leaving the rest of the method
(assertFileExists, fileBackup, File::replaceContentInFile, fileRestore,
cmd/cmdFail, syncToHost/syncToContainer calls) unchanged so the initial
existence check and subsequent modifications target the correct file.
In `@phpunit.xml`:
- Around line 63-64: Update the MINK_DRIVER_ARGS_WEBDRIVER environment value to
remove outdated flags and add Chrome 120+ headless best practices: remove
"--test-type" and "--disable-gpu", avoid globally disabling extensions (replace
"--disable-extensions" with using goog:chromeOptions.excludeSwitches for
specific switches if needed), and add modern headless flags such as "--headless"
(or "--headless=new"), "--window-size=1920,1080", and "--disable-dev-shm-usage";
adjust the JSON under the "goog:chromeOptions" args array (the
MINK_DRIVER_ARGS_WEBDRIVER value string) accordingly so tests run
deterministically in Docker/CI.
In `@tests/phpunit/bootstrap.php`:
- Around line 22-25: The mkdir call that creates $browser_output_dir may fail
silently; after attempting creation with mkdir($browser_output_dir, 0775, TRUE)
check its return value and/or re-check is_dir($browser_output_dir) and on
failure either log a clear error (e.g. error_log with $browser_output_dir and
reason from error_get_last()) or throw an exception (e.g. throw new
\RuntimeException("Failed to create directory: $browser_output_dir")) so tests
fail fast with a clear message; update the code around the mkdir call and use
the existing $browser_output_dir variable and the mkdir function name to locate
where to add this handling.
In
`@web/themes/custom/your_site_theme/tests/src/FunctionalJavascript/YourSiteThemeFunctionalJavascriptTestBase.php`:
- Around line 21-34: YourSiteThemeFunctionalJavascriptTestBase duplicates
YsBaseFunctionalJavascriptTestBase; change
YourSiteThemeFunctionalJavascriptTestBase to extend
YsBaseFunctionalJavascriptTestBase (instead of WebDriverTestBase), remove
duplicated trait usages and shared members (ArrayTrait, AssertTrait,
BrowserHtmlDebugTrait, MockTrait, ReflectionTrait and the defaultTheme if
already present in YsBaseFunctionalJavascriptTestBase), and only keep or
override unique bits (e.g., override $defaultTheme = 'stark' if needed) so the
theme base reuses the module base class functionality.
Comment on lines
+22
to
+25
| $browser_output_dir = dirname(__DIR__, 2) . '/web/sites/simpletest/browser_output'; | ||
| if (!is_dir($browser_output_dir)) { | ||
| mkdir($browser_output_dir, 0775, TRUE); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding error handling for directory creation.
If mkdir fails silently, subsequent tests may fail with confusing errors when they try to write to the non-existent directory. Consider logging a warning or throwing an exception.
♻️ Suggested improvement
// `@see` https://www.drupal.org/project/drupal/issues/2992069
$browser_output_dir = dirname(__DIR__, 2) . '/web/sites/simpletest/browser_output';
-if (!is_dir($browser_output_dir)) {
- mkdir($browser_output_dir, 0775, TRUE);
+if (!is_dir($browser_output_dir) && !@mkdir($browser_output_dir, 0775, true) && !is_dir($browser_output_dir)) {
+ trigger_error('Failed to create browser output directory: ' . $browser_output_dir, E_USER_WARNING);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/phpunit/bootstrap.php` around lines 22 - 25, The mkdir call that
creates $browser_output_dir may fail silently; after attempting creation with
mkdir($browser_output_dir, 0775, TRUE) check its return value and/or re-check
is_dir($browser_output_dir) and on failure either log a clear error (e.g.
error_log with $browser_output_dir and reason from error_get_last()) or throw an
exception (e.g. throw new \RuntimeException("Failed to create directory:
$browser_output_dir")) so tests fail fast with a clear message; update the code
around the mkdir call and use the existing $browser_output_dir variable and the
mkdir function name to locate where to add this handling.
Comment on lines
+21
to
+34
| abstract class YourSiteThemeFunctionalJavascriptTestBase extends WebDriverTestBase { | ||
|
|
||
| use ArrayTrait; | ||
| use AssertTrait; | ||
| use BrowserHtmlDebugTrait; | ||
| use MockTrait; | ||
| use ReflectionTrait; | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected $defaultTheme = 'stark'; | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing code duplication with the module base class.
YourSiteThemeFunctionalJavascriptTestBase is nearly identical to YsBaseFunctionalJavascriptTestBase. Consider having the theme base class extend the module base class to avoid duplication:
♻️ Alternative approach
-use Drupal\FunctionalJavascriptTests\WebDriverTestBase;
-use Drupal\Tests\ys_base\Traits\ArrayTrait;
-use Drupal\Tests\ys_base\Traits\AssertTrait;
-use Drupal\Tests\ys_base\Traits\BrowserHtmlDebugTrait;
-use Drupal\Tests\ys_base\Traits\MockTrait;
-use Drupal\Tests\ys_base\Traits\ReflectionTrait;
+use Drupal\Tests\ys_base\FunctionalJavascript\YsBaseFunctionalJavascriptTestBase;
-abstract class YourSiteThemeFunctionalJavascriptTestBase extends WebDriverTestBase {
-
- use ArrayTrait;
- use AssertTrait;
- use BrowserHtmlDebugTrait;
- use MockTrait;
- use ReflectionTrait;
-
- /**
- * {`@inheritdoc`}
- */
- protected $defaultTheme = 'stark';
-
+abstract class YourSiteThemeFunctionalJavascriptTestBase extends YsBaseFunctionalJavascriptTestBase {
+ // Theme-specific overrides can be added here.
}If maintaining separate base classes is intentional (e.g., to allow independent evolution), the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@web/themes/custom/your_site_theme/tests/src/FunctionalJavascript/YourSiteThemeFunctionalJavascriptTestBase.php`
around lines 21 - 34, YourSiteThemeFunctionalJavascriptTestBase duplicates
YsBaseFunctionalJavascriptTestBase; change
YourSiteThemeFunctionalJavascriptTestBase to extend
YsBaseFunctionalJavascriptTestBase (instead of WebDriverTestBase), remove
duplicated trait usages and shared members (ArrayTrait, AssertTrait,
BrowserHtmlDebugTrait, MockTrait, ReflectionTrait and the defaultTheme if
already present in YsBaseFunctionalJavascriptTestBase), and only keep or
override unique bits (e.g., override $defaultTheme = 'stark' if needed) so the
theme base reuses the module base class functionality.
AlexSkrypnyk
force-pushed
the
feature/532-func-js-tests
branch
from
fbbe458 to
5ecc81a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/modules/custom/ys_base/tests/src/Traits/BrowserHtmlDebugTrait.php`:
- Around line 27-38: The takeScreenshot method currently calls mkdir($directory,
0775, TRUE) without checking for failure which can cause createScreenshot to
fail; modify takeScreenshot to check the result of mkdir (or re-check
is_dir($directory)) and handle errors: if directory creation fails, log an error
(including $directory and the PHP error via error_get_last()) or throw an
exception before calling createScreenshot; update references to mkdir,
$directory, takeScreenshot, and createScreenshot so the code exits early on
failure and provides a clear diagnostic message.
---
Duplicate comments:
In @.vortex/tests/phpunit/Traits/Subtests/SubtestAhoyTrait.php:
- Around line 462-463: The test builds $file using $webroot and a hardcoded
module path that currently contains "sw_base"; update the constructed path
string in the assignment to $file so it points to "ys_base" (i.e., replace
'/modules/custom/sw_base/tests/src/FunctionalJavascript/ExampleTest.php' with
the equivalent path under ys_base) so that the subsequent
$this->assertFileExists($file) passes; ensure you only change the module segment
and keep $webroot and the rest of the path intact.
In `@phpunit.xml`:
- Around line 63-64: Update the MINK_DRIVER_ARGS_WEBDRIVER env value to use a
current, minimal and CI-appropriate Chrome flag set: validate and remove
deprecated flags like "--test-type", consolidate or keep only supported flags
(e.g., "--disable-extensions", "--disable-translate",
"--force-prefers-reduced-motion", "--window-size=1920,1080") and add recommended
CI flags if needed ("--no-sandbox", "--disable-dev-shm-usage", and headless mode
when running without display). Confirm the "goog:chromeOptions" shape and "w3c":
true remain compatible with your Selenium/ChromeDriver versions, then update the
env value string for MINK_DRIVER_ARGS_WEBDRIVER accordingly.
In `@tests/phpunit/bootstrap.php`:
- Around line 21-25: The current bootstrap creates $browser_output_dir with
mkdir but doesn't handle failures; update the block around $browser_output_dir
to check mkdir's boolean return and/or re-check is_dir after calling mkdir, and
if directory creation fails throw an exception or call exit with a clear error
(including $browser_output_dir and error context) so tests fail loudly;
reference the $browser_output_dir variable and the mkdir() call to locate and
modify the code.
In
`@web/themes/custom/your_site_theme/tests/src/FunctionalJavascript/YourSiteThemeFunctionalJavascriptTestBase.php`:
- Around line 1-34: Duplicate base-class functionality exists between
YourSiteThemeFunctionalJavascriptTestBase and
YsBaseFunctionalJavascriptTestBase; either consolidate or document the
divergence intent. Locate the class YourSiteThemeFunctionalJavascriptTestBase
and the other base class YsBaseFunctionalJavascriptTestBase, then either
refactor tests to extend the single shared base (removing duplicate
traits/logic) or add a clear comment/docblock inside
YourSiteThemeFunctionalJavascriptTestBase explaining why it must remain separate
and what differences it will hold going forward.
Comment on lines
+27
to
+38
| protected function takeScreenshot(string $suffix = ''): void { | ||
| $directory = getenv('BROWSERTEST_OUTPUT_DIRECTORY') ?: DRUPAL_ROOT . '/sites/simpletest/browser_output'; | ||
|
|
||
| if (!is_dir($directory)) { | ||
| mkdir($directory, 0775, TRUE); | ||
| } | ||
|
|
||
| $class = str_replace('\\', '_', static::class); | ||
| $suffix = $suffix !== '' ? $suffix : date('Ymd_His'); | ||
|
|
||
| $this->createScreenshot($directory . '/' . $class . '-' . $suffix . '.png'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding error handling for directory creation failure.
The mkdir() call can fail silently (returns false), and if it does, the subsequent createScreenshot() call will also fail. While this is a debug utility where silent failure may be acceptable, explicitly handling or logging the failure would improve debuggability.
♻️ Optional improvement for better diagnostics
protected function takeScreenshot(string $suffix = ''): void {
$directory = getenv('BROWSERTEST_OUTPUT_DIRECTORY') ?: DRUPAL_ROOT . '/sites/simpletest/browser_output';
- if (!is_dir($directory)) {
- mkdir($directory, 0775, TRUE);
+ if (!is_dir($directory) && !mkdir($directory, 0775, TRUE) && !is_dir($directory)) {
+ // Directory creation failed and directory still doesn't exist.
+ return;
}
$class = str_replace('\\', '_', static::class);
$suffix = $suffix !== '' ? $suffix : date('Ymd_His');
$this->createScreenshot($directory . '/' . $class . '-' . $suffix . '.png');
}📝 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.
Suggested change
| protected function takeScreenshot(string $suffix = ''): void { | |
| $directory = getenv('BROWSERTEST_OUTPUT_DIRECTORY') ?: DRUPAL_ROOT . '/sites/simpletest/browser_output'; | |
| if (!is_dir($directory)) { | |
| mkdir($directory, 0775, TRUE); | |
| } | |
| $class = str_replace('\\', '_', static::class); | |
| $suffix = $suffix !== '' ? $suffix : date('Ymd_His'); | |
| $this->createScreenshot($directory . '/' . $class . '-' . $suffix . '.png'); | |
| } | |
| protected function takeScreenshot(string $suffix = ''): void { | |
| $directory = getenv('BROWSERTEST_OUTPUT_DIRECTORY') ?: DRUPAL_ROOT . '/sites/simpletest/browser_output'; | |
| if (!is_dir($directory) && !mkdir($directory, 0775, TRUE) && !is_dir($directory)) { | |
| // Directory creation failed and directory still doesn't exist. | |
| return; | |
| } | |
| $class = str_replace('\\', '_', static::class); | |
| $suffix = $suffix !== '' ? $suffix : date('Ymd_His'); | |
| $this->createScreenshot($directory . '/' . $class . '-' . $suffix . '.png'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/ys_base/tests/src/Traits/BrowserHtmlDebugTrait.php` around
lines 27 - 38, The takeScreenshot method currently calls mkdir($directory, 0775,
TRUE) without checking for failure which can cause createScreenshot to fail;
modify takeScreenshot to check the result of mkdir (or re-check
is_dir($directory)) and handle errors: if directory creation fails, log an error
(including $directory and the PHP error via error_get_last()) or throw an
exception before calling createScreenshot; update references to mkdir,
$directory, takeScreenshot, and createScreenshot so the code exits early on
failure and provides a clear diagnostic message.
Member
Author
|
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Member
Author
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2300 +/- ##
==========================================
- Coverage 77.69% 77.13% -0.57%
==========================================
Files 117 110 -7
Lines 6174 6017 -157
Branches 44 0 -44
==========================================
- Hits 4797 4641 -156
+ Misses 1377 1376 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/532-func-js-tests
branch
from
5ecc81a to
43da4c1
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.vortex/tests/phpunit/Traits/Subtests/SubtestAhoyTrait.php:
- Around line 462-463: The assert is pointing to the wrong module path: update
the $file construction in SubtestAhoyTrait (the $file variable used with
$this->assertFileExists) to reference
'/modules/custom/ys_base/tests/src/FunctionalJavascript/ExampleTest.php' instead
of '/modules/custom/sw_base/tests/src/FunctionalJavascript/ExampleTest.php' so
the FunctionalJavascript ExampleTest in the ys_base module is found.
In `@phpunit.xml`:
- Around line 63-64: Verify and update the MINK_DRIVER_ARGS_WEBDRIVER value to
match current Selenium/ChromeDriver guidance: remove deprecated/redundant flags
like --test-type and (if not required) --disable-extensions, add recommended
CI-friendly flags such as --no-sandbox, --disable-dev-shm-usage and the modern
headless switch (e.g., --headless=new or --headless depending on the Chrome
version you target), keep or set an explicit --window-size, and validate the
final args against the Selenium 4+ ChromeOptions recommendations by running the
webdriver tests in CI; update the env value string in phpunit.xml
(MINK_DRIVER_ARGS_WEBDRIVER) accordingly and document which headless form you
chose and why in the test/CI README.
In `@tests/phpunit/bootstrap.php`:
- Around line 21-25: The browser output directory creation currently assumes
mkdir succeeds; check the return value of mkdir($browser_output_dir, 0775, TRUE)
and if it returns false (and the directory still doesn't exist via
is_dir($browser_output_dir)), fail fast with a clear error — e.g., write a
descriptive message to STDERR and exit non‑zero or throw an exception so tests
stop with a clear cause; ensure you reference the $browser_output_dir variable
and the mkdir/is_dir checks when adding this validation.
In `@web/modules/custom/ys_base/tests/src/Traits/BrowserHtmlDebugTrait.php`:
- Around line 27-38: The takeScreenshot method
(BrowserHtmlDebugTrait::takeScreenshot) currently ignores mkdir() failures;
update it to check the result of mkdir (or re-check is_dir after calling mkdir)
and if directory creation fails throw a RuntimeException (or call
$this->fail(...) in the test context) with a clear message that includes the
attempted $directory path and error_get_last() details so createScreenshot isn't
called on a non-existent path; keep the existing logic for building $class and
$suffix and only call createScreenshot when the directory exists.
Member
Author
|
|
Member
Author
|
Member
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #532
Summary by CodeRabbit
New Features
Tests
Documentation
Chores