-
1. Re: New behaviour of AbstractMetaDataContext.isEmpty()
kabirkhan Nov 15, 2007 1:02 PM (in response to kabirkhan)The tests are here:
$cd aop-mc-int $ mvn surefire-report:report -Pant-tests-weave
There are quite many tests, but the ones that fail now are mostly in the org.jboss.test.microcontainer.matrix.mc.test package.
If you want to run just one in IDEA, you could run org.jboss.test.microcontainer.matrix.mc.test.BaseNotAdvisedChildAdvisedAndProxyTestCase, making sure that you turn on weaving by passing in the following system property-Djava.system.class.loader=org.jboss.aop.standalone.SystemClassLoader
-
2. Re: New behaviour of AbstractMetaDataContext.isEmpty()
alesj Nov 15, 2007 1:07 PM (in response to kabirkhan)I'll have a look at that, since I wrote that. ;-(
I remember having a todo on tests, but forgot, since no exception occurred.
Btw: why are these tests excluded from maven tests? -
3. Re: New behaviour of AbstractMetaDataContext.isEmpty()
kabirkhan Nov 15, 2007 1:09 PM (in response to kabirkhan)These tests are not being run automatically, I've asked before why on another thread. I think Paul mentioned that the reason for this is that "they take a long time". Anyway, looking at the code change that caused this they would have been broken for about 2 months now, so I think they should be enabled.
I agree that they don't necesarily need to be run when doing a local "mvn install", but they should be enabled when being run on Hudson.
The tests are not part of the normal profile, to run the aop-mc-int tests you have to$cd aop-mc-int $mvn surefire-report:report -Ptests-no-weave $mvn surefire-report:report -Ptests-no-weave-secure $mvn surefire-report:report -Pant-tests-weave $mvn surefire-report:report -Pant-tests-weave-secure
-
4. Re: New behaviour of AbstractMetaDataContext.isEmpty()
adrian.brock Nov 15, 2007 1:25 PM (in response to kabirkhan)"kabir.khan@jboss.com" wrote:
The instanceMetaData.toString() method showsorg.jboss.metadata.spi.retrieval.MetaDataRetrievalToMetaDataBridge@1286c00{[JVM=THIS, CLASS=org.jboss.test.microcontainer.matrix.AnnotatedChild, INSTANCE=AnnotatedChild, WORK=1209895]}
That's clearly wrong. The invocation of:
MetaData instanceMetaData = metaData.getScopeMetaData(CommonLevels.INSTANCE);
should be giving you back a MetaData object that only contains the
INSTANCE=AnnotatedChild.
The bug is fairly obvious.
The single scope metadata shouldn't have a parent! :-)public MetaData getScopeMetaData(ScopeLevel level) { if (level == null) throw new IllegalArgumentException("Null scope level"); if (retrieval instanceof MetaDataContext) { MetaDataContext context = (MetaDataContext)retrieval; List<MetaDataRetrieval> matchingRetrievals = new ArrayList<MetaDataRetrieval>(); List<MetaDataRetrieval> localRetrievals = context.getLocalRetrievals(); for (MetaDataRetrieval localRetrieval : localRetrievals) { ScopeKey scopeKey = localRetrieval.getScope(); if (scopeKey.getScopeLevel(level) != null) matchingRetrievals.add(localRetrieval); } if (matchingRetrievals.isEmpty() == false) - return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(context, matchingRetrievals)); + return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(null, matchingRetrievals)); } else if (retrieval.getScope().getScopeLevel(level) != null) return new MetaDataRetrievalToMetaDataBridge(new AbstractMetaDataContext(retrieval)); return null; }
But on a related issue, I don't like the fact that this logic is in the bridge.
The bridge should be just delegating the request to the retrieval.
i.e. a plain retrieval will just match its scope level while a context
will have to filter its local retrievals. It's more OO. -
5. Re: New behaviour of AbstractMetaDataContext.isEmpty()
pgier Nov 15, 2007 1:26 PM (in response to kabirkhan)If you want, I can turn them all on by default. Or just turn them all on in hudson. I'll have to check with Prabhat to see if we can configure hudson to display all the test results nicely since they aren't in the normal location.
-
6. Re: New behaviour of AbstractMetaDataContext.isEmpty()
kabirkhan Nov 15, 2007 1:27 PM (in response to kabirkhan)I think you could perhaps run each aop-mc-int profile as a separate hudson run, and make them dependent on the main MC run, or the main MC run triggers these separate runs. I am not sure exactly how that works, but Ryan Campbell did something like that for the aop tests
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=122864 -
7. Re: New behaviour of AbstractMetaDataContext.isEmpty()
adrian.brock Nov 15, 2007 1:28 PM (in response to kabirkhan)"adrian@jboss.org" wrote:
But on a related issue, I don't like the fact that this logic is in the bridge.
The bridge should be just delegating the request to the retrieval.
i.e. a plain retrieval will just match its scope level while a context
will have to filter its local retrievals. It's more OO.
i.e. There should be aList<MetaDataRetrieval> getScopedRetrievals(ScopeLevel level)
on the retrieval interface and the two branches of the if statement
moved into the relevant abstract implementation (context or retrieval).
P.S. I'd also guess that the testcase for this functionality isn't very good. ;-) -
9. Re: New behaviour of AbstractMetaDataContext.isEmpty()
alesj Nov 15, 2007 3:28 PM (in response to kabirkhan)"adrian@jboss.org" wrote:
i.e. There should be aList<MetaDataRetrieval> getScopedRetrievals(ScopeLevel level)
This should be enough:MetaData getScopeMetaData(ScopeLevel level);
looking at the code from the bridgepublic MetaData getScopeMetaData(ScopeLevel level) { if (level == null) throw new IllegalArgumentException("Null scope level"); MetaDataRetrieval scopedRetrieval = retrieval.getScopedRetrieval(level); if (scopedRetrieval != null) return new MetaDataRetrievalToMetaDataBridge(scopedRetrieval); return null; }
where in context I have thispublic MetaDataRetrieval getScopedRetrieval(ScopeLevel level) { List<MetaDataRetrieval> matchingRetrievals = new ArrayList<MetaDataRetrieval>(); List<MetaDataRetrieval> localRetrievals = getLocalRetrievals(); for (MetaDataRetrieval localRetrieval : localRetrievals) { ScopeKey scopeKey = localRetrieval.getScope(); if (scopeKey.getScopeLevel(level) != null) matchingRetrievals.add(localRetrieval); } if (matchingRetrievals.isEmpty() == false) return new AbstractMetaDataContext(null, matchingRetrievals); return null; }
and in retrievalpublic MetaDataRetrieval getScopedRetrieval(ScopeLevel level) { if (getScope().getScopeLevel(level) != null) return new AbstractMetaDataContext(this); return null; }
A question here. :-)
Why do I need to wrap retrieval in context in this last code snippet?
OK, I know this was my old code :-), but can't see it now why I wrapped it.
Perhaps a port of previous code from AOPConstructorJoinpoint? -
10. Re: New behaviour of AbstractMetaDataContext.isEmpty()
adrian.brock Nov 15, 2007 3:44 PM (in response to kabirkhan)"alesj" wrote:
A question here. :-)
Why do I need to wrap retrieval in context in this last code snippet?
OK, I know this was my old code :-), but can't see it now why I wrapped it.
Perhaps a port of previous code from AOPConstructorJoinpoint?
It looks redundant to me, but it does no harm. ;-)
The same is true for the other one if matchingRetrievals.size() == 1 -
11. Re: New behaviour of AbstractMetaDataContext.isEmpty()
alesj Nov 15, 2007 6:04 PM (in response to kabirkhan)"adrian@jboss.org" wrote:
It looks redundant to me, but it does no harm. ;-)
So you're saying this is the same - for AbstractMetaDataLoader:if (getScope().getScopeLevel(level) != null) return new AbstractMetaDataContext(this);
andif (getScope().getScopeLevel(level) != null) return this;
OK, no real harm, but the second one leaves AbstractMetaDataLoader clean from importing un-spi, un-retrieval packages. :-) -
12. Re: New behaviour of AbstractMetaDataContext.isEmpty()
alesj Nov 16, 2007 4:08 PM (in response to kabirkhan)"pgier" wrote:
If you want, I can turn them all on by default. Or just turn them all on in hudson. I'll have to check with Prabhat to see if we can configure hudson to display all the test results nicely since they aren't in the normal location.
Could you make this optional?
By default only on Hudson.
But if we used some sys property, then they would also pop-up in local tests. -
13. Re: New behaviour of AbstractMetaDataContext.isEmpty()
pgier Nov 16, 2007 5:34 PM (in response to kabirkhan)I made the aop-mc-int tests on by default now. I tested them in hudson here:
http://hudson.qa.jboss.com/hudson/job/Microcontainer/org.jboss.microcontainer$jboss-aop-mc-int/730/
They can be turned off using the property "aop.tests.skip"
So from the microcontainer root or within the aop module:mvn -Daop.tests.skip install
I also added some comments about usage to top of the aop-mc-int pom.
I can make them off by default it that is better by just reversing the way that the property works.