Repository navigation
fix: updating the test extension logic - #48759
Conversation
de1f157 to
139a399
Compare
Unreported flaky test detectedIf 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#loginDifferentUserAfterDisabledUserThrownOutKeycloak CI - Forms IT (firefox) |
0d0fb84 to
e080679
Compare
|
@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. |
There was a problem hiding this comment.
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")) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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="; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@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. |
I think these changes are representative and the next intended state. There is only a single raw distribution ever created. It seemed appropriate to use the QuarkusTestResourceLifecycleManager for this, but that also requires adding our own support for that in CLITestExtension. Maybe the quarkus team could make that easier, or we could forgo using the QuarkusTestResourceLifecycleManager and mimic the support directly in the CLITestExtension. We create initial backups and a pre-initialized h2 at install time, then copy those in later as needed. There is just a single reinstall mode "DEFAULT" which means reset the augmentation inbetween classes, and reset the reset of the state inbetween each test. This roughly replaces the previous BEFORE_ALL mode. NEVER and BEFORE_TEST are not something we should strive to keep - NEVER makes running an individual method problematic, and BEFORE_TEST is far too expensive. Should be ready for review. In general these changes are a good improvement on the "slow" tests, while not impacting the "fast" tests. |
|
@vmuzikar updated based upon the review comments after this and the aligment with the pr from @michalvavrik we can explore the other topics from the quick test issue - further optimization around reaugmentation beyond just dryrun, and AOT (which could be utilized via the process being used to create the pre-initialized h2 db). Other avenues:
|
also optimized ClusterConfigDistTest closes: keycloak#48754 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
Unreported flaky test detectedIf 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#loginDifferentUserAfterDisabledUserThrownOutKeycloak CI - Forms IT (firefox) |
|
@pruivo @vmuzikar @michalvavrik here's a preview of using aot compilation: https://github.com/shawkins/keycloak/tree/iss48754-b built on the changes in this pr. "Slow" tests when from 33 minutes to 22. "Fast" tests went from 36 to 29. The help command dist test is failing, but I believe that is primarily due to the additional message in the output about using the additional java options. It would probably be a good idea to move the help command dist tests out of the integration tests as they do not need a running server either. |
also optimized ClusterConfigDistTest closes: keycloak#48754 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
also minor changes to the cluster config dist test to prevent reaugmentations
closes: #48754