HADOOP-18866. Refactor @Test(expected) with assertThrows #5982
+36
−23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
you might want to look at LambdaTestUtils, compare it to ScalaTest and see how it is being extended to handle futures and the like. outside that, we are moving towards AssertJ as our assertion language because it is better and extensible (see IOStatisticAssertions) |
Taher-Ghaleb
changed the title
|
Thanks @steveloughran for your response. I get your point, but I would like to get your input on the refactorings performed in this PR, and how such a practice is acceptable in general. In your opinion, why are those test cases still using I have created a Jira report and prefixed its id to the PR title. Thanks. |
|
i do think test age is an issue, and old code is suboptimal, but using test(expected) over our own intercept() or even assertThrows isn't something we'd do today. now, serious question for you: if a test is working, why does it need it maintenance? because there are other uses of engineering time, including
so, how would you justify the change, as "it's considered old fashioned" doesn't do it. if anyone was to go near old tests, i'd target things i really don't like
moving #3 onto intercept() would be better than worrying about "expected"; while #1 is critical for debugging a test report "testXYZ failed on Line 222". It's why I automatically veto any PR where new tests don't add details. Still disappointed that people still submit PRs where they don't do that. very bad style. we need a stylechecker rule for it, at least for new code, |
|
Thanks @steveloughran for your detailed response. I got your points and gained various insights. The issues highlighted in your comment are also part of our research, which we also noticed they exist in various repositories. |
|
ok. now, one thing to consider there is: what stylecheckers etc can we use to stop new prs coming in which don't do all of this, or lose stack traces when validating caught exceptions? As all to often, the work of getting a PR in is the time spent teaching people how to write tests that meet my expectations (for me) and the time spent waiting for review, making the changes and repeating (for them). see #6003 as an example. if we have the CI tooling automatically imposing policies on tests, then everyone's time is better used. now, we do run checkstyle on all PRs, so if you have suggestions about how to do it there, or other maven plugins (better yet, prs with tests) then I'd be very happy. put differently: lets automate enforcing test quality on new submissions before worrying about old tests which appear to work ok |
|
Right, I agree. That is the ultimate goal of our research, to automatically improve test quality from various perspectives. Thanks @steveloughran. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I am working on research that investigates test smell refactoring in which we identify alternative implementations of test cases, study how commonly used these refactorings are, and assess how acceptable they are in practice.
For example, the smell in TestSSLFactory.java occurs when exception handling can alternatively be implemented using assertion rather than annotation: using
assertThrows(IllegalStateException.class, () -> {...});instead of@Test(expected = IllegalStateException.class).While there are many cases like this, we aim in this pull request to get your feedback on this particular test smell and its refactoring. Thanks in advance for your input.