-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Remove AccountPasswordPage from testsuite #15204
Conversation
Depends on #15197 to be merged first, will rebase after that is done. |
c96ce10
to
8476de8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stianst Thanks! I've added some comments there (most of them are nitpicks). We should still provide a certain confidence level for these tests; at least such as before these changes. As it touches broker/federation tests, it's necessary to have proper tests for that. WDYT?
|
||
// Login with old password doesn't work, but with new password works | ||
loginPage.login("jduke", "theduke"); | ||
loginPage.assertCurrent(); | ||
loginPage.login("jduke", "newPass"); | ||
changePasswordPage.assertCurrent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to verify the login was really successful? In the following steps, there's only used spnegoLogin()
, which only sends HTTP requests.
// Login with username/password from kerberos | ||
changePasswordPage.open(); | ||
loginPage.assertCurrent(); | ||
loginPage.login("jduke", "theduke"); | ||
changePasswordPage.assertCurrent(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this test case is included in the abstract test and is used for various additional test classes, IMHO, the login should be verified here via the LoginPage. Also, the test case is named usernamePasswordLoginTest
, and one could say if I execute this successfully, I'm really confident the login works as expected.
We're not sure the login with password "theduke"
worked before, as it's tested in the following steps.
@@ -166,8 +152,6 @@ public void writableEditModeTest() throws Exception { | |||
loginPage.login("jduke", "theduke"); | |||
Assert.assertTrue(loginPage.isCurrent()); | |||
loginPage.login("jduke", "newPass"); | |||
changePasswordPage.assertCurrent(); | |||
changePasswordPage.logout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above with the assertion.
@@ -0,0 +1,31 @@ | |||
package org.keycloak.testsuite.util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: License?
41c32c2
to
346936e
Compare
@mabartos I've updated the tests to fix the broken tests, and also introduced a TestAppHelper to make it simpler to test login/logout. Could you take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to properly address previous comments, I'd be glad if some additional changes would be made. I don't wanna block it, but would be good to have it there. Thanks!
// Bad existing password | ||
changePasswordPage.changePassword("theduke-invalid", "newPass", "newPass"); | ||
Assert.assertTrue(driver.getPageSource().contains("Invalid existing password.")); | ||
testAppHelper.login("jduke", "theduke"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testAppHelper.login("jduke", "theduke"); | |
Assert.assertTrue(testAppHelper.login("jduke", "theduke")); |
Would be good to verify the state. I'm aware the intention for that is to simplify the process and mainly execute the methods only as certain "procedures", but in this case it's ok(IMHO).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be sorted now
loginPage.login("jduke", "theduke"); | ||
Assert.assertTrue(loginPage.isCurrent()); | ||
loginPage.login("jduke", "newPass"); | ||
changePasswordPage.assertCurrent(); | ||
changePasswordPage.logout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use the TestAppHelper here and verify the state even for the last login?
Sth like this?
Assert.assertTrue(testAppHelper.login("jduke", "newPass"));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be sorted now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I follow @mabartos comments as there are a few places where assertions were removed.
In addition to that, some assertions changed to not check the message (for instance check if Your password has been updated
). I'm wondering if we should keep checks for messages if it makes sense.
6fa95c8
to
2df32a1
Compare
@mabartos @pedroigor asserts and testing failed logins added. @pedroigor there is no longer a "your password has been updated thing" needed as it's now just testing updating the password through the admin endpoints as that's way simpler, and we're testing if the password can be updated, not if a user can update the password through the account console, so I think that's more than sufficient. |
2df32a1
to
72add7c
Compare
/rerun |
Closes #15200