-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Use Hamcrest matchers and assertThat() in ReindexRenamedSettingTests to improve readability #2503
Use Hamcrest matchers and assertThat() in ReindexRenamedSettingTests to improve readability #2503
Conversation
Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
in log 3509:
It's reported in issue #2242 |
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.
The first argument of the assert is the expected value, and the second is the actual value.
FYI, this is one reason to prefer the Hamcrest matcher style of assertThat(actualValue, equalTo(expectedValue))
because it reads more naturally and you don't have know the ordering.
@andrross Thank you for sharing the knowledge! Now I know
p.s. the advantages of |
In Log 3518:
It's reported in #1703, this one fails very often recently. |
Signed-off-by: Tianli Feng <[email protected]>
755f677
to
eccb012
Compare
import org.opensearch.common.settings.Setting; | ||
import org.opensearch.common.settings.Settings; | ||
import org.opensearch.test.OpenSearchTestCase; | ||
|
||
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
import static org.hamcrest.Matchers.equalTo; |
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.
According to suggestion from @andrross , I updated all the assertions to use assertThat()
and Matchers API from Hamcrest, in this file.
❌ Gradle Check failure 755f677ecc3d9dd91a0a4b8aa57f74fa528b5c8e |
In log 3522:
It's reported in issue #1746, this one also fails frequently recently. |
Signed-off-by: Tianli Feng <[email protected]>
…lowlist-setting Signed-off-by: Tianli Feng <[email protected]>
Signed-off-by: Tianli Feng <[email protected]>
In report 3535: start gradle check |
Description
assertThat()
inReindexRenamedSettingTests
to improve the readability.Reason:
The order of the
assertEquals()
arguments inReindexRenamedSettingTests
class is reversed.The unit test class
ReindexRenamedSettingTests
were added in PR #2221 / commit 63c75d1According to the comment in #2463 (comment):
Issues Resolved
Related to issue #1547
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.