> 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 > > John Speidel wrote: > 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.
Hi John, thank you for writing down to share details of you thoughts. I agree all 3 bullets you mentioned, but only in my second patch. My first intention and patch is much simpler and to fix a "wrong if statement". https://reviews.apache.org/r/26510/diff/1/ I still hesitant to drop the test coverage, but let me know if I don't still convince you, I will just set a @Ignore on the test. - jun ----------------------------------------------------------- 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 > >
