Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Keycloak 25 #166

Merged
merged 16 commits into from
Jul 16, 2024
Merged

Update to Keycloak 25 #166

merged 16 commits into from
Jul 16, 2024

Conversation

xgp
Copy link
Contributor

@xgp xgp commented Jun 12, 2024

@daniel-frak
Copy link
Owner

daniel-frak commented Jun 15, 2024

Hi! Thanks for the contribution!

For the changes to work, you will also need to update:

  • .env (KEYCLOAK_IMAGE, MAVEN_IMAGE and OPENJDK_IMAGE)
  • maven.yml (the Set up JDK 17 step)
  • release.yml (the Set Up Java step)
  • README.md (the Compatibility history table - the plugin version should be SNAPSHOT)

Here's a link to the Keycloak version upgrade quickstart guide, for future reference:

https://github.com/daniel-frak/keycloak-user-migration/blob/master/CONTRIBUTING.md#quick-guide-to-upgrading-keycloak-compatibility

EDIT:
I added a Quick guide to upgrading Java version to CONTRIBUTING.md with some useful links.

@daniel-frak daniel-frak added the requested changes Waiting for requested changes to be made to the code label Jun 15, 2024
README.md Outdated
@@ -22,7 +22,8 @@ https://codesoapbox.dev/keycloak-user-migration

