[#658] Added suite-level accessibility aggregate report.#661
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:
Walkthrough
ChangesSuite-level accessibility aggregate reporting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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: 4
🤖 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/AccessibilityTrait.php`:
- Around line 1061-1067: The `accessibilityAggregatePages()` method uses an `if
(!isset($pages[$url]))` guard that prevents processing of duplicate URLs,
causing subsequent results for the same URL to be ignored. Refactor this logic
to merge results when a URL is encountered multiple times: for duplicate URLs,
append violations to the existing array, and accumulate the counts for
incomplete and passes fields instead of only initializing them on first
occurrence. This ensures all accessibility findings for a given URL are properly
aggregated regardless of the order in which they appear in the results.
- Around line 1245-1253: The docs link in the meta section is always rendered
even when helpUrl is empty, resulting in broken href="" links. Modify the string
formatting to conditionally include the anchor tag only when the helpUrl from
the rule array is not empty. If helpUrl is empty, either omit the anchor tag
entirely or render plain text instead. This check should be applied to the
htmlspecialchars((string) $rule['helpUrl'], ENT_QUOTES) portion of the sprintf
call to prevent generating invalid links in the aggregate report.
In `@STEPS.md`:
- Around line 81-86: The blockquote lines in STEPS.md contain multiple spaces
after the > character, which violates the MD027 markdownlint rule that requires
exactly one space after the blockquote marker. Remove the extra space after each
> symbol in the blockquote lines (the lines containing <br/><br/> and the
Reporting description) so that each line has only a single space between the >
and the content.
In `@tests/behat/features/accessibility.feature`:
- Around line 94-99: The PyString (multi-line string) in the
accessibility.feature file uses triple double quotes (""") but according to
coding guidelines for `@trait` scenarios, it should use triple single quotes (''')
instead. Locate the PyString block containing the "Given I visit" and "When I
visit" steps, and replace both the opening delimiter on line 94 and closing
delimiter on line 99 from """ to '''.
🪄 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: e5df17ad-5eba-42a7-9e1d-bf612fb724e4
📒 Files selected for processing (4)
STEPS.mdsrc/AccessibilityTrait.phptests/behat/features/accessibility.featuretests/phpunit/src/AccessibilityTraitTest.php
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #661 +/- ##
==========================================
+ Coverage 96.60% 96.80% +0.19%
==========================================
Files 46 46
Lines 3537 3757 +220
==========================================
+ Hits 3417 3637 +220
Misses 120 120 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/phpunit/src/AccessibilityTraitTest.php (1)
381-384: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExpand blank-URL fixture to include
data:,to match the trait contract.The aggregate blank-URL contract includes
data:,, but this fixture currently only exercisesabout:blank. Adding adata:,result here (or a dedicated case) would close that regression gap.Suggested test fixture tweak
[ 'url' => 'about:blank', 'rules' => 'wcag2a', 'result' => ['violations' => [], 'incomplete' => [], 'passes' => []], ], + [ + 'url' => 'data:,', + 'rules' => 'wcag2a', + 'result' => ['violations' => [], 'incomplete' => [], 'passes' => []], + ],🤖 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 `@tests/phpunit/src/AccessibilityTraitTest.php` around lines 381 - 384, The blank-URL test fixture in AccessibilityTraitTest.php is incomplete and only covers the `about:blank` case. Add an additional test fixture case for the `data:,` URL to match the full blank-URL contract supported by the trait. The new case should follow the same structure as the existing `about:blank` fixture with the same expected results (empty violations, incomplete, and passes arrays) but with the URL set to `data:,` instead.
🤖 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 `@tests/phpunit/src/AccessibilityTraitTest.php`:
- Around line 381-384: The blank-URL test fixture in AccessibilityTraitTest.php
is incomplete and only covers the `about:blank` case. Add an additional test
fixture case for the `data:,` URL to match the full blank-URL contract supported
by the trait. The new case should follow the same structure as the existing
`about:blank` fixture with the same expected results (empty violations,
incomplete, and passes arrays) but with the URL set to `data:,` instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c02e2f8a-a591-479b-8636-76387c2f510e
📒 Files selected for processing (2)
src/AccessibilityTrait.phptests/phpunit/src/AccessibilityTraitTest.php
AlexSkrypnyk
added
the
Needs review
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/AccessibilityTrait.php`:
- Line 1158: The current implementation in the implode statement at line 1158
uses only scenario names as keys via array_keys($page['scenarios']), which
causes multiple features with identical scenario titles to collapse into a
single entry. Instead of directly using array_keys(), create a composite key
that combines feature information with the scenario identifier before imploding
to preserve the provenance of where each feature originated. This ensures that
the same scenario title from different features is tracked as separate entries
in the scenarios list.
🪄 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: 372923cc-3a76-4eeb-a38c-c94516fd8726
📒 Files selected for processing (2)
src/AccessibilityTrait.phptests/phpunit/src/AccessibilityTraitTest.php
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 #658
Summary
Adds a single suite-level accessibility report to
AccessibilityTrait, written once after the whole Behat suite via a static#[AfterSuite]hook. Each scenario still writes its own per-scenario HTML and JUnit reports; the new aggregate collects and de-duplicates findings across every scenario into one cross-page overview. It is written toaccessibility_report_<YYYYMMDD_HHMMSS>.htmlin the configured report directory - one timestamped file per run, so a run never overwrites a previous one. The work reuses the report directory and URL-formatting helpers from #657 and #659, so consumer overrides of those are honoured.Changes
Accumulation
$accessibilityAggregate(process-global) collects each scenario's results;$accessibilityAggregateReportDircaptures the resolved report directory from the instance phase for the static renderer.accessibilityAggregateCapture()records each scenario's formatted results (URLs passed throughaccessibilityFormatUrl()) inaccessibilityFinalizeScenario(), before the gate throw, so a scenario that fails the gate is still represented.accessibilityAggregateReset()(#[BeforeSuite]) clears the statics so a second suite in the same process starts clean.Calculation - one method per concern
accessibilityAggregatePages()de-duplicates assessed pages by URL, keyed byfeature > scenarioso same-titled scenarios keep their provenance.accessibilityAggregateRollup()rolls violations up by rule, tallies by impact, and sorts by severity then affected-element count.accessibilityAggregateFindings()flattens findings and stringifies element targets.accessibilityAggregateData()is the single calculation entry point: it calls the above and assembles one render-ready data array (generated,page_count,scenario_count,total_violations,totals,pages,rules,scenarios).Rendering - one method
accessibilityRenderAggregate(array $data)is the single rendering entry point. It receives the prepared data array and produces the entire HTML document: summary cards, a de-duplicated "Pages assessed" table with per-rule chips, a severity-sorted "Violations by rule" rollup, full per-scenario detail, and the CSS. Override this one method to restyle the whole report without touching any aggregation logic.Reporting and helpers
accessibilityAggregateRender()(#[AfterSuite]) andaccessibilityWriteAggregateReport()resolve the timestamp once - it drives both the filename and the in-page "generated" line - and write the report.accessibilityBlankUrls()(shared, overridable) addsdata:,to the blank-URL skip list, used by both per-step auto-assessment and the aggregate.accessibilityStringifyTarget()is promoted toprotected staticso the static renderer can reuse it.Tests
helpUrl, blank-URL skipping).@trait:AccessibilityTraitintegration scenario asserts the timestampedaccessibility_report_*.htmlis written after a real sub-process suite run, via a new generica file matching "<glob>" should existstep.Before / After
The per-scenario "single page" report machinery is unchanged. This PR adds the suite aggregate and wires it to reuse the same small helpers (
accessibilityFormatUrl(),accessibilityStringifyTarget(),accessibilityBlankUrls()) rather than re-implementing them.Before - only the per-scenario "single page" report exists:
After - the per-scenario report is unchanged; a suite aggregate is added, and both reports reuse the same helpers:
Screenshots