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

Remove redundant lambdas from test #20839

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 26, 2024

Whole test method asserts that every line of it succeeds, so using assertThatNoException is redundant. Of course there could be convention to clearly separate test setup and test assertions (given-when-then), but that's not a convention we use, thuse assertThatNoException() is redundant.

Whole test method asserts that every line of it succeeds, so using
`assertThatNoException` is redundant. Of course there could be
convention to clearly separate test setup and test assertions
(given-when-then), but that's not a convention we use, thuse
`assertThatNoException()` is redundant.
Copy link
Contributor

@ksobolew ksobolew left a comment

Choose a reason for hiding this comment

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

It seems you're arguing that assertThatNoException() serves no purpose at all, because if there is an exception, then the test will fail. The difference is that with this assertion it will be a "test failed" instead of "test error", the latter of which I think is reserved for "unexpected" failures outside of the scope of what we're testing. So I don't think this method is redundant in those tests - it makes the intent clearer.

(Unless I misunderstood the rationale!)

@findepi
Copy link
Member Author

findepi commented Feb 26, 2024

It seems you're arguing that assertThatNoException() serves no purpose at all, because if there is an exception, then the test will fail. The difference is that with this assertion it will be a "test failed" instead of "test error", the latter of which I think is reserved for "unexpected" failures outside of the scope of what we're testing.

I am not arguing with that.

See PR description. I admit that assertThatNoException() could be useful if it is established practice in a project. It just is not a practice in this project, so in this project it is not useful. Being verbose and not being useful (in this project) is sufficient condition for a removal.

@findepi findepi merged commit 786cacd into master Feb 26, 2024
96 of 98 checks passed
@findepi findepi deleted the findepi/remove-redundant-lambdas-from-test-0acb2d branch February 26, 2024 13:54
@github-actions github-actions bot added this to the 440 milestone Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants