Why does framework/component-load.xml does not include "base"

[Bob Morley]

It appears that our current unit test run excludes all of the "base" unit testers.  Two reasons I have found ...

1) basetests.xml was coded as a test-case but attempted to have multiple junit-test-suite elements in it (it is only allow to have one).  Changing this to a test-group would ensure that they are loaded.

2) ComponentConfig.getAllComponents was not including the "base" component.  Tracked this down to framework/component-load.xml not including it.  My guess is that there is probably a reason for this; but I don't know what it could be.

This may also be the reason why the code coverage metrics are inaccurate when you run from the top.  I have executed on my dev machine w/o code coverage, and the tests all passed increasing the # from 216 to 269.

If there is no known reason, I would like to submit these two changes as a patch.

[Adam Heath-2]

Bob Morley wrote:

> It appears that our current unit test run excludes all of the "base" unit
> testers.  Two reasons I have found ...
>
> 1) basetests.xml was coded as a test-case but attempted to have multiple
> junit-test-suite elements in it (it is only allow to have one).  Changing
> this to a test-group would ensure that they are loaded.
>
> 2) ComponentConfig.getAllComponents was not including the "base" component.
> Tracked this down to framework/component-load.xml not including it.  My
> guess is that there is probably a reason for this; but I don't know what it
> could be.
>
> This may also be the reason why the code coverage metrics are inaccurate
> when you run from the top.  I have executed on my dev machine w/o code
> coverage, and the tests all passed increasing the # from 216 to 269.
>
> If there is no known reason, I would like to submit these two changes as a
> patch.

Stuff in framework/base is loaded automatically by framework/start, by
reading foo.properties file.

The reason the base metrics are invalid when run-tests is run, is not
because no test cases are run in base, but because there are 2
classloaders, both with cobertura active, and at shutdown, both
classloaders try to write to the same file, and corruption ensues.  I
haven't felt the need to track it down, because base has bare tests
that give coverage.

Plus, actually fixing it would require me ripping framework/start,
base/src config/component/container logic into pieces.

[Bob Morley]

On Apr 8, 2010, at 2:58 AM, Adam Heath wrote:

> Stuff in framework/base is loaded automatically by framework/start, by
> reading foo.properties file.
>
> The reason the base metrics are invalid when run-tests is run, is not
> because no test cases are run in base, but because there are 2
> classloaders, both with cobertura active, and at shutdown, both
> classloaders try to write to the same file, and corruption ensues.  I
> haven't felt the need to track it down, because base has bare tests
> that give coverage.
>
> Plus, actually fixing it would require me ripping framework/start,
> base/src config/component/container logic into pieces.

Hey Adam, this is not what my results have shown.  When I execute the  
tests in our current trunk there are 216 tests executed (from the  
JUnit reports) and I get code coverage on UtilValidate based on my  
initial comment in the other thread "Here is my question -- when  
looking at the reports it showed 100% line code coverage in  
UtilValidate (for example) but this was for 111 lines.  Clearly this  
class has many more lines than that, and when I opened it up I saw  
that large portions of it were not marked green or red in the report.  
What gives?  :)"

Now, if I decide to explicitly load the base component (and fix the  
test def) two things happen.  1) My JUnit test report now reports 269  
tests being executed and "basetests" appears in the classes on the  
left drilling in shows 53 tests here (216 + 53 = 269).  2) when I run  
the Cobertura report and drill down into UtilValidate it now shows 23%  
line coverage (111/477 lines) and the class is properly marked green/
red.

I really think that our current builds are not executing your base  
tests and there is actually not a problem with how the cobertura is  
collecting its coverage metrics, the problem is really  
TestRunContainer creates a JunitSuiteWrapper who asks  
ComponentConfig.getAllTestSuiteInfos and this implementation uses  
getAllComponents() which serves up the static map of configs which are  
initially populated from the component-load.xml.

If we agree to that then I can propose a solution to ensure a  
ComponentConfig instance exists for "base" but avoids parts of the  
component load that we are not interested in but does perform things  
we are interested in (see current ComponentConfig constructor -  
classpathinfos vs. test suite infos).

[Adam Heath-2]

Robert Morley wrote:
> Hey Adam, this is not what my results have shown.  When I execute the
> tests in our current trunk there are 216 tests executed (from the JUnit
> reports) and I get code coverage on UtilValidate based on my initial
> comment in the other thread "Here is my question -- when looking at the
> reports it showed 100% line code coverage in UtilValidate (for example)
> but this was for 111 lines.  Clearly this class has many more lines than
> that, and when I opened it up I saw that large portions of it were not
> marked green or red in the report.  What gives?  :)"

You misunderstand.  The classes are marked 100% covered, but that is
the problem.  They are *not* 100% covered.  The bug is that the double
class loaders are writing to the same cobertura.dat file, and the
first classloader, that contains framework/base code, gets corrupted
output.

You loading framework/base as a component, so it's testdef files can
be run, will not solve the actual problem, of the double classloaders,
and 2 cobertura instances.

Without loading any of the framework/base tests, we should still get
correct coverage values on base, just because it happens to be called
by everything else in the system.  But we don't, the numbers are wrong.

[Scott Gray-2]

On 8/04/2010, at 10:54 AM, Adam Heath wrote:

