[#642] Added 'AccessibilityTrait' for tool-agnostic WCAG testing.#643
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds A11yTrait to run axe-core accessibility checks in Behat scenarios (explicit steps and automatic per-step mode), per-scenario HTML and JUnit reporting, tag-driven threshold/gate configuration, tests, fixtures, documentation, and supporting build/test config adjustments. ChangesA11yTrait accessibility feature
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composer.json`:
- Around line 66-70: The current "audit" block suppresses all severities
("ignore-severities": ["low","medium","high","critical"]) disabling Composer
security checks; update the "audit" configuration by removing the entire "audit"
block or at minimum change "ignore-severities" to only ["low"] (or delete that
key entirely) and keep "abandoned": "ignore" only if abandoned packages are the
sole concern so that medium/high/critical vulnerabilities are reported;
reference the "audit" object and its keys ("ignore-severities", "abandoned")
when making the change.
In `@src/AccessibilityTrait.php`:
- Around line 1-14: Add a proper import for the global RuntimeException in
AccessibilityTrait by adding "use RuntimeException;" near the other use
statements, then update the places where RuntimeException is thrown in this
trait (the throw statements currently using \RuntimeException and any
`@throws/doc` references) to use the imported symbol (remove the leading backslash
or switch to RuntimeException::class in throws annotations) so they follow
PSR-12 and consistent imports.
- Around line 249-255: The code that creates the report directory and writes
files (accessibilityGetReportDir(), mkdir, file_put_contents of
accessibilityRenderHtml() and accessibilityRenderJunit()) lacks error handling;
update the block so you verify mkdir succeeded (or directory exists after call),
check file_put_contents returns !== false for both writes, and surface failures
by throwing a clear exception or logging an error (including the $dir and $slug
from accessibilitySlug($this->accessibilityFeatureName) and
accessibilitySlug($this->accessibilityScenarioName)) so calling code won't
silently skip report generation.
In `@tests/behat/fixtures_drupal/d10/composer.json`:
- Around line 62-66: The composer.json test fixture currently disables Composer
security auditing by setting the "audit" block with "ignore-severities": ["low",
"medium", "high", "critical"]; update the "audit" configuration in this file so
security checks are not fully suppressed—either remove the entire "audit" block,
or change the "ignore-severities" value to only ["low"] (or remove
"critical"/higher severities) and keep "abandoned": "ignore" if you only want to
ignore abandoned packages; locate the "audit" object in composer.json and apply
one of these fixes to restore meaningful audit results.
In `@tests/behat/fixtures_drupal/d11/composer.json`:
- Around line 63-67: The composer.json test fixture currently disables Composer
security auditing via the "audit" block (keys "abandoned", "block-insecure", and
"ignore-severities"); update composer.json to re-enable meaningful audits by
either removing the entire "audit" object or changing "ignore-severities" from
["low","medium","high","critical"] to a minimal suppression such as ["low"] (or
keep only "abandoned": "ignore" if abandoned-packages are the only concern) and
ensure "block-insecure" remains false/appropriate so Composer will report
medium+ and critical vulnerabilities.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6910d1e-7f5b-490b-be65-170c27b4473e
📒 Files selected for processing (11)
README.mdSTEPS.mdcomposer.jsonsrc/AccessibilityTrait.phptests/behat/bootstrap/FeatureContext.phptests/behat/features/accessibility.featuretests/behat/fixtures/accessibility_clean.htmltests/behat/fixtures/accessibility_clean2.htmltests/behat/fixtures/accessibility_violations.htmltests/behat/fixtures_drupal/d10/composer.jsontests/behat/fixtures_drupal/d11/composer.json
Comment on lines
+1
to
+14
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace DrevOps\BehatSteps; | ||
|
|
||
| use Behat\Behat\Hook\Scope\AfterScenarioScope; | ||
| use Behat\Behat\Hook\Scope\AfterStepScope; | ||
| use Behat\Behat\Hook\Scope\BeforeScenarioScope; | ||
| use Behat\Hook\AfterScenario; | ||
| use Behat\Hook\AfterStep; | ||
| use Behat\Hook\BeforeScenario; | ||
| use Behat\Mink\Exception\ExpectationException; | ||
| use Behat\Step\Then; |
There was a problem hiding this comment.
Add import for RuntimeException.
RuntimeException is thrown on lines 349, 533, and 537 but not imported. Per PSR-12 standards, global classes should be imported.
📦 Add missing import
use Behat\Mink\Exception\ExpectationException;
use Behat\Step\Then;
+use RuntimeException;Then update the throws to remove the \ prefix if you added it, or keep them as RuntimeException::class in the throws.
📝 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
| <?php | |
| declare(strict_types=1); | |
| namespace DrevOps\BehatSteps; | |
| use Behat\Behat\Hook\Scope\AfterScenarioScope; | |
| use Behat\Behat\Hook\Scope\AfterStepScope; | |
| use Behat\Behat\Hook\Scope\BeforeScenarioScope; | |
| use Behat\Hook\AfterScenario; | |
| use Behat\Hook\AfterStep; | |
| use Behat\Hook\BeforeScenario; | |
| use Behat\Mink\Exception\ExpectationException; | |
| use Behat\Step\Then; | |
| <?php | |
| declare(strict_types=1); | |
| namespace DrevOps\BehatSteps; | |
| use Behat\Behat\Hook\Scope\AfterScenarioScope; | |
| use Behat\Behat\Hook\Scope\AfterStepScope; | |
| use Behat\Behat\Hook\Scope\BeforeScenarioScope; | |
| use Behat\Hook\AfterScenario; | |
| use Behat\Hook\AfterStep; | |
| use Behat\Hook\BeforeScenario; | |
| use Behat\Mink\Exception\ExpectationException; | |
| use Behat\Step\Then; | |
| use RuntimeException; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/AccessibilityTrait.php` around lines 1 - 14, Add a proper import for the
global RuntimeException in AccessibilityTrait by adding "use RuntimeException;"
near the other use statements, then update the places where RuntimeException is
thrown in this trait (the throw statements currently using \RuntimeException and
any `@throws/doc` references) to use the imported symbol (remove the leading
backslash or switch to RuntimeException::class in throws annotations) so they
follow PSR-12 and consistent imports.
Member
Author
There was a problem hiding this comment.
not importing - against standards
There was a problem hiding this comment.
@AlexSkrypnyk, understood — using \RuntimeException directly (without a use import) is the project's preferred convention for global PHP built-in classes. I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: AlexSkrypnyk
Repo: drevops/behat-steps PR: 643
File: src/AccessibilityTrait.php:1-14
Timestamp: 2026-05-28T10:09:36.498Z
Learning: In drevops/behat-steps, global PHP built-in/native exception classes (e.g., `\RuntimeException`, `\InvalidArgumentException`) are intentionally referenced with the backslash FQCN prefix (e.g., `throw new \RuntimeException(...)`) rather than being imported via `use` statements. Do not raise "missing import" review comments for global PHP namespace classes in this repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: AlexSkrypnyk
Repo: drevops/behat-steps PR: 560
File: src/ResponsiveTrait.php:173-173
Timestamp: 2026-04-09T08:32:59.254Z
Learning: In this project (drevops/behat-steps), Behat hook methods annotated with attributes like #[BeforeStep], #[BeforeScenario] (and similar Behat hook attributes) must keep their required scope parameter in the method signature (e.g., `BeforeStepScope $scope`, `BeforeScenarioScope $scope`) as required by the Behat framework contract. Even if the parameter is not referenced inside the method body, do not rename it to an unused placeholder like `$_scope` or otherwise change its type/name—treat it as structurally required. PHPMD `UnusedFormalParameter` warnings on these specific hook-method scope parameters are expected false positives and should be ignored.
Learnt from: AlexSkrypnyk
Repo: drevops/behat-steps PR: 623
File: STEPS.md:2647-2650
Timestamp: 2026-04-10T08:13:52.837Z
Learning: In drevops/behat-steps, docs generation (docs.php when converting docblock code/endcode blocks into STEPS.md) strips `/*` sequences from examples. When writing glob/wildcard path patterns inside trait docblocks, avoid a literal slash-star `/*`—use patterns with a trailing `*` but no preceding slash. For example, write `/news*` instead of `/news/*`, so the glob survives generation and renders correctly in STEPS.md.
Learnt from: AlexSkrypnyk
Repo: drevops/behat-steps PR: 634
File: src/Drupal/EntityFixtureTrait.php:41-95
Timestamp: 2026-05-12T07:07:50.984Z
Learning: In drevops/behat-steps, PHPMD is not part of the enforced lint/static-analysis pipeline (the repo relies on PHPStan via phpstan.neon and custom rules in composer.json). During code review, do not raise PHPMD cyclomatic complexity or NPath complexity warnings as review concerns for PHP changes; only consider complexity issues if they are flagged by PHPStan or the repo’s enforced/custom checks.
Learnt from: AlexSkrypnyk
Repo: drevops/behat-steps PR: 634
File: src/Drupal/EntityFixtureTrait.php:41-95
Timestamp: 2026-05-12T07:07:50.984Z
Learning: In this repository (drevops/behat-steps), snake_case is an intentional naming convention for **method parameter names** and **local variables** in PHP (per CLAUDE.md, e.g. `$entity_type`, `$files_path`). During code review, do **not** flag these snake_case identifiers as violations of camelCase naming conventions. Flag naming issues only when they are inconsistent with this established convention.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #643 +/- ##
==========================================
- Coverage 96.93% 96.54% -0.40%
==========================================
Files 43 44 +1
Lines 3068 3355 +287
==========================================
+ Hits 2974 3239 +265
- Misses 94 116 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…prefix method names." This reverts commit 08c7f1d.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/A11yTrait.php`:
- Around line 85-88: The a11ySetupScenario hook currently checks
$scope->getScenario()->hasTag(...) while a11yAutoAssess and a11yFinalizeScenario
check $scope->getFeature()->hasTag(...), causing inconsistent skip behavior;
update a11ySetupScenario to check
$scope->getFeature()->hasTag('behat-steps-skip:A11yTrait') so all three hooks
(a11ySetupScenario, a11yAutoAssess, a11yFinalizeScenario) use the same
feature-level tag check and behave consistently per the guideline.
In `@STEPS.md`:
- Around line 62-65: Add detailed usage docs for A11yTrait: list available tags
(`@axe`, `@axe-warning`, `@axe-critical`, `@axe-serious`, `@axe-moderate`, `@axe-minor`,
`@axe-strict`), explain each tag's effect (automatic mode vs explicit assertion
and threshold configuration), document the skip tag format
(`@behat-steps-skip`:A11yTrait), state the report output path
(.logs/test_results/a11y), and include one or two small example scenarios
demonstrating automatic mode and per-scenario threshold tags; place this content
near the existing A11yTrait summary so users can find it without reading
source/tests.
In `@tests/behat/features/a_11y.feature`:
- Around line 31-35: The inner PyString blocks in the `@trait`:A11yTrait scenarios
use triple double quotes; change those inner blocks to use triple single quotes
instead (replace """ with ''' in the nested scenario step blocks such as the
block containing Given I visit "/sites/default/files/a11y_violations.html" /
Then the current page should pass accessibility checks and the other similar
blocks), ensuring all `@javascript-tagged` scenario inner PyStrings follow the
BehatCliContext convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a61f9e4e-72c9-403b-aae6-6dd9785af0fa
📒 Files selected for processing (8)
README.mdSTEPS.mdsrc/A11yTrait.phptests/behat/bootstrap/FeatureContext.phptests/behat/features/a_11y.featuretests/behat/fixtures/a11y_clean.htmltests/behat/fixtures/a11y_clean2.htmltests/behat/fixtures/a11y_violations.html
…kinlint filename.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/A11yTrait.php (1)
149-155: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding error handling for report file operations.
If
mkdirorfile_put_contentsfails (permissions, disk full, path issues), reports are silently lost with no indication to the user. While the gate enforcement still works correctly, losing reports in CI could be confusing.🛡️ Optional: Add basic error checking
$dir = $this->getA11yReportDir(); if (!is_dir($dir)) { - mkdir($dir, 0777, TRUE); + if (!`@mkdir`($dir, 0777, TRUE) && !is_dir($dir)) { + fwrite(STDERR, sprintf("[a11y] Warning: Could not create report directory %s\n", $dir)); + return; + } } $slug = $this->a11ySlug($this->a11yFeatureName) . '__' . $this->a11ySlug($this->a11yScenarioName); - file_put_contents($dir . '/' . $slug . '.html', $this->a11yRenderHtml()); - file_put_contents($dir . '/junit-' . $slug . '.xml', $this->a11yRenderJunit()); + if (file_put_contents($dir . '/' . $slug . '.html', $this->a11yRenderHtml()) === FALSE) { + fwrite(STDERR, sprintf("[a11y] Warning: Could not write HTML report to %s\n", $dir)); + } + if (file_put_contents($dir . '/junit-' . $slug . '.xml', $this->a11yRenderJunit()) === FALSE) { + fwrite(STDERR, sprintf("[a11y] Warning: Could not write JUnit report to %s\n", $dir)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/A11yTrait.php` around lines 149 - 155, Add basic error handling around directory creation and file writes in A11yTrait: check the return value of mkdir($dir, 0777, TRUE) and each file_put_contents call (for $dir.'/'.$slug.'.html' and $dir.'/junit-'.$slug.'.xml'); on failure, log a clear error (or throw an exception) including the target path and errno/message, and ensure you still surface the problem to the caller (e.g., return false or rethrow) so CI doesn’t silently lose reports; use the existing methods a11yRenderHtml(), a11yRenderJunit(), getA11yReportDir(), and a11ySlug(...) to build the paths and include those identifiers in the log/exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/A11yTrait.php`:
- Around line 149-155: Add basic error handling around directory creation and
file writes in A11yTrait: check the return value of mkdir($dir, 0777, TRUE) and
each file_put_contents call (for $dir.'/'.$slug.'.html' and
$dir.'/junit-'.$slug.'.xml'); on failure, log a clear error (or throw an
exception) including the target path and errno/message, and ensure you still
surface the problem to the caller (e.g., return false or rethrow) so CI doesn’t
silently lose reports; use the existing methods a11yRenderHtml(),
a11yRenderJunit(), getA11yReportDir(), and a11ySlug(...) to build the paths and
include those identifiers in the log/exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5ed4cc42-5536-434e-b82e-326d5078a175
📒 Files selected for processing (5)
STEPS.mddocs.phpphpstan.neonsrc/A11yTrait.phptests/behat/features/a11y.feature
💤 Files with no reviewable changes (1)
- tests/behat/features/a11y.feature
|
Actionable comments posted: 0 |
AlexSkrypnyk
changed the title
…lug', IMPACT constants, and explicit normalize remap.
AlexSkrypnyk
changed the title
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 #642
Summary
Adds
AccessibilityTraitto the behat-steps library: a tool-agnostic accessibility-testing trait for Behat scenarios. The trait owns scenario lifecycle, tag-driven gate semantics, and report writing; the actual engine (default: axe-core via CDN) is plugged in via two overridable methods -accessibilityRunEngine()runs the engine and returns raw results,accessibilityNormalizeResults()remaps the raw output into a canonical shape the rest of the trait expects. Reports land as one HTML and one JUnit XML per scenario, with one section per visited URL.Changes
New trait -
src/AccessibilityTrait.phpThen the current page should pass accessibility checksandThen the current page should pass accessibility checks for tags :rules.@accessibility,@accessibility-critical,@accessibility-serious,@accessibility-moderate,@accessibility-minor,@accessibility-any,@accessibility-warning, or@accessibility-strictfor per-scenario gate configuration. The trait runs the engine after every step that changes the URL and aggregates findings.accessibilityRunEngine($rules)runs the engine and returns raw output;accessibilityNormalizeResults($raw)maps it into the canonical shape (violations[],incomplete[],passes[]). The default normalize uses aswitchover the engine's impact string and assigns one of the four publicIMPACT_*constants (IMPACT_CRITICAL,IMPACT_SERIOUS,IMPACT_MODERATE,IMPACT_MINOR) the gate logic compares against. Consumers wiring a different engine override both methods.accessibility*method.accessibilityRenderHtmlPage($sections)for the page wrapper (override to brand the page) andaccessibilityRenderHtmlSections()for the per-URL section logic.@behat-steps-skip:AccessibilityTrait.Shared helper -
src/HelperTrait.phphelperSlug(string $value): stringthat lowercases, collapses non-alphanumeric runs to-, trims surrounding hyphens, and falls back tountitled. Used byAccessibilityTraitto build the per-scenario report filename and available to any other trait that needs filesystem-safe slugs.Tests -
tests/behat/features/accessibility.feature@accessibility-warningnever fails on a broken page, plus three@trait:AccessibilityTraitsubprocess scenarios that verify the broken page fails the explicit step, fails auto mode with the expected gate message, and that@accessibility-criticalis honoured.Fixture HTML pages
tests/behat/fixtures/accessibility_clean.html,accessibility_clean2.html,accessibility_violations.html- WCAG-clean pages plus a deliberately inaccessible one (missingalt, empty<button>, empty<a>).Unit tests -
tests/phpunit/src/HelperTraitTest.phphelperSlug()covering lowercase passthrough, whitespace collapse, mixed case, punctuation, digits, leading/trailing hyphen stripping, theuntitledfallback for empty / whitespace-only / punctuation-only / unicode-only inputs, and thea11ydigit-boundary case.Tooling adjustments unrelated to the trait itself
phpstan.neon- newignoreErrorsentry forfunction.alreadyNarrowedTypeagainstConfigOverrideTrait::$restHeaders. PHPStan 2.2.0 (released the same day as this PR) introduced aConstantConditionInTraitCollector(PRphpstan/phpstan-src#5309) that emits the error from a different rule than the original, so the existing inline@phpstan-ignorecomments at the call site no longer suppress it.tests/behat/fixtures_drupal/d10/composer.json- pinnedphpunit/php-code-coverageto^9.2.31, the first 9.x with explicit?self $parent = nullinAbstractNode::__construct(required by PHP 8.4).tests/behat/fixtures_drupal/d11/composer.json- pinnedphpunit/php-code-coverageto^11.0.6for the same reason in the PHPUnit 11 line.auditblock (abandoned: ignore,block-insecure: false, all severities ignored) so the disposable Drupal fixture builds are not blocked by unrelated upstream advisories on Drupal core releases.Before / After
Engine plug points: