> 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. > > jun aoki wrote: > 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, The change that you made in patch 1 fixes a race condition where we could break out of the while loop if only one of the 2 conditiona are satisfied. But, I still have a serious concern that this test could/will at some point loop forever because one or both of the conditions are not satisfied. So, I will relunctantly agree to +1 the patch if - you add a relief valve to the test so that it fails out if the conditions aren't met in some period of time (something like 5 mins or we risk lots of intermittent failures) - a Jira is filed to properly fix these tests in the near future - we agree that the next time the test fails that we @Ignore it - 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 > >