> Robert Morley wrote:
>> Hey Adam, this is not what my results have shown.  When I execute the
>> tests in our current trunk there are 216 tests executed (from the JUnit
>> reports) and I get code coverage on UtilValidate based on my initial
>> comment in the other thread "Here is my question -- when looking at the
>> reports it showed 100% line code coverage in UtilValidate (for example)
>> but this was for 111 lines.  Clearly this class has many more lines than
>> that, and when I opened it up I saw that large portions of it were not
>> marked green or red in the report.  What gives?  :)"
>
> You misunderstand.  The classes are marked 100% covered, but that is
> the problem.  They are *not* 100% covered.  The bug is that the double
> class loaders are writing to the same cobertura.dat file, and the
> first classloader, that contains framework/base code, gets corrupted
> output.
>
> You loading framework/base as a component, so it's testdef files can
> be run, will not solve the actual problem, of the double classloaders,
> and 2 cobertura instances.
>
> Without loading any of the framework/base tests, we should still get
> correct coverage values on base, just because it happens to be called
> by everything else in the system.  But we don't, the numbers are wrong.

Shooting from the hip here but couldn't the classpath entries be removed from the base ofbiz-component file?

Regards
Scott

[Adam Heath-2]

Scott Gray wrote:

> On 8/04/2010, at 10:54 AM, Adam Heath wrote:
>
>> Robert Morley wrote:
>>> Hey Adam, this is not what my results have shown.  When I execute the
>>> tests in our current trunk there are 216 tests executed (from the JUnit
>>> reports) and I get code coverage on UtilValidate based on my initial
>>> comment in the other thread "Here is my question -- when looking at the
>>> reports it showed 100% line code coverage in UtilValidate (for example)
>>> but this was for 111 lines.  Clearly this class has many more lines than
>>> that, and when I opened it up I saw that large portions of it were not
>>> marked green or red in the report.  What gives?  :)"
>> You misunderstand.  The classes are marked 100% covered, but that is
>> the problem.  They are *not* 100% covered.  The bug is that the double
>> class loaders are writing to the same cobertura.dat file, and the
>> first classloader, that contains framework/base code, gets corrupted
>> output.
>>
>> You loading framework/base as a component, so it's testdef files can
>> be run, will not solve the actual problem, of the double classloaders,
>> and 2 cobertura instances.
>>
>> Without loading any of the framework/base tests, we should still get
>> correct coverage values on base, just because it happens to be called
>> by everything else in the system.  But we don't, the numbers are wrong.
>
> Shooting from the hip here but couldn't the classpath entries be removed from the base ofbiz-component file?

Wouldn't have anything to do with the problem.

framework/start reads foo.properties file.  Finds location of
framework/base/lib.  Does a recursive directory scan, creates
classloader will all found jars.

With this first classloader, it now starts loading component/container
classes, which read all the rest of the defined components in the rest
of framework, applications, and specialpurpose.

Now that we have a list of components, a second classloader is
created, with all their defined classpath entries.  It's parent is the
first classloader.

In both cases, these classloaders are instrumented dynamically at
construction time.  All found jars are copied to $TEMP, and
instrumented on the fly.  Both classloaders are configured to write to
the same cobertura.dat file.

Ideally, I'd like to see some of component/container moved into start.
 Then base would be just like any other component.

[Bob Morley]

On Apr 8, 2010, at 12:54 PM, Adam Heath wrote:

>
> You misunderstand.  The classes are marked 100% covered, but that is
> the problem.  They are *not* 100% covered.  The bug is that the double
> class loaders are writing to the same cobertura.dat file, and the
> first classloader, that contains framework/base code, gets corrupted
> output.
>
> You loading framework/base as a component, so it's testdef files can
> be run, will not solve the actual problem, of the double classloaders,
> and 2 cobertura instances.
>
> Without loading any of the framework/base tests, we should still get
> correct coverage values on base, just because it happens to be called
> by everything else in the system.  But we don't, the numbers are  
> wrong.

I see your point -- how about this, we put these fixes in which will  
accomplish at the very least ensuring that the base unit testers are  
getting exercised.  I can create another JIRA ticket to handle the two  
class loaders / cobertura.dat file issue.  Once the patch is ready we  
can look it over with an eye towards any adverse affects.

[Adam Heath-2]

Robert Morley wrote:

> On Apr 8, 2010, at 12:54 PM, Adam Heath wrote:
>>
>> You misunderstand.  The classes are marked 100% covered, but that is
>> the problem.  They are *not* 100% covered.  The bug is that the double
>> class loaders are writing to the same cobertura.dat file, and the
>> first classloader, that contains framework/base code, gets corrupted
>> output.
>>
>> You loading framework/base as a component, so it's testdef files can
>> be run, will not solve the actual problem, of the double classloaders,
>> and 2 cobertura instances.
>>
>> Without loading any of the framework/base tests, we should still get
>> correct coverage values on base, just because it happens to be called
>> by everything else in the system.  But we don't, the numbers are wrong.
>
> I see your point -- how about this, we put these fixes in which will
> accomplish at the very least ensuring that the base unit testers are
> getting exercised.  I can create another JIRA ticket to handle the two
> class loaders / cobertura.dat file issue.  Once the patch is ready we
> can look it over with an eye towards any adverse affects.

I'd rather fix the duplicate classloader/cobertura thing first, then
apply whatever changes you come up with after that.  Otherwise, it
might be harder to fix if we do this current thread first.

[Bob Morley]

On Apr 8, 2010, at 1:16 PM, Adam Heath wrote:

> I'd rather fix the duplicate classloader/cobertura thing first, then
> apply whatever changes you come up with after that.  Otherwise, it
> might be harder to fix if we do this current thread first.

Ok -- let me spend some time working on that and then you can review  
the patch and we can drive to a resolution.  From what you said  
earlier, we will drive to make "base" more like the other components.  
If this is done then we would likely only instrument once for the  
components classpath (rather then being triggered from Start).