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

[MINOR] fix(test): Fix flaky test for CoordinatorReconfigureNodeMaxTest#testReconfigureNodeMax #2325

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maobaolong
Copy link
Member

What changes were proposed in this pull request?

Fix flaky test for CoordinatorReconfigureNodeMaxTest#testReconfigureNodeMax

Why are the changes needed?

Make CI stable.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs.

Copy link

github-actions bot commented Jan 5, 2025

Test Results

 2 966 files  ±0   2 966 suites  ±0   6h 35m 54s ⏱️ + 8m 36s
 1 100 tests ±0   1 098 ✅ ±0   2 💤 ±0  0 ❌ ±0 
13 774 runs  ±0  13 744 ✅ ±0  30 💤 ±0  0 ❌ ±0 

Results for commit 39ee790. ± Comparison against base commit 3b0521a.

♻️ This comment has been updated with latest results.

@maobaolong maobaolong closed this Jan 5, 2025
@maobaolong maobaolong reopened this Jan 5, 2025
jerqi
jerqi previously approved these changes Jan 7, 2025
@@ -131,9 +131,11 @@ public void testReconfigureNodeMax() throws Exception {
assertEquals(5, info.getServerToPartitionRanges().keySet().size());

// case3: recover its value to 10
Thread.sleep(1000L);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should Awaitility.await.

Copy link
Member Author

@maobaolong maobaolong Jan 8, 2025

Choose a reason for hiding this comment

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

@jerqi I added a new comment above this line. This sleep to make sure the last modification time of the tempConfFilePath file changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

You wait for a modification timestamp instead sleep.

File tempConfFile = new File(tempConfFilePath);
long currentTime = System.currentTimeMillis();
if (currentTime - tempConfFile.lastModified() < 1000) {
Thread.sleep(1000 - (currentTime - tempConfFile.lastModified()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use Awaitility.await() instead of sleep?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants