-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-13798 TestFromClientSide* don't close the Table #193
Conversation
💔 -1 overall
This message was automatically generated. |
Well the unit tests hopefully are just a bad CI host. Let me go relaunch those. |
💔 -1 overall
This message was automatically generated. |
much more reasonable! @anmolnar can you clean up the checkstyle complaints? looks like mostly improper line length handling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh. TestFromClientSide.testNull should be several tests instead of mixing up all of those "should/should not throw" conditions. Not your problem here, but filing a follow-on jira to make note of the need would be good.
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide3.java
Show resolved
Hide resolved
@busbey Agreed. Similarly it would be nice to split |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Rebased on latest master. I expect lots of checkstyle warnings again, so probably still not ready to commit. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@busbey @petersomogyi |
@anmolnar The build only shows newly introduced violations. Let's wait for the latest build if they got resolved. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@petersomogyi Looks like we have a green build finally? |
The latest verification looks good. I'm retriggering it once more. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
I refactored test cases
TestFromClientSide
andTestFromClientSide3
to use try-with-resources for classes which are auto-closeable: Table, Scanner, etc.TestFromClientSide
Refactored all individual tests to close closeable resources properly. Doing this in setUp/tearDown methods is not beneficial, because most tests create their tables with custom parameters.
TestFromClientSide3
Initialisation of tables are pretty much the same in these tests, so I ended up writing setUp/tearDown methods. For tests which need custom initialisation I delete the created table first and let them create custom one instead.