8000 fix: updating the test extension logic by shawkins · Pull Request #48759 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Skip to content
8000

fix: updating the test extension logic - #48759

Merged
vmuzikar merged 1 commit into
keycloak:mainfrom
shawkins:iss48754
May 11, 2026
Merged

vmuzikar merged 1 commit into
keycloak:mainfrom
shawkins:iss48754

Conversation

@shawkins
@shawkins shawkins commented May 6, 2026 •
Copy link
Copy Markdown
Contributor

also minor changes to the cluster config dist test to prevent reaugmentations

closes: #48754

@shawkins shawkins changed the title fix: correcting the test extension logic fix: updating the test extension logic May 6, 2026
@shawkins
shawkins force-pushed the iss48754 branch 2 times, most recently from de1f157 to 139a399 Compare May 6, 2026 18:31
@keycloak-github-bot
Copy link
Copy Markdown

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.forms.LoginTest#loginDifferentUserAfterDisabledUserThrownOut

Keycloak CI - Forms IT (firefox)

org.openqa.selenium.TimeoutException: 
Navigation timed out after 10000 ms
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.17.0-1010-azure', java.version: '25.0.2'
Driver info: org.openqa.selenium.firefox.FirefoxDriver
...

Report flaky test

@keycloak-github-bot keycloak-github-bot Bot left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@shawkins
shawkins force-pushed the iss48754 branch 2 times, most recently from 0d0fb84 to e080679 Compare May 7, 2026 03:08
@shawkins
shawkins commented May 7, 2026
Copy link
Copy Markdown
Contributor Author

@michalvavrik @vmuzikar I took the changes mentioned in the issue a little further by adding in the pre-initialized db reuse mentioned elsewhere. Rather than baking that init into the distribution (which we may not want to do - at least in the default location), I'm having the Raw logic create it. This does mean there will be an initialization with ever non ReInstall.NEVER test class - including DB tests that don't need the h2 database. In any case I'm mostly looking to show what the effect of the optimization is by comparing to a run with that removed as well.

I'm not sure if we really need the ReInstall.NEVER, or the reinstall enum, moving forward. We should probably just create the raw dist once, rather than for every test class.

Other than the minor changes to ClusterConfigDistTest, there's no optimizations yet for reaugmentation.

@michalvavrik michalvavrik left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, test failures are related but that is probably expected.

@@ -110,6 +111,13 @@ private synchronized void runBootstrap(DefaultKeycloakSessionFactory keycloakSes
var startTime = System.nanoTime();

keycloakSessionFactory.init();

// for testing, allow the quarkus application to exit early
if (io.quarkus.runtime.Application.currentApplication() != null && Boolean.getBoolean("init_db_only")) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have secret system property that stops Keycloak from starting, it is all based on hope that noone ever sets init_db_only in production for their own purpose. I think there should be some logging if this happens for the unlikely situation when someone else but our tests set it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be viewed as temporary - we'll refine it based upon #48733 - and yes it can have logging or a more complicated key so that it's not accidentally used.

@@ -61,6 +60,8 @@
@RawDistOnly(reason = "Requires creating JSON file to be available between containers")
public class UpdateCommandDistTest {

private static final String CONFIG_FILE_HASH = "Czh0QKyu1fHcvW0XFuYb/Mdkya0ASlObok+xaRqaP8k=";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will break if the default file hash changes. Also there is a test failure in CI UpdateCommandDistTest.testCacheLocalChange:157 expected: <Czh0QKyu1fHcvW0XFuYb/Mdkya0ASlObok+xaRqaP8k=> but was: <not_found>. Maybe it is expected as this is PR is marked as draft.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a fluke of the raw dist logic that the config file was getting deleted, so I was suprised to see that the test logic was dependent on that. This was the quick attempt at making use the actual state, but yes to be resliant it would need to compute hash, rather than just using a hardcoded one. For now I've switched the logic back to deleting the config file.

@shawkins
shawkins commented May 7, 2026
Copy link
Copy Markdown
Contributor Author

@michalvavrik Thanks for the review. I won't address most of the comments yet as this is still in a rough draft state - I really had only intended to use it for comparison purposes on strategies for reusing the raw distribution more. In particular reusing the pre-initializated database on a per class basis as shown here does is not the correct approach as it's forcing the initialization for a number of test classes in the fast tests that did not previously need it.

My next iteration will be to remove the NEVER mode to see if we can get this done to only a single installation of the raw distribution.

@shawkins
shawkins commented May 7, 2026 •