The complete current QA test suite passed with this change included. Also, I ran the "txnmanager" category, all tests pass except GetStateTest.td (which hangs), but this one was already behaving that way without this fix.
I'd say commit it. PS: if anyone would be able to debug GetStateTest and figure out what is wrong with it, we could add another 40-something tests (covering mahalo impl and the transactionmanager spec) to our QA run on Hudson (txnmanager category). 2010/9/27 Jonathan Costers <[email protected]> > This fix actually made the test pass for me for the first time. > I'm running the complete suite against it (all the categories that are > executed on Hudson) now. > > > 2010/9/27 Tom Hobbs <[email protected]> > > Good find! >> >> > There is an issue of whether there is anything else that depends on >> getting a RuntimeException rather than a >> > checked exception, so it needs a full regression test. >> >> If other tests rely on the incorrect behaviour of TxnManagerImpl then I'd >> say that the other tests are wrong by definition. If this change breaks >> other tests then those other tests need appropriate fixes. I'd check in >> the >> fix now, if the build starts breaking in new and exciting places then the >> cause of those breakages should be easy to diagnose. >> >> >> On Mon, Sep 27, 2010 at 11:18 AM, Patricia Shanahan <[email protected]> wrote: >> >> > I've diagnosed com/sun/jini/test/impl/mahalo/RandomStressTest.td, but >> may >> > not have time to regression test and check in the fix. >> > >> > The test failure is due to an error in >> > com/sun/jini/mahalo/TxnManagerImpl.java. It throws a >> NullPointerException >> > from abort if the transaction is not known to transaction management. >> That >> > case can arise, even for a formerly valid transaction, due to it having >> > already been committed or aborted, so it is timing dependent and more >> likely >> > to happen under stress than when doing more controlled tests. >> > >> > The TransactionManager interface specifies UnknownTransactionException >> for >> > that case. The test catches UnknownTransactionException and treats it as >> a >> > valid, non-error result. NullPointerException is not caught until it >> gets >> > back to the TaskManager's thread run method, which turns it into a >> logged >> > WARNING. The uncaught exception prevents the task from reporting >> completion >> > back to the test, leading to the timeout and test failure. >> > >> > There is an issue of whether there is anything else that depends on >> getting >> > a RuntimeException rather than a checked exception, so it needs a full >> > regression test. >> > >> > I also recommend reducing the timeout from 30 minutes. When the test >> > succeeds, it takes less than a minute. >> > >> > Here is a patch to change the exception: >> > >> > Index: src/com/sun/jini/mahalo/TxnManagerImpl.java >> > =================================================================== >> > --- src/com/sun/jini/mahalo/TxnManagerImpl.java (revision 1000286) >> > +++ src/com/sun/jini/mahalo/TxnManagerImpl.java (working copy) >> > @@ -933,7 +933,7 @@ >> > } >> > >> > } else { >> > - throw new NullPointerException("No such transaction >> ["+id+"]"); >> > + throw new UnknownTransactionException("No such transaction >> > ["+id+"]"); >> > >> > } >> > >> > >> > Patricia >> > >> > >> > On 9/25/2010 7:17 AM, Jonathan Costers wrote: >> > >> >> I actually did come across a test that may be interesting to run for >> these >> >> changes ... >> >> >> >> com/sun/jini/test/impl/mahalo/RandomStressTest.td >> >> >> >> I have not been able to get this test to pass on any of my machines or >> VMs >> >> ... >> >> It seems to make heavy use of TaskManager and RetryTask. >> >> It can take a while to finish as well, so beware. >> >> >> >> You think this may be a good one? >> >> >> >> you can run it if you change to the<river>/qa directory and execute: >> >> ant -Drun.tests=com/sun/jini/test/impl/mahalo/RandomStressTest.td >> >> run-tests >> >> >> >> or similarly in an IDE, set property >> >> run.tests=com/sun/jini/test/impl/mahalo/RandomStressTest.td >> >> and run the run-tests target >> >> >> >> make sure everything is built first, of course. >> >> >> >> 2010/9/21 Patricia Shanahan<[email protected]> >> >> >> >> I'm testing my new TaskManager the , but I have some anomalies. It >> would >> >>> help me to get some more testing of >> >>> >> >>> >> https://svn.apache.org/repos/asf/incubator/river/jtsk/skunk/patsTaskManagerdoneinother >> WindowsXP environments. >> >>> >> >>> Both the head revision and revision 998737 need to be tested. >> Revision >> >>> 998737 is the one I plan to merge into the trunk. It changes the >> >>> interface >> >>> between TaskManager and its callers, with minimal changes to >> TaskManager. >> >>> >> >>> It is important that it be tested widely, because it affects a lot of >> >>> critical classes, and would be difficult to back out. >> >>> >> >>> The head revision drops in a revised TaskManager. It should be easy to >> >>> back >> >>> out if necessary. >> >>> >> >>> Patricia >> >>> >> >>> >> >> >> > >> > >
