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

Make transport and MockTicker tests more stable #13713

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

chrisvest
Copy link
Contributor

@chrisvest chrisvest commented Nov 30, 2023

Motivation:
A number of flaky tests were identified.

Modification:
Address test isolation failures and resource exhaustion errors.
Also enable parallel test execution, becuase the tests can probably tolerate that now.
Improve the DefaultMockTicker test to be deterministic, by adding the ability to wait for other threads to enter the sleep method.

Result:
Tests in the transport module should now run stable enough to run in parallel.
All the tests can now also be run in repeat.

Motivation:
A number of flaky tests were identified.

Modification:
Address test isolation failures and resource exhaustion errors.
Also enable parallel test execution, becuase the tests can probably tolerate that now.

Result:
Tests in the transport module should now run stable enough to run in parallel.
Also made the test run dramatically faster.
We don't need to wait that long to confirm that a future hasn't completed.
Thread.sleep(50);
} catch (InterruptedException e) {
// Ignore
}
allTimeStampsLatch.countDown();
}, 100, 100, TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these windows a bit stingy? It seems pretty easy for a CI thread to lose 100 ms.

@chrisvest
Copy link
Contributor Author

NioEventLoopTest. testTaskRemovalOnShutdownThrowsNoUnsupportedOperationException still produces OOME, but we didn't get any artifacts again…

The test tended to run out of memory by filling up the executor task queue.
Copy link
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

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

Thanks @chrisvest!

@chrisvest
Copy link
Contributor Author

Panama build looks like it got stuck on uploading its docker cache.
Let's merge this and see if it helps our other PRs.

@chrisvest chrisvest changed the title Make transport tests more stable Make transport and MockTicker tests more stable Dec 1, 2023
@chrisvest chrisvest merged commit 438b436 into netty:main Dec 1, 2023
12 of 13 checks passed
@chrisvest chrisvest deleted the 5x-transport-tests branch December 1, 2023 22:10
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.

None yet

2 participants