Repository navigation
Migrate KcOidcFirstBrokerLoginDetectExistingUserTest to tests/base - #52787
Merged
shawkins merged 3 commits intoOct 2, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Migrates the first-broker-login “detect existing user” test to the tests/base infrastructure and introduces reusable broker test scaffolding for programmatic realm/flow setup.
Changes:
- Migrated
KcOidcFirstBrokerLoginDetectExistingUserTestfrom the Arquillian-style setup to the newtests/basebroker infrastructure. - Added
BrokerConfiguration-driven base test classes and first-broker-login flow mutators for broker scenarios. - Added
LoginPage.login()helpers to simplify broker UI login flows.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginDetectExistingUserTest.java | Migrates the existing test to JUnit 5 + new broker base classes and UI helpers |
| tests/base/src/test/java/org/keycloak/tests/broker/KcOidcBrokerConfiguration.java | Adds OIDC broker realm/client/idp configuration for the new broker test base |
| tests/base/src/test/java/org/keycloak/tests/broker/BrokerTestTools.java | Adds shared URL/config helpers and selenium wait utilities |
| tests/base/src/test/java/org/keycloak/tests/broker/BrokerTestConstants.java | Adds shared constants for broker tests |
| tests/base/src/test/java/org/keycloak/tests/broker/BrokerServerConfig.java | Adds server dependency config for running broker tests |
| tests/base/src/test/java/org/keycloak/tests/broker/BrokerFirstLoginFlowMutators.java | Adds helpers to mutate first-broker-login flow behavior during tests |
| tests/base/src/test/java/org/keycloak/tests/broker/BrokerConfiguration.java | Defines the configuration contract for broker test realm/client/idp setup |
| tests/base/src/test/java/org/keycloak/tests/broker/AbstractInitializedBaseBrokerTest.java | Adds broker test base that initializes realms, user, idp, and endpoints |
| tests/base/src/test/java/org/keycloak/tests/broker/AbstractBaseBrokerTest.java | Adds core broker test base utilities (realm import/cleanup, login flows) |
| test-framework/ui/src/main/java/org/keycloak/testframework/ui/page/LoginPage.java | Adds login() convenience methods for username+password and password-only flows |
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Provider endpoint initialization ignores the configured provider client ID, breaking reusable configurations with non-default client IDs.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/base/src/test/java/org/keycloak/tests/broker/AbstractInitializedBaseBrokerTest.java:105
- This hard-codes
brokerappeven thoughBrokerConfiguration#getIDPClientIdInProviderRealm()defines the provider-side client ID andcreateProviderClients()uses it. Any configuration with a custom provider client ID will fail setup here before the broker flow runs; resolve and report the configured ID instead.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
michalvavrik
suggested changes
Sep 24, 2026
Add BrokerConfiguration-based broker test infrastructure for first-broker-login scenarios that still need programmatic realm setup, and migrate the detect-existing- user OIDC broker tests as the first batch of the broker package migration for keycloak#50755. Supersedes the monolithic keycloak#51881 approach with a focused follow-up aligned to the incremental broker migrations already on main. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Initialize identity-provider and protocol-mapper config maps before use, fail fast when the provider broker client is missing, use ThreadLocal for broker server-root state, and tidy BrokerTestConstants modifier order. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
edewit
force-pushed
the
issue-50755-migrate-broker-first-login
branch
from
September 24, 2026 13:59
0710b3e to
9d701f7
Compare
Drop the parallel BrokerConfiguration-based test hierarchy and unused helpers, addressing review feedback about duplication and dead code. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
edewit
force-pushed
the
issue-50755-migrate-broker-first-login
branch
from
September 24, 2026 14:28
9d701f7 to
7895c8c
Compare
edewit
force-pushed
the
issue-50755-migrate-broker-first-login
branch
from
October 1, 2026 14:20
7895c8c to
d5291b8
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
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
KcOidcFirstBrokerLoginDetectExistingUserTestfrom arquillian totests/baseas the first focused batch of broker package migration for Migrate broker package to the new test framework #50755AbstractKcOidcBrokerTest/ ManagedRealm broker infrastructure instead of a parallelBrokerConfigurationhierarchyThis supersedes the monolithic #51881 approach, which conflicted with the incremental broker migrations already merged to
main(#51859, #52372, #52379, #52376, #52382, #52387, etc.).Test plan
./mvnw -pl tests/base -Dtest=KcOidcFirstBrokerLoginDetectExistingUserTest testPart of #50755