test(e2e): both URL prefixes now resolve, so invert the positive control - #1807
Merged
Conversation
#1805 made routerBase() derive the router base from window.location.pathname instead of generateUrl(), so a deep link resolves under BOTH /apps/integriq and /index.php/apps/integriq. That broke this file's third test, which asserted the OTHER prefix was redirected to the app root. The test was right when it was written and the fix is what invalidated it. Its own closing comment said so: 'if this assertion ever fails, the prefix distinction has stopped mattering and appRoot.ts can be simplified — do that rather than deleting this test.' So it is inverted, not deleted. It now walks both prefixes and requires each to KEEP its route and not land on the Dashboard. The guard keeps a job: it no longer proves the resolver picks the right prefix, it proves the silently swallowed deep link cannot come back — which is the failure this app already paid for once, with 36 specs in manifest-pages.spec.ts passing against the Dashboard. appRoot.ts is left alone. It is no longer load-bearing for correctness, since either prefix now works, but it is still correct and still used by six spec files, and simplifying it is a separate change from proving the bug is gone.
Contributor
Quality Report — ConductionNL/integriq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 141/141 | |||
| npm | ✅ | ✅ 537/537 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-01 19:43 UTC
Download the full PDF report from the workflow artifacts.
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
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.
#1805 made
routerBase()derive the router base fromwindow.location.pathnameinstead ofgenerateUrl(), so a deep link now resolves under both/apps/integriqand/index.php/apps/integriq. That turned this file's third test red on the push run:The test was correct when it was written, and the fix is what invalidated it. It was a POSITIVE CONTROL: it proved the two prefixes were distinguishable, so that tests 1 and 2 could not pass vacuously on an instance where the distinction did not exist. With the base now derived from the pathname, the distinction genuinely no longer exists, by design.
The test says what to do in exactly this case, and I followed it rather than reaching for a delete:
So the assertion is inverted, not removed. It walks both prefixes and requires each to keep its route and not land on the Dashboard. The guard keeps a real job: it no longer proves the resolver picks the right prefix, it proves the silently swallowed deep link cannot come back. That is the failure this app already paid for once, with 36 specs in
manifest-pages.spec.tspassing against the Dashboard while claiming to test other pages.appRoot.tsis left alone on purpose. It is no longer load-bearing for correctness, since either prefix now works, but it is still correct and still used by six spec files. Simplifying it is a separate change from proving the bug is gone, and I would rather this PR be reviewable as the one thing it is.Verified locally: prettier clean, eslint clean, and the suite still collects 194 tests across 36 files. The E2E job runs on the push to development rather than on this PR, so the real verdict lands after merge and I will report it there.