Thanks! > Sveltin, if you’re up for it I can probably use this as an excuse to pull some Arquillian devs over for some fun hacking :)
I'm not that knowledgeable in Arquillian, but I'll help with whatever I can :) > I also say +1. I’d love to see the layers thinned out of the Arquillian test, but this could be done in another PR I removed the servlet, maybe I can remove the bean as well. It's not needed as long as the test is executed against the IvmContext. Kind regards, Svetlin 2017-07-19 10:53 GMT+03:00 Jean-Louis Monteiro <jlmonte...@tomitribe.com>: > Looks like we are all ok. I'll proceed and merge. > Thanks > > Le 19 juil. 2017 00:56, "David Blevins" <david.blev...@gmail.com> a écrit > : > > I also say +1. I’d love to see the layers thinned out of the Arquillian > test, but this could be done in another PR. > > Sveltin, if you’re up for it I can probably use this as an excuse to pull > some Arquillian devs over for some fun hacking :) > > > -- > David Blevins > http://twitter.com/dblevins > http://www.tomitribe.com > > > On Jul 14, 2017, at 5:14 AM, Romain Manni-Bucau <rmannibu...@gmail.com> > wrote: > > > > looks good to apply to me > > > > > > 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-14 12:55 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. > com > >: > > > >> 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.zarev@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 > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> >