| Keycloak Version | Version/Commit |
|------------------|----------------------------------------------------------------------------------------------------------------------------------------------------|
| 24.X | [4.0.0](https://github.com/daniel-frak/keycloak-user-migration/releases/tag/4.0.0) |
| 25.X | [5.0.0](https://github.com/daniel-frak/keycloak-user-migration/releases/tag/5.0.0) |
Copy link
Owner

Choose a reason for hiding this comment

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

This version should just be be SNAPSHOT (with no links). The release workflow will change it to the correct tag automatically and that way we won't have dead links.

@daniel-frak
Copy link
Owner

daniel-frak commented Jun 17, 2024

Thanks!

I just noticed that the end-to-end tests are failing, however :/
It seems that the "should migrate user" test cannot find the .pf-c-alert__title element.

Do you feel like you can tackle that as well, or would you like me to help?

@xgp
Copy link
Contributor Author

xgp commented Jun 17, 2024 via email

@daniel-frak
Copy link
Owner

Ok, I'll try to carve out some time near the end of this week to take a look.

@daniel-frak
Copy link
Owner

daniel-frak commented Jun 22, 2024

Ok, this is a bit weird. I cloned your repo locally and ran the E2E tests, and they pass.

Looking at the code, the issue is at this place (docker/e2e/cypress/e2e/migrating_users.cy.js:69):

        cy.get('#kc-forgot-pw-switch').then(($checkbox) => {
            if (!$checkbox.prop('checked')) {
                cy.wrap($checkbox).check({ force: true });
                cy.get('.pf-c-alert__title').should('contain', "Forgot password changed successfully");
            }
        });

For whatever reason, when checking the "Forgot password" checkbox in Keycloak's Realm settings, the confirmation popup is not being detected by Cypress, but seemingly only on CI.

Unfortunately I'm also not too adept in frontend technologies, so we'll have to play this one by ear, a bit. If any of your colleagues know Cypress and would like to contribute their knowledge, that would also be appreciated :)

  1. Could you try modifying the test so that it intercepts a PUT request, like below? Perhaps that will coax it to behave:
        cy.get('#kc-forgot-pw-switch').then(($checkbox) => {
            if (!$checkbox.prop('checked')) {
                cy.intercept('PUT', '/admin/realms/master') // Add this
                    .as("saveRealmSettings");
                cy.wrap($checkbox).check({ force: true });
                cy.wait("@saveRealmSettings"); // And this
                cy.get('.pf-c-alert__title').should('contain', "Forgot password changed successfully");
            }
        });
  1. Could you run the tests locally, as described in CONTRIBUTING.md? That way we can make sure that this is strictly a CI issue (and therefore most likely a race condition or something similar).

  2. If all else fails, we could try removing the cy.get('.pf-c-alert__title')... line altogether - perhaps just acknowledging that the @saveRealmSettings request was successful will be enough.

@xgp
Copy link
Contributor Author

xgp commented Jun 24, 2024

@daniel-frak regarding running the tests locally, I get the same error:

DevTools listening on ws://127.0.0.1:41043/devtools/browser/4c62d347-9a73-4a35-9aa3-995824169efa

====================================================================================================

  (Run Starting)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Cypress:        13.11.0                                                                        │
  │ Browser:        Electron 118 (headless)                                                        │
  │ Node Version:   v20.11.0 (/home/garth/.nvm/versions/node/v20.11.0/bin/node)                    │
  │ Specs:          1 found (migrating_users.cy.js)                                                │
  │ Searched:       cypress/e2e/**/*.cy.{js,jsx,ts,tsx}                                            │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                    
  Running:  migrating_users.cy.js                                                           (1 of 1)


  user migration plugin
    1) "before all" hook for "should migrate user"


  0 passing (24s)
  1 failing

  1) user migration plugin
       "before all" hook for "should migrate user":
     AssertionError: Timed out retrying after 20000ms: Expected to find element: `.pf-c-alert__title`, but never found it.

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `user migration plugin`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
      at Context.eval (webpack://e2e-tests/./cypress/e2e/migrating_users.cy.js:72:45)




  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        4                                                                                │
  │ Passing:      0                                                                                │
  │ Failing:      1                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      3                                                                                │
  │ Screenshots:  1                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     23 seconds                                                                       │
  │ Spec Ran:     migrating_users.cy.js                                                            │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


  (Screenshots)

  -  /home/garth/projects/saascrud/keycloak-user-migration/docker/e2e/cypress/screens     (1280x720)
     hots/migrating_users.cy.js/user migration plugin -- should migrate user -- befor               
     e all hook (failed).png                                                                        


====================================================================================================

  (Run Finished)


       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✖  migrating_users.cy.js                    00:23        4        -        1        -        3 │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✖  1 of 1 failed (100%)                     00:23        4        -        1        -        3  

Inspecting the HTML, it looks like the style has changed (Keycloak team upgraded to Patternfly 5) from pf-c-alert__title pf-v5-c-alert__title. When I replace all occurrences of that, it passes that error, but then fails with another error:

DevTools listening on ws://127.0.0.1:32863/devtools/browser/bb9e67a5-2fca-4309-a779-927dc880b362

====================================================================================================

  (Run Starting)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Cypress:        13.11.0                                                                        │
  │ Browser:        Electron 118 (headless)                                                        │
  │ Node Version:   v20.11.0 (/home/garth/.nvm/versions/node/v20.11.0/bin/node)                    │
  │ Specs:          1 found (migrating_users.cy.js)                                                │
  │ Searched:       cypress/e2e/**/*.cy.{js,jsx,ts,tsx}                                            │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────
                                                                                                    
  Running:  migrating_users.cy.js                                                           (1 of 1)


  user migration plugin
    1) "before all" hook for "should migrate user"


  0 passing (7s)
  1 failing

  1) user migration plugin
       "before all" hook for "should migrate user":
     CypressError: Timed out retrying after 5000ms: `cy.wait()` timed out waiting `5000ms` for the 1st request to the route: `realm`. No request ever occurred.

