-
Notifications
You must be signed in to change notification settings - Fork 25.4k
[Test] Actually invoke the listener on failure case #132229
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
Conversation
The listener is expected to be called on failure case. This PR does that. Resolves: elastic#131979
Pinging @elastic/es-core-infra (Team:Core/Infra) |
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, one optional comment only
var listener = ActionListener.assertAtLeastOnce( | ||
ActionTestUtils.<AcknowledgedResponse>assertNoSuccessListener(e -> assertTrue(e instanceof IllegalArgumentException)) | ||
); | ||
var listener = ActionListener.assertAtLeastOnce(ActionTestUtils.<AcknowledgedResponse>assertNoSuccessListener(e -> { | ||
assertTrue(e instanceof IllegalArgumentException); | ||
})); |
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.
Nit: I don't think these lines need to be changed - changing the parameter to a lambda block seems redundant.
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.
Yeah you are right. It was unintentional. Pushed 2716157
💚 Backport successful
|
The listener is expected to be called on failure case. This PR does that. Resolves: elastic#131979
The listener is expected to be called on failure case. This PR does that. Resolves: elastic#131979
The listener is expected to be called on failure case. This PR does that.
Resolves: #131979