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

Investigate flaky test in AsyncTest #1070

Closed
sleberknight opened this issue Nov 10, 2023 · 4 comments · Fixed by #1072 or #1076
Closed

Investigate flaky test in AsyncTest #1070

sleberknight opened this issue Nov 10, 2023 · 4 comments · Fixed by #1072 or #1076
Assignees
@sleberknight
Labels
investigation Something that needs to be investigated before implementation can proceed
Milestone
3.2.0

Comments

@sleberknight
Copy link
Member

sleberknight commented Nov 10, 2023 •
edited
Loading

In a few builds today, this error happened:

AsyncTest$WaitFor.shouldSucceed_WhenTheFutureCompletes_BeforeTimeout:273 »
   Async TimeoutException occurred (maximum wait was specified as 250 MILLISECONDS)

Why is this happening when the ConcurrentTask used inside this test has a default "task" duration of 10 milliseconds?
@sleberknight sleberknight self-assigned this Nov 10, 2023
@sleberknight sleberknight added the investigation Something that needs to be investigated before implementation can proceed label Nov 10, 2023
@sleberknight
Copy link
Member Author

This just happened several more times in (dependabot) PR #1071

@sleberknight sleberknight added this to the 3.1.1 milestone Nov 15, 2023
sleberknight added a commit that referenced this issue Nov 15, 2023
@sleberknight
Make flaky test in AsyncTest a "retrying test"
d2ea6c8 
Hopefully temporary, but change
shouldSucceed_WhenTheFutureCompletes_BeforeTimeout
to a RetryingTest so that it (hopefully) will consistently
pass when running in GitHub actions.

Relatest to #1070
@sleberknight sleberknight mentioned this issue Nov 15, 2023
Merged
@sleberknight sleberknight linked a pull request Nov 15, 2023 that will close this issue
Make flaky test in AsyncTest a "retrying test" #1072
Merged
sleberknight added a commit that referenced this issue Nov 15, 2023
@sleberknight
Make flaky test in AsyncTest a "retrying test" (#1072)
2f96e45 
Hopefully temporary, but change
shouldSucceed_WhenTheFutureCompletes_BeforeTimeout
to a RetryingTest so that it (hopefully) will consistently
pass when running in GitHub actions.

Relatest to #1070
@sleberknight
Copy link
Member Author

sleberknight commented Nov 15, 2023 •
edited
Loading

No, GitHub, "Relates to" should not close an issue...so, I am re-opening

@sleberknight sleberknight reopened this Nov 15, 2023
@sleberknight
Copy link
Member Author

This is not fixed. I merely made that test a retrying test, which is 100% not a fix. It is just something to make sure the tests pass, and provide more time for investigation.

@sleberknight
Copy link
Member Author

There are now four tests that use @RetryingTest. All of them call Async#doAsync(Supplier<T> supplier), which under the covers does this:

return doAsync(supplier, ForkJoinPool.commonPool());

I am wondering if it's possible that the first call to ForkJoinPool.commonPool() takes a while, for example maybe that method does lazy initialization. And since the tests are not run in the same order, it means any of the four tests that are now using @RetryingTest could be executed first. And maybe that very first time the commonPool() is called, it takes longer than our 205 millisecond timeout. This does seem like an eternity for a method in the JDK like this, but this might be something to look into.

And if it does turn out to be the problem, then we could probably call ForkJoinPool.commonPool() in a @BeforeAll method just to make sure it's initialized. If this doesn't end up being the problem, then we need more investigation...

@sleberknight sleberknight mentioned this issue Nov 16, 2023
Merged
@sleberknight sleberknight linked a pull request Nov 18, 2023 that will close this issue
Fix flaky tests in AsyncTest #1076
Merged
sleberknight added a commit that referenced this issue Nov 18, 2023
@sleberknight
Fix flaky tests in AsyncTest (#1076)
7b13e28 
Ensure that the tests all pass consistently by canceling the futures in the tests
where we expect to timeout before the tasks complete. Without canceling them,
tests which call methods in Async that use the common ForkJoinPool can be
blocked by tasks that are still executing from previous tests! So, in the three tests
where waitFor/waitForAll/waitForAllIgnoringTypes time out before the tasks complete,
wrap the assertions in a try/finally and cancel the futures in the finally block. This
commit also removes the RetryingTest annotations on the four tests which had them
and replaces them with the standard JUnit Test annotation. Hopefully this will be
the last time we ever need to deal with flaky tests in Async.

Closes #1070
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sleberknight sleberknight

Labels
investigation Something that needs to be investigated before implementation can proceed
Projects
None yet
Milestone
3.2.0
1 participant
@sleberknight