https://on.cypress.io/wait

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `user migration plugin`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
      at cypressErr (http://localhost:8024/__cypress/runner/cypress_runner.js:75613:18)
      at Object.errByPath (http://localhost:8024/__cypress/runner/cypress_runner.js:75668:10)
      at checkForXhr (http://localhost:8024/__cypress/runner/cypress_runner.js:135230:84)
      at <unknown> (http://localhost:8024/__cypress/runner/cypress_runner.js:135256:28)
      at tryCatcher (http://localhost:8024/__cypress/runner/cypress_runner.js:1807:23)
      at Promise.attempt.Promise.try (http://localhost:8024/__cypress/runner/cypress_runner.js:4315:29)
      at whenStable (http://localhost:8024/__cypress/runner/cypress_runner.js:143678:68)
      at <unknown> (http://localhost:8024/__cypress/runner/cypress_runner.js:143619:14)
      at tryCatcher (http://localhost:8024/__cypress/runner/cypress_runner.js:1807:23)
      at Promise._settlePromiseFromHandler (http://localhost:8024/__cypress/runner/cypress_runner.js:1519:31)
      at Promise._settlePromise (http://localhost:8024/__cypress/runner/cypress_runner.js:1576:18)
      at Promise._settlePromise0 (http://localhost:8024/__cypress/runner/cypress_runner.js:1621:10)
      at Promise._settlePromises (http://localhost:8024/__cypress/runner/cypress_runner.js:1701:18)
      at Promise._fulfill (http://localhost:8024/__cypress/runner/cypress_runner.js:1645:18)
      at <unknown> (http://localhost:8024/__cypress/runner/cypress_runner.js:5450:46)
  From Your Spec Code:
      at visitMigrationConfigPage (webpack://e2e-tests/./cypress/e2e/migrating_users.cy.js:96:11)
      at configureMigrationPlugin (webpack://e2e-tests/./cypress/e2e/migrating_users.cy.js:78:8)
      at Context.eval (webpack://e2e-tests/./cypress/e2e/migrating_users.cy.js:42:8)




  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        4                                                                                │
  │ Passing:      0                                                                                │
  │ Failing:      1                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      3                                                                                │
  │ Screenshots:  1                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     7 seconds                                                                        │
  │ Spec Ran:     migrating_users.cy.js                                                            │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘


  (Screenshots)

  -  /home/garth/projects/saascrud/keycloak-user-migration/docker/e2e/cypress/screens     (1280x720)
     hots/migrating_users.cy.js/user migration plugin -- should migrate user -- befor               
     e all hook (failed).png                                                                        


====================================================================================================

  (Run Finished)


       Spec                                              Tests  Passing  Failing  Pending  Skipped  
  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ ✖  migrating_users.cy.js                    00:07        4        -        1        -        3 │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘
    ✖  1 of 1 failed (100%)                     00:07        4        -        1        -        3  

@daniel-frak
Copy link
Owner

daniel-frak commented Jun 25, 2024

Weirdly, I'm getting the same error (which I probably should have mentioned) when running your code, unless I increase the wait time of cy.get('.pf-c-alert__title') or change the code like I did above.

However, I don't get the second error that you're getting... For me, the pf-c-alert__title class remained unchanged between Keycloak builds, unless I'm checking something wrong.

What happens if you change the code like I did (adding the @saveRealmSettings check)? Does it also fail in the same way?

On my side, I'll try to find some time to reconfirm the pf-c-alert__title situation.

@xgp
Copy link
Contributor Author

xgp commented Jun 25, 2024

@pnzrr Can you look at this?

@daniel-frak
Copy link
Owner

daniel-frak commented Jul 2, 2024

Upon a closer look, it seems that the E2E tests have become generally flaky (even on the master branch).

I've improved them somewhat on the main branch. Could you pull from master and see if the CI passes this time?

In the meantime, I'll do some more investigating regarding the tests...

EDIT:

After the most recent refactoring on the master branch, the tests now all work consistently on my machine and (seemingly) on CI.

@daniel-frak
Copy link
Owner

@xgp Just a quick reminder about the rebase, so that we can merge the PR :)

@daniel-frak
Copy link
Owner

Ah, shucks :/

I'll try to find some time to confirm if the tests are just flaky here. If that's the case, I'll merge the PR and try to fix the test flakiness later...

@daniel-frak
Copy link
Owner

daniel-frak commented Jul 16, 2024

@xgp Ok, so I took a look at your branch and it seems that the change you made, where you replaced pf-c-alert__title with .pf-v5-c-alert__title is causing the tests to fail. Reverting that change makes the tests pass, at least locally.

Not sure why #kc-ui-display-name is failing in CI, but I think we can ignore that once the tests pass locally.

@xgp
Copy link
Contributor Author

xgp commented Jul 16, 2024

I still can't get tests to pass locally, even when reverting that change. I'm somewhat out of my element on these tests. As much as I've tried, I'm really unable to grok what's going on.

@daniel-frak
Copy link
Owner

Yeah, frontend's a bit of a mystery for me as well XD

But it does pass on my machine after the revert, so let's get it to that state and I'll merge it in, to not waste more of your time on the E2E test side.

Then I'll try to find some free time to stabilize the tests on the main branch.

Copy link

sonarcloud bot commented Jul 16, 2024

@daniel-frak daniel-frak merged commit db2732f into daniel-frak:master Jul 16, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requested changes Waiting for requested changes to be made to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants