-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix failed tests for branch-2.10 #15995
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
Fix failed tests for branch-2.10 #15995
Conversation
|
@codelipenghui:Thanks for your contribution. For this PR, do we need to update docs? |
|
@codelipenghui:Thanks for providing doc info! |
|
/pulsarbot run-failure-checks |
congbobo184
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't merge, wait the broker group2 pass, we can merge. now is testing, some tests can't pass
|
I have created an issue to track the failed test #16000 |
ac4cd17 to
ba020fd
Compare
ba020fd to
cb46c14
Compare
| Awaitility.await().untilAsserted(() -> { | ||
| assertEquals(managedLedger.getLedgersInfo().size(), 2); | ||
| assertEquals(managedLedger.getState(), ManagedLedgerImpl.State.ClosedLedger); | ||
| assertEquals(managedLedger.getLedgersInfo().size(), 3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this changed the intention of the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please see the description of this PR.
eolivelli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
eolivelli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codelipenghui can you please add in the description the reason why we need to set -DtestReuseFork=false ?
Not 100% sure what's going on for now, but looks like It's related to the concurrency issue in the class initialization. Just found some related information. |
Fixes: apache#16000 After cherry-picked apache#15627, the test `org.apache.bookkeeper.mledger.impl.ManagedLedgerTest` will fail for branch-2.10. Details to see apache#15627 (comment) Not 100% sure what's going on for now, but looks like It's related to the concurrency issue in the class initialization. Just found some related information. https://maven.apache.org/surefire/maven-surefire-plugin/examples/fork-options-and-parallel-execution.html#parallel-test-execution testcontainers/testcontainers-java#2939 (comment) (cherry picked from commit 0b7bacc)
Fixes: #16000
After cherry-picked #15627, the test
org.apache.bookkeeper.mledger.impl.ManagedLedgerTestwill fail for branch-2.10. Details to see #15627 (comment)