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

XWIKI-22680: Regression on createAndDeleteUser #3675

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Nov 22, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22680
related to the regression introduced yesterday: https://ci.xwiki.org/job/XWiki/job/xwiki-platform/job/stable-16.10.x/7/testReport/junit/org.xwiki.administration.test.ui/AllIT$NestedRegisterIT/Platform_Builds___main___integration_tests___IT_for_xwiki_platform_core_xwiki_platform_administration_xwiki_platform_administration_test_xwiki_platform_administration_test_docker___Build_for_IT_for_xwiki_platform_core_xwiki_platform_administration_xwiki_platform_administration_test_xwiki_platform_administration_test_docker___registerExistingUser_boolean__boolean__boolean__TestUtils__2_/

Changes

Description

  • Fixed the wait condition at the end of the formFilling function

Clarifications

  • The wait was done on the password2 field. However, in the new testing conditions, this wait was incorrect because this field was not the last one we validate (yesterday we set a deterministic order where it wouldn't be the last). While editing, the field does not have any class on it. Once it's done being edited, it gets a class quickly after its validation. Here, we would still have the "old" validation state with an "invalid" class, that would end the wait immediately, but the form was not yet in its stable state. This means that the wait is too fast, and the form registration is tried when the state is incorrect, which means that the form is not submitted as we would want it to.

Screenshots & Video

None, test change only.

Executed Tests

After building the change with mvn clean install -f xwiki-platform-core/xwiki-platform-test/xwiki-platform-test-ui, I ran all of the docker tests for administration mvn clean install -f xwiki-platform-core/xwiki-platform-administration/xwiki-platform-administration-test/xwiki-platform-administration-test-docker. They did pass successfully. Unfortunately I wasn't able to catch a lot of those wait time issues when running locally previously, I can't be sure it doesn't fail CI until it runs here... :/

Expected merging strategy

* Fixed the wait condition at the end of the formFilling function
@Sereza7 Sereza7 marked this pull request as draft November 22, 2024 12:24
* Fixed the wait condition for password change
* Various code quality changes
@Sereza7
Copy link
Contributor Author

Sereza7 commented Nov 22, 2024

(for each tests, I made sure to build the changes before running them)

After 22aebac , rebuilt mvn clean install -f xwiki-platform-core/xwiki-platform-administration/xwiki-platform-administration-test/xwiki-platform-administration-test-docker -Dit.test=RegisterIT#registerExistingUser, it went okay.

On another hand, two other fails related to yesterday's PR were solved in d0d38f0 . Successful tests to validate this: mvn clean install -f xwiki-platform-core/xwiki-platform-security/xwiki-platform-security-authentication/xwiki-platform-security-authentication-test/xwiki-platform-security-authentication-test-docker/ -Dit.test=ResetPasswordIT#resetForgottenPassword and mvn clean install -f xwiki-platform-core/xwiki-platform-user/xwiki-platform-user-profile/xwiki-platform-user-profile-test/xwiki-platform-user-profile-test-docker/ -Dit.test=UserChangePasswordIT#changePassword.
The change password tests didn't fail like the others because the password2 validation was not ever done before trying to submit the form. With yesterday's change, it was run once when the password1 field was filled up. This first evaluation ended up in failure (fields are different for a moment), and the form couldn't be submitted. I just added a waiting time after filling up the form to solve this test failure.

@Sereza7 Sereza7 marked this pull request as ready for review November 22, 2024 13:16
@Sereza7
Copy link
Contributor Author

Sereza7 commented Nov 22, 2024

I retested changes with the updated pageobjects and could pass mvn clean install -f xwiki-platform-core/xwiki-platform-administration/xwiki-platform-administration-test/xwiki-platform-administration-test-docker -Dit.test=RegisterIT#registerExistingUser

getDriver().waitUntilCondition(driver -> !getFormElement().findElement(By.name("register2_password"))
.getAttribute(CLASS_ATTRIBUTE).isEmpty());
if(!valuesByElements.isEmpty()) {
WebElement finalLastElement = lastElement;
Copy link
Member

Choose a reason for hiding this comment

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

you don't need that variable, do you?

Copy link
Contributor Author

@Sereza7 Sereza7 Nov 22, 2024

Choose a reason for hiding this comment

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

The code was underlined with Variable used in lambda expression should be final or effectively final this was the quickest fix to get rid of this warning.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, you use it in a lambda.

if (valuesByNames.containsKey("register2_password")) {
getDriver().waitUntilCondition(driver -> !getFormElement().findElement(By.name("register2_password"))
.getAttribute(CLASS_ATTRIBUTE).isEmpty());
if(!valuesByElements.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

now you could also check if lastElement != null :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically if the list is not empty the lastElement should be properly set at least once because we do at least one pass in the loop above . It doesn't hurt to check though . Added in 3efdb2a 👍
I didn't run tests again, I suppose this change won't break things.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sorry my "also" was confusing, I meant you could have check only for the lastElement != null, would be more accurate given what you're doing inside. But nevermind I'll merge it like that, I've been annoying enough on this one ;)

* Various code quality changes
@surli surli merged commit 3d67202 into xwiki:master Nov 22, 2024
1 check passed
github-actions bot pushed a commit that referenced this pull request Nov 22, 2024
* Fixed the wait condition at the end of the formFilling function
* Fixed the wait condition for password change

(cherry picked from commit 3d67202)
surli pushed a commit that referenced this pull request Nov 22, 2024
* Fixed the wait condition at the end of the formFilling function
* Fixed the wait condition for password change

(cherry picked from commit 3d67202)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants