Skip to content

HDDS-9982. Improve assertTrue assertions in hdds-server-framework#5862

Merged
adoroszlai merged 3 commits intoapache:masterfrom
wzhallright:HDDS-9982
Dec 26, 2023
Merged

HDDS-9982. Improve assertTrue assertions in hdds-server-framework#5862
adoroszlai merged 3 commits intoapache:masterfrom
wzhallright:HDDS-9982

Conversation

@wzhallright
Copy link
Contributor

@wzhallright wzhallright commented Dec 25, 2023

What changes were proposed in this pull request?

Improve assertTrue assertions in hadoop-hdds/framework
Assertions in the form:

assertTrue(<string>.contains(...))

do not provide any information about the actual value, if the assertion fails.

AssertionFailedError: expected: <true> but was: <false>

This can be improved by replacing them with:

import static org.assertj.core.api.Assertions.assertThat;
assertThat(<string>).contains(...)

which gives us more info in the form:

AssertionError: 

Expecting:
 <"actual string">
to contain:
 <"part"> 

Similarly, assertions about inequality relations, e.g.:

assertTrue(<x> > <y>)

can be replaced with:

assertThat(<x>).isGreaterThan(<y>);

The goal of this task is to find and replace such assertTrue assertions.

What is the link to the Apache JIRA

HDDS-9982

CI

https://github.com/wzhallright/ozone/actions/runs/7318537019

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @wzhallright for working on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

assertFalse assertions can be changed along the same lines:

assertThat(result).doesNotContain(entry.getKey());

Sorry if that wasn't clear from the task description. I have now updated the parent issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 186 to 190
Copy link
Contributor

Choose a reason for hiding this comment

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

With assertTrue(..., message), the second parameter was only passed to get some useful hint in case of failure. When using assertThat, we get such hint automatically, so we can omit withFailMessage(message).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to omit all waitFailMessage or allow the user to partially customize useful messages?
Because there is still some user-defined information in the current PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove two kinds:

  • where the string itself is passed (like here)
  • message similar to "expected: ..., got: ..."

We can keep the rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here the "not equal" value is changed from 0 to 1. Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

I think withFailMessage can be omitted here, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: keeping .contains... on new line makes the assertion more readable, because assertThat's argument is also a chained method call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

This one can also be converted to assertThat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@adoroszlai adoroszlai merged commit 41f284a into apache:master Dec 26, 2023
@adoroszlai
Copy link
Contributor

Thanks @wzhallright for the patch.

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.

2 participants

Comments