> On Oct. 10, 2014, 6:49 p.m., John Speidel wrote: > > I don't feel comfortable with this test being merged as is. > > If you need to resolve this before a proper fix is implemented, please > > ignore the test via @Ignore and file a Jira to properly fix the test. > > jun aoki wrote: > Hi John, I hear your concern and agree with you two that the test should > not use asynchronous. > But the test has been this way since 2012 and I am hesitent to drop the > test coverage by adding @Ignore. > > https://github.com/apache/ambari/commits/trunk/ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java
Jun, first off thanks for taking the time to analyze this intermittent test failure. I spent the last hour looking at this test. My conclusion is that this is a very poorly written test from it's orgin. It is way too coarse grained, uses the NPE as an intentional error mechanism and expects several iterations of doWork() to complete based on the states returned by the test mocks. Your changes, although they result in a passing test now, have several issues. - They circumvent the run method which under the case being tested, catches the NPE and makes some state changes - The changes are not written in a way that guarantees a deterministic behavior: "while(count < 10)" is a big red flag - The changes will make the test VERY confusing to anybody who needs to fix these tests in the future All of the tests in this class need to be rewrittn in a way that is deterministic and doesn't require obscure NPE exceptions or multiple interations of doWork(). I still feel it is better to @Ignore the test, or all of them in this class for that matter, and have them fixed properly when possible. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26510/#review56194 ----------------------------------------------------------- On Oct. 9, 2014, 10:05 p.m., jun aoki wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26510/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2014, 10:05 p.m.) > > > Review request for Ambari and Yusaku Sako. > > > Bugs: AMBARI-7622 > https://issues.apache.org/jira/browse/AMBARI-7622 > > > Repository: ambari > > > Description > ------- > > Tweaked the waiting condition upon ActionScheduler > > > Diffs > ----- > > > ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java > a20f252 > > Diff: https://reviews.apache.org/r/26510/diff/ > > > Testing > ------- > > > Thanks, > > jun aoki > >
