Skip to content

Replace Assert.fail usage with AssertJ fluent testing #6029

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
Merged
rdblue merged 1 commit into from
Oct 21, 2022
Merged

Replace Assert.fail usage with AssertJ fluent testing #6029

rdblue merged 1 commit into from
Oct 21, 2022

Conversation

@nastra
Copy link
Contributor

@nastra nastra commented Oct 21, 2022 •
edited
Loading

This replaces a bunch of code of the form

try {
      // do something
      Assert.fail(...);
    } catch (Exception e) {
        // make sure exception has correct message/code/...
    }

There are still a few legit places left that use Assert.fail() so I didn't create a checkstyle rule to prevent usage

nastra
nastra commented Oct 21, 2022
View reviewed changes
api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java
@Test
public void testRunSafely() {
CustomCheckedException exc = new CustomCheckedException("test");
try {
Copy link
Contributor Author

@nastra nastra Oct 21, 2022

Choose a reason for hiding this comment

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

I have to admit that the original test code was quite difficult to read, so I tried to simplify it to the bare minimum

@nastra nastra force-pushed the assert-fail-improvements branch from 0ba187a to ac040a6 Compare October 21, 2022 11:05
@nastra nastra force-pushed the assert-fail-improvements branch from ac040a6 to a06939c Compare October 21, 2022 11:08
@nastra nastra mentioned this pull request Oct 21, 2022
Merged
Fokko
Fokko approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

I love it!

@nastra nastra requested a review from rdblue October 21, 2022 13:38
rdblue
rdblue reviewed Oct 21, 2022
View reviewed changes
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java
"Validation should complain about missing field",
e.getMessage().contains("Cannot find field 'missing' in struct:"));
}
Assertions.assertThatThrownBy(() -> unbound.bind(struct))
Copy link
Contributor

@rdblue rdblue Oct 21, 2022

Choose a reason for hiding this comment

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

Wow, this was so old it predated adding the assertThrows methods!

rdblue
rdblue reviewed Oct 21, 2022
View reviewed changes
api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java
"test finally suppression",
finallySuppressed.getMessage());
}
Exception suppressedOne = new Exception("test catch suppression");
Copy link
Contributor

@rdblue rdblue Oct 21, 2022

Choose a reason for hiding this comment

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

This looks a lot better.

rdblue
rdblue reviewed Oct 21, 2022
View reviewed changes
aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java
Assertions.assertThatThrownBy(
() ->
glueCatalog.createNamespace(
Namespace.of("denied_" + UUID.randomUUID().toString().replace("-", ""))))
Copy link
Contributor

@rdblue rdblue Oct 21, 2022

Choose a reason for hiding this comment

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

In the future, it would be better to put the namespace in a variable to avoid such awkward auto formatting.

rdblue
rdblue reviewed Oct 21, 2022
View reviewed changes
aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java
glueCatalog.createNamespace(namespace);
} catch (GlueException e) {
LOG.error("fail to create or delete Glue database", e);
Assert.fail("create namespace should succeed");
Copy link
Contributor

@rdblue rdblue Oct 21, 2022

Choose a reason for hiding this comment

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

Is the idea here that the whole test will fail so we don't need to assert?

rdblue
rdblue reviewed Oct 21, 2022
View reviewed changes
dell/src/test/java/org/apache/iceberg/dell/mock/ecs/TestExceptionCode.java
Assert.assertEquals(message + ", http code", httpCode, e.getHttpCode());
Assert.assertEquals(message + ", error code", errorCode, e.getErrorCode());
}
Assertions.assertThatThrownBy(task::run)
Copy link
Contributor

@rdblue rdblue Oct 21, 2022

Choose a reason for hiding this comment

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

This seems more complicated than the original try/catch. Not sure it's worth it.

rdblue
rdblue reviewed Oct 21, 2022
View reviewed changes
orc/src/test/java/org/apache/iceberg/orc/TestBloomFilter.java
}

Class clazzFileDump = Class.forName("org.apache.orc.tools.FileDump");
Class<?> clazzFileDump = Class.forName("org.apache.orc.tools.FileDump");
Copy link
Contributor

@rdblue rdblue Oct 21, 2022

Choose a reason for hiding this comment

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

Thanks for adding these! Never good to have types without parameters.

@rdblue rdblue merged commit 39a2c12 into apache:master Oct 21, 2022
@rdblue
Copy link
Contributor

rdblue commented Oct 21, 2022

Thanks, @nastra!

@nastra nastra deleted the assert-fail-improvements branch October 21, 2022 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdblue rdblue rdblue approved these changes

@Fokko Fokko Fokko approved these changes

Assignees

No one assigned

Labels

ALIYUN API AWS core data DELL flink MR ORC parquet spark

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nastra @rdblue @Fokko