Run tests only if connection with server is made. #1
Conversation
There was a problem hiding this comment.
Please improve your PR to (1) make the least amount of changes as possible (see the "Files changed" tab) -- any changes that were not necessary to remove the flakiness should NOT appear in this tab) and (2) update the description of your PR to more closely match the following. LALAYANG/hbase#1
Made the changes as you said. |
There was a problem hiding this comment.
How many times did you run this test in NonDex to observe it to fail without your changes?
How many times did you run this test in NonDex to observe it to not fail with your changes?
Does this test fail when you run it without NonDex for the same number of times?
| } |
There was a problem hiding this comment.
Please remove unnecessary changes. You may have added an empty line at the end of this file when there wasn't one.
There was a problem hiding this comment.
I don't know why, but the GitHub editor is weirdly adding a line automatically at the end of the file. Even if I remove the line, it appears again. It appears in the 'Files changed' page but doesn't appear when I just view the commit.
| } | ||
|
|
||
| @Test | ||
| public void testStatement() throws SQLException, IOException, | ||
| InterruptedException { | ||
| if (conn!=(Connection)null){ |
There was a problem hiding this comment.
Instead of this if statement, you may use the following instead.
https://junit.org/junit4/javadoc/4.12/org/junit/Assume.html#assumeNotNull(java.lang.Object...)
It gives an error once in about 100 times.
The change I have made doesn't fix the cause of the error i.e Hadoop ZookeeperCluster failing to start. So, it would still give an error once in about 100 times.
No. |
| @@ -29,6 +29,7 @@ | |||
| import org.junit.AfterClass; | |||
| import org.junit.BeforeClass; | |||
| import org.junit.Test; | |||
| import org.junit.Assume; | |||
| @@ -61,6 +62,7 @@ public static void beforeClass() throws Exception { | |||
|
|
|||
| @AfterClass | |||
| public static void afterClass() throws Exception { | |||
| Assume.assumeNotNull(conn); | |||
There was a problem hiding this comment.
Should you skip the test result if you cannot close the connection?
There was a problem hiding this comment.
I think I have made a mistake here. I should have maybe preserved the if-else check here so that the conn.close() line isn't executed if conn is null. 'Assume' isn't probably the right thing to use here.
|
You need a bigger sample size than 100 if it fails once out of 100. Can you please run the test and report back your results from (1) should show the test failing 10 or so times, while (2) should show the test failing 0 times (instead it should just be skipped/ignored). |
Okay, will do that. |
I ran the test 1000 times and it failed once. I used the following bash script for it:
Even with my changes, NonDex categorises the failure as an Error (not skipped/ignored). I think this is because the error props up in the @Beforeclass function. So, even though the test is skipped, that piece of code is executed, hence NonDex reports it. |
|
The bash script that you wrote does not seem to be running NonDex. When you run the test with your changes and NonDex, does the Surefire XML reports say that the test failed or that the test was skipped? |
The bash script is meant for counting number of failures among the 1000 runs. For running NonDeX, I used:
Neither. It categorises it as an 'Error'. |
|
Thanks for the clarifications. Please let me know when you have found a fix that makes the test pass when it is run with NonDex. |
Flaky Test
com.alibaba.wasp.jdbc.TestJdbcStatement
Why is it flaky?
The following call
TEST_UTIL.startMiniCluster(3)on line 57 passes the time-out time on occasions when the Apache ZooKeeper server fails to start. (This might happen due to reasons not under our control)Note: I ran this test several times with NonDex and it doesn't always give this error.
Non Dex log before making the changes:
Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 200.986 sec <<< FAILURE!
com.alibaba.wasp.jdbc.TestJdbcStatement Time elapsed: -1,573,282.773 sec <<< ERROR!
java.io.IOException: Waiting for startup of standalone server
at org.apache.hadoop.hbase.zookeeper.MiniZooKeeperCluster.startup(MiniZooKeeperCluster.java:180)
at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniZKCluster(HBaseTestingUtility.java:518)
at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniZKCluster(HBaseTestingUtility.java:507)
at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:623)
at org.apache.hadoop.hbase.HBaseTestingUtility.startMiniCluster(HBaseTestingUtility.java:575)
at com.alibaba.wasp.WaspTestingUtility.startMiniCluster(WaspTestingUtility.java:172)
at com.alibaba.wasp.WaspTestingUtility.startMiniCluster(WaspTestingUtility.java:144)
at com.alibaba.wasp.jdbc.TestJdbcStatement.beforeClass(TestJdbcStatement.java:57)
com.alibaba.wasp.jdbc.TestJdbcStatement Time elapsed: -1,573,282.773 sec <<< ERROR!
java.lang.NullPointerException: null
at com.alibaba.wasp.jdbc.TestJdbcStatement.afterClass(TestJdbcStatement.java:64)
What change have I made?
As the server didn't start, the connection with it couldn't be established and the object 'conn' (59) continued to have a 'null' value. This is the reason why we got a null pointer exception when the program reached 'conn.close()' (64) in the 'afterclass' function. I have added the if-else checks to ensure that the tests run only when the connection with server could be established.