Repository navigation
migration test RequiredActionUpdateProfileTest - #48648
Conversation
8fdb9ed to
7843fca
Compare
There was a problem hiding this comment.
Thanks @gaoyikeshuer! LGTM, just we need to add to the database and forms suite. I'm addiing @vaceksimon for the review too.
282e64f to
4912c47
Compare
|
Hi Yike, thanks for the contribution! I am swamped this week, I can take a look on Tuesday. |
ed93004 to
fd7a3aa
Compare
There was a problem hiding this comment.
Thanks @gaoyikeshuer! It's almost OK, I don'r understand why you are removing the other class RequiredActionUpdateProfileWithUserProfileTest. Is this done on purpose? I think that this class should not be touched for the moment.
The other comment is just because I think you can just create the users with the same generated password and do not need to the resetPassword method anymore. It's much clearer this way. WDYT?
fd7a3aa to
6ae390c
Compare
6ae390c to
6a42987
Compare
7ed1145 to
f6e4ed4
Compare
| @@ -493,12 +532,12 @@ public void testMultivaluedAttributes() { | |||
| updateProfilePage.clickAddAttributeValue(elementId); | |||
| } | |||
| updateProfilePage.update("f", "l", "e@keycloak.org"); | |||
There was a problem hiding this comment.
@gaoyikeshuer I think you need to wait here for the page, my feeling is that you are reading the user before the browser has submitted the page.
driver.waiting().waitForOAuthCallback();In the four updateProfilePage.update in this method.
There was a problem hiding this comment.
Hi @rmartinc , thanks for pointing that out! Your suggestion fixed testMultivaluedAttributes and I added driver.waiting().waitForOAuthCallback(); after each of the four updateProfilePage.update(...) calls in that method.
But that alone wasn't enough for the rest of the class. The other tests that submit invalid input (updateProfileInvalidEmail, updateProfileMissingLastName, updateProfileMissingUsername, updateProfileDuplicateUsername) were failing with StaleElementReferenceException: after the server re-renders the form with the error, the cached WebElement is gone, so reading its value throws. I updated LoginUpdateProfilePage.readValue to catch SERE and retry with a fresh findElement.
With both fixes the suite is green on firefox-headless now. The whole build is red only because of a flaky unrelated test, but I don't have permission to re-run a single job. Moving the PR to ready for review.
2b1c1b2 to
e5c0fb0
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.BrowserFlowTest#testUserWithoutAdditionalFactorConnectionKeycloak CI - Forms IT (chrome) |
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.BrowserFlowTest#testUserWithoutAdditionalFactorConnectionKeycloak CI - Forms IT (chrome) |
There was a problem hiding this comment.
Thanks @gaoyikeshuer! LGTM! Good job!
@vaceksimon Do you want to review this one tomorrow? Please let me know because for me it's OK now.
There was a problem hiding this comment.
Good work @gaoyikeshuer! I have a suggestion to use ManagedUser instances, which will reduce boilerplate code and the number of admin API requests.
|
|
||
| @Page | ||
| protected AppPage appPage; | ||
| @InjectOAuthClient(config = RequiredActionUpdateProfileClientConfig.class) |
There was a problem hiding this comment.
This config class is not needed and can be removed.
| realm.users(UserBuilder.create("test-user@localhost") | ||
| .email("test-user@localhost") | ||
| .name("Tom", "Brady") | ||
| .password(PASSWORD) | ||
| .emailVerified(true)); | ||
| realm.users(UserBuilder.create("john-doh@localhost") | ||
| .email("john-doh@localhost") | ||
| .name("John", "Doh") | ||
| .password(PASSWORD) | ||
| .emailVerified(true)); |
There was a problem hiding this comment.
These two users can be moved to their own configuration classes instead:
private static class TestUserConfig implements UserConfig {
@Override
public UserBuilder configure(UserBuilder user) {
return user.username("test-user@localhost")
.email("test-user@localhost")
.name("Tom", "Brady")
.password(PASSWORD)
.emailVerified(true);
}
}
private static class JohnDohUserConfig implements UserConfig {
@Override
public UserBuilder configure(UserBuilder user) {
return user.username("john-doh@localhost")
.email("john-doh@localhost")
.name("John", "Doh")
.password(PASSWORD)
.emailVerified(true);
}
}This brings the benefit that it can be passed to @InjectUser and you get access to a ManagedUser instance. Something like:
@InjectUser(ref="testUser", config = RequiredActionUpdateProfileTest.TestUserConfig.class, lifecycle = LifeCycle.METHOD)
ManagedUser testUser;
@InjectUser(ref="johnDoh", config = RequiredActionUpdateProfileTest.JohnDohUserConfig.class, lifecycle = LifeCycle.METHOD)
ManagedUser johnDohUser;With these, you can now get rid of the whole @BeforeEach method.
It also gives access to this entities UserResource. So basically instead of looking fot the user with:
AdminApiUtil.findUserByUsername(realm.admin(), "test-user@localhost")you can use directly:
testUser.admin().toRepresentation();| Assertions.assertEquals("New first", user.getFirstName()); | ||
| Assertions.assertEquals("New last", user.getLastName()); | ||
| Assertions.assertEquals("john-doh@localhost", user.getEmail()); | ||
| Assertions.assertEquals("new", user.getUsername()); | ||
| // email not changed so verify that emailVerified flag is NOT reset | ||
| Assertions.assertEquals(true, user.isEmailVerified()); | ||
| getCleanup().addUserId(user.getId()); | ||
| realm.cleanup().add(r -> r.users().get(user.getId()).remove()); |
There was a problem hiding this comment.
Once you have a ManagedUser with a lifecycle method, this should be redundant
e5c0fb0 to
0ee253e
Compare
There was a problem hiding this comment.
@gaoyikeshuer The tests seems to be failing now! Take a look when you have time. Thanks!
0ee253e to
2ffd553
Compare
Signed-off-by: Yike Gao <yikegao8@gmail.com>
2ffd553 to
57c8400
Compare
There was a problem hiding this comment.
Thanks @gaoyikeshuer! Please @vaceksimon check if it is OK now for you.
There was a problem hiding this comment.
Perfect @gaoyikeshuer !
|
Thanks @gaoyikeshuer and @vaceksimon! |
…loak#48648) Signed-off-by: Yike Gao <yikegao8@gmail.com>
Closes #48149