Hi, Do you have any comments ? Do you thing that something is missing or maybe that something additional is needed ?
Kind regards, Svetlin 2017-07-11 15:07 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: > 2017-07-11 12:38 GMT+02:00 Svetlin Zarev <svetlin.angelov.za...@gmail.com > >: > > > I've added several JUnit test cases in openejb-core that should verify > > IvmContext.list() behaviour, yet I'll feel safer if we keep the > arquillian > > test as well. > > > > +1, both are needed > > > > > > 2017-07-11 10:10 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: > > > > > side note: embedded (not tomcat based) testing is needed to ensure we > > don't > > > break but doesn't fully test the ivmcontext code because the federation > > is > > > different so guess starting with tomcat is not that bad. > > > > > > > > > Romain Manni-Bucau > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog > > > <http://rmannibucau.wordpress.com> | Github <https://github.com/ > > > rmannibucau> | > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > > > <https://javaeefactory-rmannibucau.rhcloud.com> > > > > > > 2017-07-11 8:28 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. > com > > >: > > > > > > > Hi David, > > > > > > > > Thank you for sparing some time to look into my PR ! > > > > > > > > > I’m not sure I can see how the test actually works > > > > > > > > The issue is that IvmContext.list() returns objects that are not > really > > > > bound into the listed context. For instance if you run the MCVE > > attached > > > to > > > > the jira ticket you'll see that it returns [1]. There you can see > that > > > > TestEJB is bound in "java:" (and even appears several times!) or that > > > > "java:global" is bound in "java:module". But if you try to look up > > those > > > > entries, the lookup fails with a NameNotFoundException, because all > > these > > > > references are not really bound there. So the test recursively lists > > all > > > > contexts in the JNDI tree and tries to lookup up every name-class > pair > > > that > > > > is returned. If the lookup fails, it means that list() has returned > > > > something that is not really there. You can compare [1] and [2] > 9after > > > the > > > > fix) to see the difference in list()'s behaviour > > > > > > > > > Is there a test for listBindings? It’s mentioned as also broken, > > but I > > > > didn’t see a test for it. > > > > > > > > IvmContext.listBindings() and IvmContext.list() use the very same > > > > IvmContext.MyNamingEnumeration abstract class and share the very same > > > logic > > > > to traverse the naming tree. I didn't write test for it because they > > > share > > > > the same code, but I can easily modify it to run aginst both methods. > > > > > > > > > What is the PrintWriter used for? It seems it could be useful to > > > assert > > > > it prints what we expect. Not sure if that is there and I am missing > > it. > > > > > > > > I thought it would be helpful to be able to see what went wrong if > the > > > test > > > > fails. The IvmContextTest class collects the output from the > servlet's > > > > output stream (the print writer) and if the test fails it prints it > in > > > the > > > > console. > > > > > > > > > There is an IvmContextTest, could we put the test there? > > > > > > > > That was my initial idea, but AppComposer's naming tree is very > > different > > > > that tomee's. For instance it does not have the "app", "global", etc > > top > > > > level contexts and my fix has special code for such top-level > > contexts. I > > > > also was not bale to bind any env-entries to my EJB (I'm not really > > > > familiar with how to write a proper appcomposer test, so I guess I > > didn't > > > > do something that I should have.). The env-entries are needed just to > > > > create a couple of branches to the tree in order to test if > > > > MyNamingEnumeration.isMyChild() works correctly > > > > > > > > > so we could potentially skip the plumbing of the > test->servlet->ejb. > > > > > > > > I'll look into it. I also have a few ideas for additioanl tests. > > > > > > > > [1] https://gist.github.com/SvetlinZarev/ > > 6b9377fe05b7887d681491c3e9e538 > > > 21 > > > > [2] https://gist.github.com/SvetlinZarev/ > > db3b59404198cd494b45b23db7129e > > > dd > > > > > > > > Bets regards, > > > > Svetlin > > > > > > > > 2017-07-11 2:28 GMT+03:00 David Blevins <david.blev...@gmail.com>: > > > > > > > > > Hi Svetlin! > > > > > > > > > > This is an awesome catch. Also, my apologies for the time you had > to > > > > > spend in that code. It literally hasn’t changed much since > 1999/2000 > > > and > > > > > it shows. :) > > > > > > > > > > Looking at the PR I’m not sure I can see how the test actually > works. > > > > > Here’s what I can follow, any gaps you can fill in are excellent: > > > > > > > > > > The call chain as I can see it: > > > > > > > > > > - IvmContextTest.testListContextTree > > > > > - IvmContextTest.validateTest("testListContextTree”) > > > > > [network call] > > > > > - IvmContextServlet.doGet # invokes itself via reflection, > > returns > > > > > “true” if no exception > > > > > [reflection call] > > > > > - IvmContextServlet.testListContextTree > > > > > - NamingBean.verifyListContext # throws exception if listContext > > > > returns > > > > > false > > > > > - NamingBean.listContext > > > > > > > > > > Looks like the essence of the test is in NamingBean.listContext. > > > Inside > > > > > it looks like the heart of it is that if we can’t lookup an item > > > listed, > > > > we > > > > > return false. > > > > > > > > > > Not sure if I got it perfectly, so definitely say so :) > > > > > > > > > > Couple questions: > > > > > > > > > > - Is there a test for listBindings? It’s mentioned as also > broken, > > > but > > > > > I didn’t see a test for it. > > > > > > > > > > - What is the PrintWriter used for? It seems it could be useful > to > > > > > assert it prints what we expect. Not sure if that is there and I > am > > > > > missing it. > > > > > > > > > > - There is an IvmContextTest, could we put the test there? > > > > > https://github.com/apache/tomee/blob/master/container/ > > > > > openejb-core/src/test/java/org/apache/openejb/ivm/naming/ > > > > > IvmContextTest.java > > > > > > > > > > On the last one I noted the patched code mentions Tomcat, so maybe > it > > > > does > > > > > have to be Arquillian based. If so, maybe we could still trim out > > some > > > > of > > > > > those layers. I think Arquillian has the ability to execute remote > > > code, > > > > > so we could potentially skip the plumbing of the > test->servlet->ejb. > > > > > > > > > > Regardless, looking IvmContext always makes my brain hurt so > > incredibly > > > > > well done surviving it. You’re clearly quite sharp. > > > > > > > > > > > > > > > -David > > > > > > > > > > > > > > > > On Jul 10, 2017, at 4:08 AM, Svetlin Zarev > > > > <svetlin.angelov.zarev@gmail. > > > > > com> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > I'd like to propose the following fix: [1]. It fixes > > > > > > IvmContext.list()/listBindings(). There are several issues that > > are > > > > > being > > > > > > addressed: > > > > > > > > > > > > * MyNamingEnumeration.gatherNodes() adds the wrong federated > > context > > > > > > entries in the result set. It should add the nodes belonging to > the > > > > > > "initiallyRequestedNode", otherwise in some cases we are adding > > nodes > > > > > from > > > > > > two different parents (in other words we are mixing two different > > > > > > sub-contexts), which is incorrect. > > > > > > > > > > > > * MyNamingEnumeration.isMyChild() considers entries that are NOT > > > > > children > > > > > > to the "parent" tree as such - the original code was using > > > > "parentTree", > > > > > to > > > > > > check if a given node is a child to another one, which is wrong. > > The > > > > > > "parentTree" relationship indicates only the physical layout of > the > > > > tree, > > > > > > and NOT the relationship between the sub-contexts and their > > entries. > > > > > Hence > > > > > > it considers for instance "java:global" to be a child of > > > "java:module" > > > > > > which is obviously incorrect. The relationship between a context > > and > > > > the > > > > > > bound entries is denoted by the "parent" node. So when listing a > > > > context > > > > > > isMyChild should rely only on the "parent" node. There is one > > > > exception - > > > > > > the top level contexts (app, global, etc) which do not have a > > parent. > > > > > > > > > > > > * Wrong parentNode is passed as argument to gatherNodes in case > we > > > are > > > > > > listing the context for any "IvmContext != this. When we call > > > > > > IvmContext.list(), it looks up the relevant IvmContext and tries > to > > > > > build a > > > > > > NamingEnumeration for its sub-tree. So far so good, but the > looked > > up > > > > > > context might be different than the context on which we called > > > list(). > > > > If > > > > > > that's the case, the wrong NameNode is passed as "parent" to > > > > > gatherNodes() > > > > > > and as a result some nodes are not listed. > > > > > > > > > > > > PS: This issue is relevant for tomee 1.7.x as well. i noticed it > > does > > > > not > > > > > > correctly list the naming tree as well. It also does not list the > > > > > federated > > > > > > contexts which was implemented in [3]. > > > > > > > > > > > > [1] https://github.com/apache/tomee/pull/88 > > > > > > [2] https://issues.apache.org/jira/browse/TOMEE-2087 > > > > > > [3] https://github.com/apache/tomee/pull/81 > > > > > > > > > > > > Best regards, > > > > > > Svetlin > > > > > > > > > > > > > > > > > > > >
