[
https://issues.apache.org/jira/browse/HBASE-18902?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16186761#comment-16186761
]
Appy edited comment on HBASE-18902 at 9/30/17 12:59 AM:
--------------------------------------------------------
Am not clear what are you trying to shoot for here.
There are three things
1. Using single HBaseTestingUtility instance
2. Using single test function
3. Using single cluster
At first, the motivation was "we should only instantiate HBaseTestingUtility
once.". But i don't see any advantage of it. Less objects? We have never
worried about GC in tests. Reduce test times? Creating 2 less object doesn't
save anything.
If am missing something there, please let me know. I'd like to learn and adopt
better testing patterns moving forward.
But if there's no good reason for single HBaseTestingUtility, then I'd prefer
the current approach of creating new one everytime, because i think it's a
better testing pattern. Why?
1. cleaning up confs manually is very bug prone, and the effects can be very
magnified. One test messes up, and the whole test class can have buggy tests.
On the other hand, using new object every time is very clean and simple way of
testing.
2. cleaning up confs manually at the end is ugly.
Your [second
suggestion|https://issues.apache.org/jira/browse/HBASE-18902?focusedCommentId=16185089&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16185089]
had more weight in the motivation with "we start / stop mini cluster once".
However
- your assumption that i did it coz i couldn't separate three calls was not
good one. But it's fine.
- "This way only one subtest" is imo the wrong direction. Unit tests are better
if they test different code paths in isolation.
bq. Shouldn't the change leading to the above outcome be immediately flagged
and corrected ?
If the answer to "[if one functionality is broken], that'd have affected other
tests." is "if the functionality is broken, won't we correct it immediately
?", then
1. Why do have flaky tests? After all, the flakyness can be due to wrong
functionality.
2. Why don't we merge all test functions in TestAdmin into one test. The
suggestion of merging tests and the argument for it is applicable for TestAdmin
too. But does't sound right, right? That's what i meant when i said "The
reason was very general - test the 3 CP services envs in isolation."
I think if at any point you made a complete coherent case with supporting
details, maybe something like the following, i'd have been onboard with
whatever patch you had.
"CoprocessorService hasn't changed in long time (since 2012). Is it likely to
change again soon? If not, can we merge the tests into one? It'll hurt
debuggability and is against general testing principles, but if don't touch
this piece of code often, then it maybe better to save in runtime here. Setting
up cluster once instead of three times will bring down runtime from 24 s to
12s, which is substantial save from a single test."
The answer to those questions are, No and Yes.
So please go ahead and refactor the tests into one.
In the end, while this discussion lead to the right solution, i'd say that it
wasn't fun. I mean, you wanted to see it through, great, but when proposing
something, please make a sound case for it. Here, I had to do all the work, add
in details, and make the case for you!
was (Author: appy):
Am not clear what are you trying to shoot for here.
There are three things
1. Using single HBaseTestingUtility instance
2. Using single test function
3. Using single cluster
At first, the motivation was "we should only instantiate HBaseTestingUtility
once.". But i don't see any advantage of it. Less objects? We have never
worried about GC in tests. Reduce test times? Creating 2 less object doesn't
save anything.
If am missing something there, please let me know. I'd like to learn and adopt
better testing patterns moving forward.
But if there's no good reason for single HBaseTestingUtility, then I'd prefer
the current approach of creating new one everytime, because i think it's a
better testing pattern. Why?
1. cleaning up confs manually is very bug prone, and the effects can be very
magnified. One test messes up, and the whole test class can have buggy tests.
On the other hand, using new object every time is very clean and simple way of
testing.
2. cleaning up confs manually at the end is ugly.
Your [second
suggestion|https://issues.apache.org/jira/browse/HBASE-18902?focusedCommentId=16185089&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16185089]
had more weight in the motivation with "we start / stop mini cluster once".
However
- your assumption that i did it coz i couldn't separate three calls was not
good one. But it's fine.
- "This way only one subtest" is imo the wrong direction. Unit tests are better
if they test different code paths in isolation.
bq. Shouldn't the change leading to the above outcome be immediately flagged
and corrected ?
If the answer to "[if one functionality is broken], that'd have affected other
tests." is "if the functionality is broken, won't we correct it immediately
?", then
1. Why do have flaky tests? After all, the flakyness can be due to wrong
functionality.
2. why don't we merge all test cases in TestAdmin into one test. Does't sound
right, right? That's what i meant when i said "The reason was very general -
test the 3 CP services envs in isolation."
I think if at any point you made a complete coherent case with supporting
details, maybe something like the following, i'd have been onboard with
whatever patch you had.
"CoprocessorService hasn't changed in long time (since 2012). Is it likely to
change again soon? If not, can we merge the tests into one? It'll hurt
debuggability and is against general testing principles, but if don't touch
this piece of code often, then it maybe better to save in runtime here. Setting
up cluster once instead of three times will bring down runtime from 24 s to
12s, which is substantial save from a single test."
The answer to those questions are, No and Yes.
So please go ahead and refactor the tests into one.
In the end, while this discussion lead to the right solution, i'd say that it
wasn't fun. I mean, you wanted to see it through, great, but when proposing
something, please make a sound case for it. Here, I had to do all the work, add
in details, and make the case for you!
> TestCoprocessorServiceBackwardCompatibility fails in master branch
> ------------------------------------------------------------------
>
> Key: HBASE-18902
> URL: https://issues.apache.org/jira/browse/HBASE-18902
> Project: HBase
> Issue Type: Test
> Reporter: Ted Yu
> Assignee: Ted Yu
> Attachments: 18902.v1.txt, 18902.v2.txt, 18902.v3.txt,
> HBASE-18902.master.001.patch, HBASE-18902.master.001.patch
>
>
> Starting mini cluster in subtests without shutting down resulted in:
> {code}
> java.lang.IllegalStateException: A mini-cluster is already running
> at
> org.apache.hadoop.hbase.coprocessor.TestCoprocessorServiceBackwardCompatibility.testCoprocessorServiceLoadedByRegion(TestCoprocessorServiceBackwardCompatibility.java:109)
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
