New behaviour of AbstractMetaDataContext.isEmpty()

[kabirkhan]

This code in AOPConstructorJoinpoint

 private boolean hasInstanceOrJoinpointMetaData(MetaData metaData)
 {
 if (metaData == null)
 {
 return false;
 }

 MetaData instanceMetaData = metaData.getScopeMetaData(CommonLevels.INSTANCE);
 if (instanceMetaData != null && instanceMetaData.isEmpty() == false)
 {
 return true;
 }
 //Check for method annotations
 return hasMethodMetaData(metaData);
 }

The instanceMetaData.isEmpty() check used to return false for the following case:

@Test
public class AnnotatedChild extends Base
{
 public static boolean childInvoked;
 public void childOnly()
 {
 childInvoked = true;
 }

 public void baseOverridden()
 {
 childInvoked = true;
 }
}

<deployment xmlns="urn:jboss:bean-deployer:2.0">

 ...
 <bean name="AnnotatedChild" class="org.jboss.test.microcontainer.matrix.AnnotatedChild"/>
</deployment>

It now returns true, this breaks loads of the aop/mc int tests.

The instanceMetaData.toString() method shows

org.jboss.metadata.spi.retrieval.MetaDataRetrievalToMetaDataBridge@1286c00{[JVM=THIS, CLASS=org.jboss.test.microcontainer.matrix.AnnotatedChild, INSTANCE=AnnotatedChild, WORK=1209895]}

and stepping into the isEmpty() method, the retrieval/MemoryMetaDataLoader with ScopeKey:

[INSTANCE=AnnotatedChild]

is empty, but it then calls isEmpty() on the parent MetaDataContext, which contains the following retrievals and associated SkopeKeys

MemoryMetaDataLoader [WORK=1209895]
MemoryMetaDataLoader [INSTANCE=AnnotatedChild]
AnnotatedElementMetaDataLoader [CLASS=org.jboss.test.microcontainer.matrix.AnnotatedChild]
MemoryMetaDataLoader [JVM=THIS]

The CLASS level one has annotations, so isEmpty() ends up returning false.

Is this a bug or should I be checking if it is empty another way?

[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

[alesj]

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?

[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

[adrian.brock]

The instanceMetaData.toString() method shows

org.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.

[pgier]

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.

[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

[adrian.brock]

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 a

List<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. ;-)

[alesj]

http://jira.jboss.com/jira/browse/JBMICROCONT-218

[alesj]

i.e. There should be a

List<MetaDataRetrieval> getScopedRetrievals(ScopeLevel level)

This should be enough:

 MetaData getScopeMetaData(ScopeLevel level);

looking at the code from the bridge

 public 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 this

 public 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 retrieval

 public 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?

[adrian.brock]

"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

[alesj]

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);

and

 if (getScope().getScopeLevel(level) != null)
 return this;

OK, no real harm, but the second one leaves AbstractMetaDataLoader clean from importing un-spi, un-retrieval packages. :-)

[alesj]

"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.

[pgier]

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.