Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-19 Thread Svetlin Zarev
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, bu

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-19 Thread Jean-Louis Monteiro
Looks like we are all ok. I'll proceed and merge. Thanks Le 19 juil. 2017 00:56, "David Blevins" 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 pu

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-18 Thread David Blevins
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://

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-14 Thread Romain Manni-Bucau
looks good to apply to me Romain Manni-Bucau @rmannibucau | Blog | Old Blog | Github | LinkedIn | JavaEE Factory

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-14 Thread Svetlin Zarev
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 : > 2017-07-11 12:38 GMT+02:00 Svetlin Zarev >: > > > I've added several JUnit test cases in openejb-core tha

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-11 Thread Romain Manni-Bucau
2017-07-11 12:38 GMT+02:00 Svetlin Zarev : > 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 : > > > side n

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-11 Thread Svetlin Zarev
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 : > side note: embedded (not tomcat based) testing is needed to ensure we don't > break but

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-11 Thread Romain Manni-Bucau
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 | Blog

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-11 Thread Svetlin Zarev
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

Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-10 Thread David Blevins
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

[PR] TOMEE-2087 IvmContext.list() does not correctly list the context content

2017-07-10 Thread Svetlin Zarev
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 "initiallyRequeste