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.

Sign up for GitHub

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

Jump to bottom

[#6459]improve(test): Use explicit wait for time-based condition #6460

Merged
merged 2 commits into from
Feb 26, 2025
Merged

[#6459]improve(test): Use explicit wait for time-based condition #6460

merged 2 commits into from
Feb 26, 2025
+132 −155

Conversation

zzzk1
Copy link
Contributor

@zzzk1 zzzk1 commented Feb 15, 2025

What changes were proposed in this pull request?

This improvement follows our E2E testing guide by utilizing the wait methods and removing Thread.sleep(sleepTimeMillis).

Why are the changes needed?

Fix: #6459

Does this PR introduce any user-facing change?

no

How was this patch tested?

CI

@tengqm
Copy link
Contributor

tengqm commented Feb 17, 2025

lgtm. Good job.

@jerryshao
Copy link
Contributor

@zzzk1 can you please fix the CI issue here?

@zzzk1
Copy link
Contributor Author

zzzk1 commented Feb 17, 2025

@zzzk1 can you please fix the CI issue here?

Yes, ./gradlew test [--rerun-tasks] -PskipTests -PtestMode=embedded. What should I replace [--rerun-tasks] with if I want to set up E2E tests locally?

@jerryshao
Copy link
Contributor

You can keep --rerun-tasks, don't need to remove or replace with others. You should make sure you have docker environment locally, please check this doc https://gravitino.apache.org/docs/0.8.0-incubating/how-to-test.

@yuqi1129 yuqi1129 closed this Feb 19, 2025
@yuqi1129 yuqi1129 reopened this Feb 19, 2025
@jerryshao
Copy link
Contributor

Hi @zzzk1 are you still working on this, are you stuck on the CI issue? @yuqi1129 can you please help @zzzk1 to handle this?

@zzzk1
Copy link
Contributor Author

zzzk1 commented Feb 21, 2025

Hi @zzzk1 are you still working on this, are you stuck on the CI issue? @yuqi1129 can you please help @zzzk1 to handle this?

Yes, I'm trying to find why the integration test failed.

@yuqi1129 yuqi1129 closed this Feb 22, 2025
@yuqi1129 yuqi1129 reopened this Feb 22, 2025
yuqi1129
yuqi1129 reviewed Feb 22, 2025
View reviewed changes
...gration-test/src/test/java/org/apache/gravitino/integration/test/web/ui/utils/BaseWebIT.java
@@ -82,12 +82,10 @@ protected void clickAndWait(final Object locator) throws InterruptedException {
wait.until(ExpectedConditions.elementToBeClickable(locatorElement(locator)));

locatorElement(locator).click();
Thread.sleep(ACTION_SLEEP_MILLIS);
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue lies in here and L90. You should not delete these two lines unless you have alternatives.

@zzzk1 zzzk1 changed the title [#6459]improve(test): Use implicit wait for time-based condition [#6459]improve(test): Use explicit wait for time-based condition Feb 25, 2025
@zzzk1 zzzk1 force-pushed the improve-wait-condition branch from f32360e to c3ad38d Compare February 25, 2025 01:27
yuqi1129
yuqi1129 reviewed Feb 25, 2025
View reviewed changes
...gration-test/src/test/java/org/apache/gravitino/integration/test/web/ui/utils/BaseWebIT.java
@@ -82,12 +82,12 @@ protected void clickAndWait(final Object locator) throws InterruptedException {
wait.until(ExpectedConditions.elementToBeClickable(locatorElement(locator)));

locatorElement(locator).click();
Thread.sleep(ACTION_SLEEP_MILLIS);
Thread.sleep(ACTION_SLEEP * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

@zzzk1 @LauraXia123
Can this be replaced with WebDriverWait since we are going to remove Thread.sleep in this PR.

Copy link
Contributor Author

@zzzk1 zzzk1 Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@yuqi1129 I have tried, but there is no way to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zzzk1, Please revert the changes here. I will take a final review.

@zzzk1
Copy link
Contributor Author

zzzk1 commented Feb 25, 2025

I follow this: run-integration-test to execute the integration test locally, but it failed. (Ubuntu 24.04.2 LTS, JDK17 & JDK8)
my steps:

  1. ./gradlew build -x test
  2. ./gradlew test -PskipTests -PtestMode=embedded

image
However, using ./gradlew build was successful. Is there any mistake here?

@yuqi1129
Copy link
Contributor

2. ./gradlew test -PskipTests -PtestMode=embedded

Running ./gradlew test -PskipTests -PtestMode=embedded should not require the file gravitino/distribution/package/bin/gravitino.sh, can you please create a issue to record this problem?

@zzzk1
Copy link
Contributor Author

zzzk1 commented Feb 25, 2025

  1. ./gradlew test -PskipTests -PtestMode=embedded

Running ./gradlew test -PskipTests -PtestMode=embedded should not require the file gravitino/distribution/package/bin/gravitino.sh, can you please create a issue to record this problem?

sure.

@zzzk1 zzzk1 force-pushed the improve-wait-condition branch from 3ed7102 to ff3bc2a Compare February 25, 2025 07:36
yuqi1129
yuqi1129 previously approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

LGTM

jerryshao
jerryshao reviewed Feb 25, 2025
View reviewed changes
...tion-test/src/test/java/org/apache/gravitino/integration/test/web/ui/pages/CatalogsPage.java Outdated
return false;
}
String xpath =
"//div[@data-refer='table-grid']//div[contains(@class, 'MuiDataGrid-main')]/div[contains(@class, 'MuiDataGrid-virtualScroller')]/div/div[@role='rowgroup']//div[@data-field='name']";
Copy link
Contributor

Choose a reason for hiding this comment

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

Some lines like here are too long, our format requires that each line should not be longer than 100 characters, can you break such super long line into several lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course I will.

@jerryshao jerryshao merged commit f7719f8 into apache:main Feb 26, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerryshao jerryshao jerryshao approved these changes

@yuqi1129 yuqi1129 yuqi1129 left review comments

Assignees

@zzzk1 zzzk1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement] Use the explicit wait feature provided by Selenium to replace Thread.Sleep
4 participants
@zzzk1 @tengqm @jerryshao @yuqi1129