Skip to content

GH-70: Move from hamcrest to assertj in flight-sql #772

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

Merged
merged 3 commits into from
May 28, 2025

Conversation

rtadepalli
Copy link
Contributor

What's Changed

Series of PRs to consolidate on using assertj in tests as part of #70.

This comment has been minimized.

containsString("SQL"), // pk_key_name
is("3"), // update_rule
is("3")); // delete_rule
new HamcrestCondition<>(nullValue(String.class)), // pk_catalog_name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not particularly certain how to get rid of Hamcrest entirely here. I was aware of HamcrestCondition so using that, but open to suggestions if anyone knows how this list-based matching can be accomplished solely using assertj.

Copy link
Member

Choose a reason for hiding this comment

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

Using a list of lambdas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entirely overlooked that assertj provides lambda based conditions. Changed. Thanks.

@lidavidm lidavidm added the chore PRs that make misc changes. label May 27, 2025
@rtadepalli
Copy link
Contributor Author

I've formatted my changes using mvn spotless:apply.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@@ -247,16 +247,15 @@ private static List<List<String>> getNonConformingResultsForGetSqlInfo(
@Test
public void testGetTablesSchema() {
final FlightInfo info = sqlClient.getTables(null, null, null, null, true);
MatcherAssert.assertThat(
info.getSchemaOptional(), is(Optional.of(FlightSqlProducer.Schemas.GET_TABLES_SCHEMA)));
Assertions.assertThat(info.getSchemaOptional())
Copy link
Member

Choose a reason for hiding this comment

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

nit, but IMO it's OK to statically import assertThat.

containsString("SQL"), // pk_key_name
is("3"), // update_rule
is("3")); // delete_rule
new HamcrestCondition<>(nullValue(String.class)), // pk_catalog_name
Copy link
Member

Choose a reason for hiding this comment

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

Using a list of lambdas?

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidavidm lidavidm merged commit 17f85a1 into apache:main May 28, 2025
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore PRs that make misc changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants