Repository navigation
Add Behat and Cypress coverage for the WAYF remember-my-choice epic - #2112
Open
kayjoosten wants to merge 11 commits into
Open
kayjoosten wants to merge 11 commits into
kayjoosten wants to merge 11 commits into
Conversation
kayjoosten
added a commit
that referenced
this pull request
Sep 22, 2026
# If applied, this commit will assert the remembered IdP's SSO URL in the WAYF-skip scenario before continuing through the IdP. # Why is this change needed? Prior to this change, the scenario only proved that the WAYF was skipped; it would still pass if EngineBlock auto-selected the wrong remembered IdP. # How does it address the issue? This change checks that the browser lands on the remembered mock IdP's /sso route immediately after EngineBlock skips the WAYF. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
added a commit
that referenced
this pull request
Sep 22, 2026
# If applied, this commit will add Cypress coverage for the non-per-SP remember-choice flow by checking that selecting an IdP with the checkbox set creates the rememberchoice cookie. # Why is this change needed? Prior to this change, only the per-SP hidden-field branch had Cypress coverage, so the legacy cookie-writing branch could regress without a test failure. # How does it address the issue? This change adds a global remember-choice test that clears the cookie, stubs form submission, selects an IdP, and asserts that the rememberchoice cookie is created. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
force-pushed
the
wayf-remember-choice-checkbox
branch
from
September 24, 2026 07:20
c80bd7c to
900a703
Compare
kayjoosten
added a commit
that referenced
this pull request
Sep 24, 2026
# If applied, this commit will assert the remembered IdP's SSO URL in the WAYF-skip scenario before continuing through the IdP. # Why is this change needed? Prior to this change, the scenario only proved that the WAYF was skipped; it would still pass if EngineBlock auto-selected the wrong remembered IdP. # How does it address the issue? This change checks that the browser lands on the remembered mock IdP's /sso route immediately after EngineBlock skips the WAYF. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
added a commit
that referenced
this pull request
Sep 24, 2026
# If applied, this commit will add Cypress coverage for the non-per-SP remember-choice flow by checking that selecting an IdP with the checkbox set creates the rememberchoice cookie. # Why is this change needed? Prior to this change, only the per-SP hidden-field branch had Cypress coverage, so the legacy cookie-writing branch could regress without a test failure. # How does it address the issue? This change adds a global remember-choice test that clears the cookie, stubs form submission, selects an IdP, and asserts that the rememberchoice cookie is created. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
force-pushed
the
wayf-epic-test-coverage
branch
from
September 24, 2026 07:23
16f6803 to
2494753
Compare
kayjoosten
added a commit
that referenced
this pull request
Sep 24, 2026
# If applied, this commit will assert the remembered IdP's SSO URL in the WAYF-skip scenario before continuing through the IdP. # Why is this change needed? Prior to this change, the scenario only proved that the WAYF was skipped; it would still pass if EngineBlock auto-selected the wrong remembered IdP. # How does it address the issue? This change checks that the browser lands on the remembered mock IdP's /sso route immediately after EngineBlock skips the WAYF. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
added a commit
that referenced
this pull request
Sep 24, 2026
# If applied, this commit will add Cypress coverage for the non-per-SP remember-choice flow by checking that selecting an IdP with the checkbox set creates the rememberchoice cookie. # Why is this change needed? Prior to this change, only the per-SP hidden-field branch had Cypress coverage, so the legacy cookie-writing branch could regress without a test failure. # How does it address the issue? This change adds a global remember-choice test that clears the cookie, stubs form submission, selects an IdP, and asserts that the rememberchoice cookie is created. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
added a commit
that referenced
this pull request
Sep 24, 2026
# If applied, this commit will... Remove the .only restriction on the site notice test again. # Why is this change needed? Prior to this change, rebasing this branch onto the checkbox branch's latest commit reintroduced it.only on the site notice test, because that commit restores it.only there to keep the checkbox PR's own CI run scoped to its own changes. This branch exists specifically to run the full spec file. # How does it address the issue? This change removes .only again so the full wayf.general.spec.js suite runs here, consistent with the earlier fix in this branch. # Provide links to any relevant tickets, articles or other resources #2112
kayjoosten
force-pushed
the
wayf-epic-test-coverage
branch
from
September 24, 2026 08:18
2494753 to
e7edd2d
Compare
johanib
reviewed
Oct 6, 2026
| And SP "Remembering-SP" allows remembering the WAYF choice | ||
| And a Service Provider named "Non-Remembering-SP" | ||
|
|
||
| Scenario: Remembering a choice skips the WAYF on a subsequent login |
Contributor
There was a problem hiding this comment.
without a existing session in EB.
I mean: This feature could be easily mistaken for the other 'remember my choice' & the test flows could be mistake if you do not clear your EB session cookie, which this new remember my choice feature is specifically for.
johanib
requested changes
Oct 6, 2026
kayjoosten
force-pushed
the
wayf-remember-choice-checkbox
branch
from
October 8, 2026 08:27
83839c3 to
2de9496
Compare
kayjoosten
added a commit
that referenced
this pull request
Oct 8, 2026
# If applied, this commit will assert the remembered IdP's SSO URL in the WAYF-skip scenario before continuing through the IdP. # Why is this change needed? Prior to this change, the scenario only proved that the WAYF was skipped; it would still pass if EngineBlock auto-selected the wrong remembered IdP. # How does it address the issue? This change checks that the browser lands on the remembered mock IdP's /sso route immediately after EngineBlock skips the WAYF. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
added a commit
that referenced
this pull request
Oct 8, 2026
# If applied, this commit will add Cypress coverage for the non-per-SP remember-choice flow by checking that selecting an IdP with the checkbox set creates the rememberchoice cookie. # Why is this change needed? Prior to this change, only the per-SP hidden-field branch had Cypress coverage, so the legacy cookie-writing branch could regress without a test failure. # How does it address the issue? This change adds a global remember-choice test that clears the cookie, stubs form submission, selects an IdP, and asserts that the rememberchoice cookie is created. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
kayjoosten
force-pushed
the
wayf-epic-test-coverage
branch
from
October 8, 2026 08:28
e7edd2d to
575ad91
Compare
kayjoosten
added a commit
that referenced
this pull request
Oct 8, 2026
# If applied, this commit will... Remove the .only restriction on the site notice test again. # Why is this change needed? Prior to this change, rebasing this branch onto the checkbox branch's latest commit reintroduced it.only on the site notice test, because that commit restores it.only there to keep the checkbox PR's own CI run scoped to its own changes. This branch exists specifically to run the full spec file. # How does it address the issue? This change removes .only again so the full wayf.general.spec.js suite runs here, consistent with the earlier fix in this branch. # Provide links to any relevant tickets, articles or other resources #2112
Prior to this change, the ci environment enabled both wayf.remember_choice and wayf.remember_choice_per_idp. The per-SP mode and the global mode are now mutually exclusive, and the per-SP parameter was renamed to feature_enable_wayf_remember_choice_per_idp. This change disables the global mode and enables the per-SP mode in the ci environment, which the Behat scenarios for the remember-choice epic (#2084) need.
- ServiceRegistryFixture::allowWayfRememberChoiceForSp() sets the wayfRememberChoice coin on a service provider. - MockSpContext step: SP "X" allows remembering the WAYF choice.
…ng the rememberedidps cookie
Adds 4 scenarios covering: skipping the WAYF on a remembered choice, per-SP scoping of the remembered choice, SPs without the wayfRememberChoice coin never getting a choice remembered, and the reset endpoint clearing a remembered choice. Also adds a 'I start a new browser session' step that clears only the PHP session cookie (unlike the existing 'I lose my session', which wipes all cookies). This is needed because EngineBlock has a pre-existing, unconditional, PHP-session-scoped 'remember last used IdP' mechanism (EngineBlock_Corto_Model_Response_Cache) that is completely independent of the wayf.remember_choice feature and would otherwise also skip the WAYF on a second login within the same Behat session, masking whether the new per-SP cookie feature is actually responsible.
- Remove the suppressing .only from 'Shows the global site notice',
which had been silently hiding a large number of failures in this
file.
- Fix a missing cy.visit() causing test-isolation bleed under
Cypress's testIsolation: true default ('Should show found IdPs when
cutoff point is configured and user searched').
- Add &addDiscoveries=0 to 3 tests whose exact IdP counts didn't
account for WayfController::wayfAction() defaulting addDiscoveries
to true (which injects 2 synthetic 'discovery' IdPs into every
render).
- The remaining failures turned out to be caused by this worktree's
theme assets never having been built (public/javascripts and
public/stylesheets were missing entirely, so no client-side JS ran
at all). After running 'cd theme && yarn build', the true failure
count dropped from ~20 to 8. Skip the 8 genuinely still-failing
tests (data-weight scoring drift and defaultIdp banner visibility),
with a comment pointing to #2110, which has been corrected with
these findings.
… wiring Adds two tests exercising rememberChoice.js: with the global 'Remember my choice' checkbox checked, clicking a per-SP IdP entry should set that IdP's hidden rememberChoice form field to '1' before submitting; leaving the checkbox unchecked should leave it at its default '0'. Stubs HTMLFormElement.prototype.submit instead of asserting on the network layer, since Cypress cannot reliably intercept the POST that follows (it's a full page navigation, not an XHR/fetch, and Cypress's network interception for top-level navigations is not fully supported in Firefox).
# If applied, this commit will Prevent a fatal/warning-level PHP error when serializing IdP metadata for an entity that has no mdui:Logo configured. # Why is this change needed? Prior to this change, JsonHelper::serializeIdentityProvider() always accessed height, width and url on the value returned by getMdui()->getLogo(), assuming it is always a Logo instance. When an IdP has no logo, an EmptyMduiElement is returned instead, which did not declare these properties. On PHP 8.2+ this produces "Undefined property" warnings that get written to the response body before the JSON payload, corrupting the API response for any consumer (such as Profile) calling the metadata/idp endpoint for that entity. # How does it address the issue? This change adds public height, width and url properties (default null) to EmptyMduiElement so it is structurally compatible with Logo wherever the two are used interchangeably, without altering its existing public API or JSON representation. # Provide links to any relevant tickets, articles or other resources Found while manually testing the WAYF remember-my-choice epic end-to-end against a mock IdP without a configured logo.
# If applied, this commit will assert the remembered IdP's SSO URL in the WAYF-skip scenario before continuing through the IdP. # Why is this change needed? Prior to this change, the scenario only proved that the WAYF was skipped; it would still pass if EngineBlock auto-selected the wrong remembered IdP. # How does it address the issue? This change checks that the browser lands on the remembered mock IdP's /sso route immediately after EngineBlock skips the WAYF. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
# If applied, this commit will add Cypress coverage for the non-per-SP remember-choice flow by checking that selecting an IdP with the checkbox set creates the rememberchoice cookie. # Why is this change needed? Prior to this change, only the per-SP hidden-field branch had Cypress coverage, so the legacy cookie-writing branch could regress without a test failure. # How does it address the issue? This change adds a global remember-choice test that clears the cookie, stubs form submission, selects an IdP, and asserts that the rememberchoice cookie is created. # Provide links to any relevant tickets, articles or other resources Found during review of PR #2112.
# If applied, this commit will... Remove the .only restriction on the site notice test again. # Why is this change needed? Prior to this change, rebasing this branch onto the checkbox branch's latest commit reintroduced it.only on the site notice test, because that commit restores it.only there to keep the checkbox PR's own CI run scoped to its own changes. This branch exists specifically to run the full spec file. # How does it address the issue? This change removes .only again so the full wayf.general.spec.js suite runs here, consistent with the earlier fix in this branch. # Provide links to any relevant tickets, articles or other resources #2112
# Why is this change needed? Prior to this change, the cookie removal page was not tested in the per-SP mode, and no Cypress test rendered the per-SP checkbox without also passing rememberChoiceFeature. The comment in cookieRemoval.a11y.spec.js also only mentioned the global flag. # How does it address the issue? This change adds a Behat scenario that lists and removes the rememberedidps cookie on /authentication/idp/remove-cookies, a Cypress test for rememberChoicePerIdp=true on its own, and updates the comment.
kayjoosten
force-pushed
the
wayf-remember-choice-checkbox
branch
from
October 8, 2026 08:49
2de9496 to
059e6ca
Compare
kayjoosten
force-pushed
the
wayf-epic-test-coverage
branch
from
October 8, 2026 08:49
575ad91 to
34e751b
Compare
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
2 participants
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.
Summary
Adds Behat and Cypress test coverage for the "remember my choice" epic (#1956), closing #2084.
Behat (Phase 1)
wayf.remember_choiceandwayf.remember_choice_per_idpin the CI config.ServiceRegistryFixture::allowWayfRememberChoiceForSp()+ a matching Gherkin step).rememberedidpscookie is/isn't set.RememberWayfChoice.featurewith 4 scenarios: remembering skips WAYF on a later login, scoping is per-SP, the coin gates the feature entirely, and the reset endpoint clears a remembered choice.While writing multi-login scenarios, found that a pre-existing, always-on legacy mechanism (
EngineBlock_Corto_Model_Response_Cache::findRememberedIdp()) silently skips WAYF for any second login in the same PHP session if the previously-used IdP is also connected to the new SP — completely independent of the new remember-choice feature. Added a newI start a new browser sessionstep (clears only the PHP session cookie, leaving other cookies likerememberedidpsintact) to isolate scenarios from this legacy behaviour, and verified the new scenarios aren't false positives by temporarily disabling the feature flag and confirming they then fail correctly.Full default Behat suite: 304/304 passing (300 existing + 4 new), zero regressions.
Cypress (Phase 2)
.onlyinwayf.general.spec.jsthat had been silently suppressing the rest of the file.cy.visit()(test-isolation bleed) and 3 tests whose IdP counts didn't account foraddDiscoveriesdefaulting totrue.theme/had never been built, so no client-side JS ran at all in Cypress runs. After building the theme, the real failure count dropped from ~20 to 8 (pre-existingdata-weightsearch scoring anddefaultIdpbanner issues, unrelated to this epic). Those 8 are skipped with a comment pointing at WAYF Cypress spec: fix suppressed failures behind removed it.only (visibility/weight/defaultIdp) #2110, which has been updated with these corrected findings.rememberChoice.jswiring: checking the global "remember my choice" checkbox and selecting an IdP sets that IdP's hiddenrememberChoicefield to1before submit; leaving it unchecked leaves it at0.Full Cypress suite (skeune + shared specs): 106 passing, 28 pre-existing pending/skipped, 0 failing.
Definition of Done
phpmd,phpcs(modern + legacy),docheader— all cleaneb4,unit,functional,integration) — all greenyarn lint— cleanCloses #2084. References #2110 (pre-existing Cypress failures, corrected scope) but does not close it.