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

Replace assertTrue/assertFalse with AssertJ assertions #11665

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Replace assertTrue/assertFalse with AssertJ assertions #11665

merged 1 commit into from
Mar 29, 2022

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Mar 25, 2022

The reason for these improvements is because the AssertJ assertions generally
provide the actual values when an assertion fails.
This helps a lot when understanding why something failed on CI, while
assertTrue/assertFalse checks don't provide that kind of information

@cla-bot
Copy link

cla-bot bot commented Mar 25, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

hashhar
hashhar reviewed Mar 25, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Overall a very welcome change.

However in some of the assertions the error message helped "explain" why the assertion exists. Can we add those back via assertThat(...).as("message")...?

@nastra nastra force-pushed the assertion-improvements branch from 952ab9b to cf3fced Compare March 25, 2022 11:37
@cla-bot
Copy link

cla-bot bot commented Mar 25, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

@nastra nastra force-pushed the assertion-improvements branch from cf3fced to 58fd5bb Compare March 25, 2022 12:38
@cla-bot
Copy link

cla-bot bot commented Mar 25, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

@nastra
Copy link
Contributor Author

nastra commented Mar 25, 2022

Overall a very welcome change.

However in some of the assertions the error message helped "explain" why the assertion exists. Can we add those back via assertThat(...).as("message")...?

makes sense @hashhar, I added those "explaining" messages back. thanks for the review

@nastra nastra closed this Mar 29, 2022
@nastra nastra reopened this Mar 29, 2022
@nastra nastra force-pushed the assertion-improvements branch from 58fd5bb to 4cc57b8 Compare March 29, 2022 09:01
@cla-bot cla-bot bot added the cla-signed label Mar 29, 2022
@nastra nastra requested a review from hashhar March 29, 2022 09:55
hashhar
hashhar reviewed Mar 29, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % some style comments.

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMaterializedViews.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMaterializedViews.java Outdated Show resolved Hide resolved
@nastra
Replace assertTrue/assertFalse with AssertJ assertions
4f5e76e 
The reason for these changes is because the AssertJ assertions generally
provide the actual values when an assertion fails.
This helps a lot when understanding why something failed on CI, while
`assertTrue`/`assertFalse` checks don't provide that kind of information
@nastra nastra force-pushed the assertion-improvements branch from 4cc57b8 to 4f5e76e Compare March 29, 2022 13:29
hashhar
hashhar approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % comments.

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMaterializedViews.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMaterializedViews.java Show resolved Hide resolved
@hashhar hashhar added test no-release-notes This pull request does not require release notes entry labels Mar 29, 2022
@hashhar hashhar merged commit 884e756 into trinodb:master Mar 29, 2022
@github-actions github-actions bot added this to the 376 milestone Mar 29, 2022
@mosabua mosabua mentioned this pull request Mar 29, 2022
Merged
@nastra nastra deleted the assertion-improvements branch March 30, 2022 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashhar hashhar hashhar approved these changes

Assignees
No one assigned
Labels
cla-signed no-release-notes This pull request does not require release notes entry test
Milestone
376
Development

Successfully merging this pull request may close these issues.
2 participants
@nastra @hashhar