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.
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.za...@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 > > > > > > > > >
