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.

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

Bmoric/fix test bad catch #7290

Merged
merged 3 commits into from
Oct 23, 2021
Merged

Bmoric/fix test bad catch #7290

merged 3 commits into from
Oct 23, 2021

Conversation

benmoriceau
Copy link
Contributor

What

Remove the static import.

Fix a test which was not testing what it was claiming to test.

@github-actions github-actions bot added the area/worker Related to worker label Oct 22, 2021
@benmoriceau benmoriceau temporarily deployed to more-secrets October 22, 2021 22:37 Inactive
@Dharminder315
Copy link

Internet connection

Copy link
Contributor

@jrhizor jrhizor left a comment

Choose a reason for hiding this comment

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

I'm not sure removing static imports for when and assertThat are useful. They're super common to statically import, and I don't think Assertions.assertThat(x).isEqualTo(y) is easier to read. I don't feel too strongly either way on this though, so 🤷

The exception handling fluent interface (and the improved test cases) is a big improvement though.

@benmoriceau benmoriceau temporarily deployed to more-secrets October 22, 2021 23:31 Inactive
@benmoriceau
Copy link
Contributor Author

I'm not sure removing static imports for when and assertThat are useful. They're super common to statically import, and I don't think Assertions.assertThat(x).isEqualTo(y) is easier to read. I don't feel too strongly either way on this though, so 🤷

The exception handling fluent interface (and the improved test cases) is a big improvement though.

For the static import, I like to remove them for on main reason: autocompletion. I like to be able to type the root class name and see what is available. I have removed it for this review.

I agree that Assertions.assertThat(x).isEqualTo(y) doesn't bring much but overall AssertJ has much better wrapper that Junit5 to make your test as easy to understand as possible. For example you can test list like that:

assertThat(list).hasSize(2)
        .containsExactlyInAnyOrder("blabla")
        .isSubsetOf(Lists.newArrayList("1", "2"));

They provide those nice wrapper for many type of the java standard lib. I would prefer to use it everywhere we can even if for some case it might not bring much for some assertion.

@benmoriceau benmoriceau merged commit e95a805 into master Oct 23, 2021
@benmoriceau benmoriceau deleted the bmoric/fix-test-bad-catch branch October 23, 2021 02:11
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
Fix a test which was not testing what it was claiming to test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants