[GSoC] Project update test/code separation

[Ganath Rathnayake <ga...@gmail.com>]

Hi all,
Sorry for being late for update you. I sent this to the mailing list but
unfortunately it bounced back.

Last week of time I was working on creating a bash script which use to
change the exiting folder structure in to the new test/code separated
structure.

Here I had to address few different issues.

1. Creating the main/java and test/java folders in the src directory and put
test files in test/java and rest in the main/java
I did this by putting the tests in the test folders in each module in the
test/java folder. I used the following code to do that. $test variable is
the folder structure of each module.

cp main/java/$test/test/*  test/java/$test/
rm -r main/java/$test/test/

2. Fixing the paths in the macros.xml file. This was achieved by the
following piece of code.
sed 's/@{prefix}src/@{prefix}src\/main\/java/' <macros.xml >temp

cat temp >macros.xml
rm temp

3. Fixing the paths in build.xml files. This was achieved by first replacing
all the paths by adding main/java/ to the baginning of each path and then
changing it with test/java if it is a test. You can see the code follows.
Also some package name the files also change when it needed to do so.

for x in $(ls main/java/$test/test/)
do
    echo $x
    sed 's/\.test;/;/' <main/java/$test/test/$x >temp

    cat temp >main/java/$test/test/$x

    j=`expr "$test" : 'org/ofbiz/base'`
    if [ $j != 0 ];
    then
        sed 's/\.test\./\./' <main/java/$test/test/$x >temp
        cat temp >main/java/$test/test/$x
    fi

    echo $test
    sed "s#main/java/${test}test/#test/java/${test}#g" <../build.xml >temp
# ./org/ofbiz does not match
    cat temp >../build.xml

done
rm temp

4. Remove the package names which include ".test." from the files. I went
through all the xml files and found out the places where it needed the
changes and hard coded since we dont need to go through every files for
changing small number of files in some of the modules(framework/service,
framework/minilang, framework/entity, framework/base,
application/securityext, application/product, application/order,
application/content, application/accounting). You can find the code follows.

                    if [ $dir = 'service' ];
                    then
                        sed 's/\.test\./\./'
<servicedef/services_test_se.xml >temp
                        cat temp >servicedef/services_test_se.xml

                        sed 's/\.test\./\./' <testdef/servicetests.xml >temp
                        cat temp >testdef/servicetests.xml

                    elif [ $dir = 'minilang' ];
                    then
                        sed 's/\.test\./\./' <testdef/MinilangTests.xml
>temp
                        cat temp >testdef/MinilangTests.xml

                    elif [ $dir = 'entity' ];
                    then
                        sed 's/\.test\./\./' <testdef/entitytests.xml >temp
                        cat temp >testdef/entitytests.xml

                    elif [ $dir = 'base' ];
                    then
                        sed 's/\.test\./\./' <build.xml >temp
                        cat temp >build.xml

                        sed 's/\.test\./\./' <config/test-containers.xml
>temp
                        cat temp >config/test-containers.xml

                        sed 's/\.test\./\./' <testdef/basetests.xml >temp
                        cat temp >testdef/basetests.xml


                    elif [ $dir = 'securityext' ];
                    then
                        sed 's/\.test\./\./' <testdef/securitytests.xml
>temp
                        cat temp >testdef/securitytests.xml

                        sed 's/\.test\./\./'
<testdef/data/SecurityTestData.xml >temp
                        cat temp >testdef/data/SecurityTestData.xml

                    elif [ $dir = 'product' ];
                    then
                        sed 's/\.test\./\./' <testdef/FacilityTest.xml >temp
                        cat temp >testdef/FacilityTest.xml

                    elif [ $dir = 'order' ];
                    then
                        sed 's/\.test\./\./' <servicedef/services.xml >temp
                        cat temp >servicedef/services.xml

                        sed 's/\.test\./\./' <testdef/OrderTest.xml >temp
                        cat temp >testdef/OrderTest.xml

                    elif [ $dir = 'content' ];
                    then
                        sed 's/\.test\./\./' <testdef/lucenetests.xml >temp
                        cat temp >testdef/lucenetests.xml

                    elif [ $dir = 'accounting' ];
                    then
                        sed 's/\.test\./\./' <testdef/accountingtests.xml
>temp
                        cat temp >testdef/accountingtests.xml

                    fi



If you have any suggestions or any input to improve the script please let me
know. You can find the full script in [1].


-- 
thanks
Ganath

[1]. https://github.com/ganathr/ofbiz/blob/trunk/old2new.sh

Re: [GSoC] Project update test/code separation

[Adam Heath <do...@brainfood.com>]

On 08/11/2011 11:42 PM, Ganath Rathnayake wrote:
> Hi Adam,
> 
> Thanks for your ideas and comments. I like to do some the changes you
> mentioned.
> 
> 
>> Your script should do 'set -e' at the beginning.  It's possible that
>> your script is buggy, or doesn't handle something on a particular
>> machine, or the disk runs out of space.  Without that, you would have
>> to check the return of *every* command(cp, rm, cat, whatever), and
>> handle the error.  Without such checking, things can sometimes go very
>> bad.
>>
> 
> I'll do it.
> 
> 
>>
>>> 1. Creating the main/java and test/java folders in the src directory and
>> put
>>> test files in test/java and rest in the main/java
>>> I did this by putting the tests in the test folders in each module in the
>>> test/java folder. I used the following code to do that. $test variable is
>>> the folder structure of each module.
>>
>> What is $test?  Don't summarize, show code.  I don't understand your
>> explanation.
>>
> 
> $test is a variable I use to store the relative path to the test folders. I
> used this variable since one module can have several different test classes.

I think I know what you mean, but if so, then are you implying that
you call the script manually, changing $test each time?  If so, don't
do that.  All logic should be contained in the script.  Even if you
have to hard-code the list of things being iterated.

There are other people on this list who could then help remove the
hard-coded sub-package list, and extract it directly from the filesystem.

The following is completely untested.  It moves all files in
src/**/test/* to src/test/java, then moves everything left to
src/main/java.
It does *NOT* handle things in src/META-INF/services that are actually
located in src/**/test/*.  It does *NOT* handle extracting the list of
*all* test package names; but if you don't change the package names,
then you probably don't need that.

Fixing META-INF/services involves finding all the files that reference
*/test/*, making a copy of that file in test, remove the lines that
are in main, leaving only the test versions, then modifying the
original to remove the test versions.  That is an excersize left to
the reader.

==
for comp_xml in
 framework/*/ofbiz-component.xml
 applications/*/ofbiz-component.xml
 specialpurpose/*/ofbiz-component.xml
do
 comp_dir="${comp_xml%/ofbiz-component_xml}"
 if ! [ -d "$comp_dir/src" ]; then
  continue
 fi

 for test_subdir in
  $(find "$comp_dir/src" -type d -name test -printf '%P\n' | sort -r)
 do
  if ! [ -d "$comp_dir/src/test/java/$test_subdir" ]; then
   mkdir -p "$comp_dir/src/test/java/$test_subdir"
  fi
  for file in
   $(find "$comp_dir/src/$test_subdir" -maxdepth 1 \
    -type f -printf '%P'n')
  do
   svn mv \
    "$comp_dir/src/$file" \
    "$comp_dir/src/test/java/$file"
  done
 done
 for file in
  $(find "$comp_dir/src" -type f -printf '%P\n')
 do
  svn mv \
   "$comp_dir/src/$file" \
   "$comp_dir/src/main/java/$file"
 done
done
==

ps: The script is mostly POSIX, except for:

1: If $(find) returns an empty list, then non-bash shells will throw a
syntax error.

2: I am not certain if find/%P is POSIX.

3: ${%} actually IS POSIX, most people just aren't aware of it.

4: ! [ -test file ] is POSIX, [ ! -test file ] isn't.

5: [ arg op value ] && [ arg op value ] is POSIX, [ arg op value -a
arg op value ] isn't.  (I don't use this construct here).

6: mkdir -p isn't POSIX, but I was too lasy to do it the right away.

> 
> 
>>
>>> cp main/java/$test/test/*  test/java/$test/
>>
>> cp -a, preserve timestamps/permissions/ownership.
>>
>> Your cp will *not* work if there are sub-folders in test, as it would
>> then copy the .svn files, which is *not* what you want.
>>
>> rsync main/java/$test/test/ test/java/$test/ -a --exclude '.svn'
>>
> 
> Since I worked in the the git repo I didn't notice that. Thanks for pointing
> out me the issue and the solution.

That's ok initially, but this kind of script should *not* be run in
git, once it is fully developed.  The script should *not* do:

cp foo bar

or

mv foo bar

Those will not maintain proper svn history.  The script should
directly call:

mkdir $new/path/directory
svn add new/path/directory
svn cp old/path/directory/foo new/path/directory/bar

or svn mv.  Otherwise, you won't maintain proper svn history.

>  sed 's,@{prefix}src,@{prefix}src/main/java,'
>>
>> Note the use of ',' as the separator; this is easier to read, as you
>> don't have to escape all the / in the path.
>>
> 
> Thanks for the suggestion. I didn't know it before.

Actually, I think you did, I seem to recall other sed uses in your
script that do change the separator(from / to #).

>>> cat temp >macros.xml
>>> rm temp
>>>
>>> 3. Fixing the paths in build.xml files. This was achieved by first
>> replacing
>>> all the paths by adding main/java/ to the baginning of each path and then
>>> changing it with test/java if it is a test. You can see the code follows.
>>> Also some package name the files also change when it needed to do so.
>>>
>>> for x in $(ls main/java/$test/test/)
>>> do
>>>     echo $x
>>>     sed 's/\.test;/;/' <main/java/$test/test/$x >temp
>>>
>>>     cat temp >main/java/$test/test/$x
>>>
>>>     j=`expr "$test" : 'org/ofbiz/base'`
>>>     if [ $j != 0 ];
>>>     then
>>>         sed 's/\.test\./\./' <main/java/$test/test/$x >temp
>>>         cat temp >main/java/$test/test/$x
>>>     fi
>>
>> What?  Are we removing the test sub packages?  As in:
>>
>> package org.ofbiz.entity.test;
>>
>> becomes
>>
>> package org.ofbiz.entity;
>>
>> ?
>>
>> If so, then NO! DO NOT DO THAT.  Test cases should not ever be inside
>> the same package as the code they are testing.  Bugs might be missed,
>> as you can call package private/protected methods when in the same
>> package, that you can't when in another sub-package.
>>
>> I can *not* stress this enough.  Do *NOT* move the test code out of a
>> test sub-package.
>>
>>
> Here I removed the test folder because it already in a different package.
> src
>    |---main
>    |       `----java
>    `---test
>           `----java
> 
> Since all the test classes in the test/java I thought we do not have to use
> extra test class for module structure. We can make a jar for that by
> changing the packaging criteria described in macro.xml. Please let me know
> your ideas on this.

Just because the prefix for the test class location is different, does
not mean we can remove the actual 'test' from the package name.
Otherwise:

src/main/java:
 org/ofbiz/entity/GenericDelegator.java
src/test/java:
 org/ofbiz/entity/GenericDelegatorTest.java

If those are put into 2 different jars, and the jars are 'signed' and
'sealed', then the second jar is not allowed to put classes into a
package that was previously sealed.  I can see us wanting to do that.

Also, if there are package private or protected methods in
GenericDelegator, then having the test code in the same package might
allow for accidentally calling those internal methods; that's not a
proper test case.  You should test the public api(by instantiating the
object and calling it's methods), or the protected api, by extending
the class(and then calling the protected methods).

Having a separate 'test' sub package makes both of these possible, and
leads to better test cases.  The test cases also end up not being tied
to the implementation, so that you can do a heavy rewrite to the
class, moving methods around, and as long as you keep the same public
api, things should still work.

Re: [GSoC] Project update test/code separation

[Ganath Rathnayake <ga...@gmail.com>]

Hi Adam,

Thanks for your ideas and comments. I like to do some the changes you
mentioned.


> Your script should do 'set -e' at the beginning.  It's possible that
> your script is buggy, or doesn't handle something on a particular
> machine, or the disk runs out of space.  Without that, you would have
> to check the return of *every* command(cp, rm, cat, whatever), and
> handle the error.  Without such checking, things can sometimes go very
> bad.
>

I'll do it.


>
> > 1. Creating the main/java and test/java folders in the src directory and
> put
> > test files in test/java and rest in the main/java
> > I did this by putting the tests in the test folders in each module in the
> > test/java folder. I used the following code to do that. $test variable is
> > the folder structure of each module.
>
> What is $test?  Don't summarize, show code.  I don't understand your
> explanation.
>

$test is a variable I use to store the relative path to the test folders. I
used this variable since one module can have several different test classes.


>
> > cp main/java/$test/test/*  test/java/$test/
>
> cp -a, preserve timestamps/permissions/ownership.
>
> Your cp will *not* work if there are sub-folders in test, as it would
> then copy the .svn files, which is *not* what you want.
>
> rsync main/java/$test/test/ test/java/$test/ -a --exclude '.svn'
>

Since I worked in the the git repo I didn't notice that. Thanks for pointing
out me the issue and the solution.


 sed 's,@{prefix}src,@{prefix}src/main/java,'
>
> Note the use of ',' as the separator; this is easier to read, as you
> don't have to escape all the / in the path.
>

Thanks for the suggestion. I didn't know it before.



>
> > cat temp >macros.xml
> > rm temp
> >
> > 3. Fixing the paths in build.xml files. This was achieved by first
> replacing
> > all the paths by adding main/java/ to the baginning of each path and then
> > changing it with test/java if it is a test. You can see the code follows.
> > Also some package name the files also change when it needed to do so.
> >
> > for x in $(ls main/java/$test/test/)
> > do
> >     echo $x
> >     sed 's/\.test;/;/' <main/java/$test/test/$x >temp
> >
> >     cat temp >main/java/$test/test/$x
> >
> >     j=`expr "$test" : 'org/ofbiz/base'`
> >     if [ $j != 0 ];
> >     then
> >         sed 's/\.test\./\./' <main/java/$test/test/$x >temp
> >         cat temp >main/java/$test/test/$x
> >     fi
>
> What?  Are we removing the test sub packages?  As in:
>
> package org.ofbiz.entity.test;
>
> becomes
>
> package org.ofbiz.entity;
>
> ?
>
> If so, then NO! DO NOT DO THAT.  Test cases should not ever be inside
> the same package as the code they are testing.  Bugs might be missed,
> as you can call package private/protected methods when in the same
> package, that you can't when in another sub-package.
>
> I can *not* stress this enough.  Do *NOT* move the test code out of a
> test sub-package.
>
>
Here I removed the test folder because it already in a different package.
src
   |---main
   |       `----java
   `---test
          `----java

Since all the test classes in the test/java I thought we do not have to use
extra test class for module structure. We can make a jar for that by
changing the packaging criteria described in macro.xml. Please let me know
your ideas on this.

thanks
Ganath

Re: [GSoC] Project update test/code separation

[Adam Heath <do...@brainfood.com>]

On 08/11/2011 12:36 PM, Ganath Rathnayake wrote:
> Hi all,
> Sorry for being late for update you. I sent this to the mailing list but
> unfortunately it bounced back.
> 
> Last week of time I was working on creating a bash script which use to
> change the exiting folder structure in to the new test/code separated
> structure.
> 
> Here I had to address few different issues.


Your script should do 'set -e' at the beginning.  It's possible that
your script is buggy, or doesn't handle something on a particular
machine, or the disk runs out of space.  Without that, you would have
to check the return of *every* command(cp, rm, cat, whatever), and
handle the error.  Without such checking, things can sometimes go very
bad.

> 1. Creating the main/java and test/java folders in the src directory and put
> test files in test/java and rest in the main/java
> I did this by putting the tests in the test folders in each module in the
> test/java folder. I used the following code to do that. $test variable is
> the folder structure of each module.

What is $test?  Don't summarize, show code.  I don't understand your
explanation.

> cp main/java/$test/test/*  test/java/$test/

cp -a, preserve timestamps/permissions/ownership.

Your cp will *not* work if there are sub-folders in test, as it would
then copy the .svn files, which is *not* what you want.

rsync main/java/$test/test/ test/java/$test/ -a --exclude '.svn'

> rm -r main/java/$test/test/
> 
> 2. Fixing the paths in the macros.xml file. This was achieved by the
> following piece of code.
> sed 's/@{prefix}src/@{prefix}src\/main\/java/' <macros.xml >temp

sed 's,@{prefix}src,@{prefix}src/main/java,'

Note the use of ',' as the separator; this is easier to read, as you
don't have to escape all the / in the path.

> cat temp >macros.xml
> rm temp
> 
> 3. Fixing the paths in build.xml files. This was achieved by first replacing
> all the paths by adding main/java/ to the baginning of each path and then
> changing it with test/java if it is a test. You can see the code follows.
> Also some package name the files also change when it needed to do so.
> 
> for x in $(ls main/java/$test/test/)
> do
>     echo $x
>     sed 's/\.test;/;/' <main/java/$test/test/$x >temp
> 
>     cat temp >main/java/$test/test/$x
> 
>     j=`expr "$test" : 'org/ofbiz/base'`
>     if [ $j != 0 ];
>     then
>         sed 's/\.test\./\./' <main/java/$test/test/$x >temp
>         cat temp >main/java/$test/test/$x
>     fi

What?  Are we removing the test sub packages?  As in:

package org.ofbiz.entity.test;

becomes

package org.ofbiz.entity;

?

If so, then NO! DO NOT DO THAT.  Test cases should not ever be inside
the same package as the code they are testing.  Bugs might be missed,
as you can call package private/protected methods when in the same
package, that you can't when in another sub-package.

I can *not* stress this enough.  Do *NOT* move the test code out of a
test sub-package.

>     echo $test
>     sed "s#main/java/${test}test/#test/java/${test}#g" <../build.xml >temp
> # ./org/ofbiz does not match
>     cat temp >../build.xml
> 
> done
> rm temp
> 
> 4. Remove the package names which include ".test." from the files. I went
> through all the xml files and found out the places where it needed the
> changes and hard coded since we dont need to go through every files for
> changing small number of files in some of the modules(framework/service,
> framework/minilang, framework/entity, framework/base,
> application/securityext, application/product, application/order,
> application/content, application/accounting). You can find the code follows.

This whole part is not needed, if you don't remove the test sub package.

> 
>                     if [ $dir = 'service' ];
>                     then
>                         sed 's/\.test\./\./'
> <servicedef/services_test_se.xml >temp
>                         cat temp >servicedef/services_test_se.xml
> 
>                         sed 's/\.test\./\./' <testdef/servicetests.xml >temp
>                         cat temp >testdef/servicetests.xml
> 
>                     elif [ $dir = 'minilang' ];
>                     then
>                         sed 's/\.test\./\./' <testdef/MinilangTests.xml
>> temp
>                         cat temp >testdef/MinilangTests.xml
> 
>                     elif [ $dir = 'entity' ];
>                     then
>                         sed 's/\.test\./\./' <testdef/entitytests.xml >temp
>                         cat temp >testdef/entitytests.xml
> 
>                     elif [ $dir = 'base' ];
>                     then
>                         sed 's/\.test\./\./' <build.xml >temp
>                         cat temp >build.xml
> 
>                         sed 's/\.test\./\./' <config/test-containers.xml
>> temp
>                         cat temp >config/test-containers.xml
> 
>                         sed 's/\.test\./\./' <testdef/basetests.xml >temp
>                         cat temp >testdef/basetests.xml
> 
> 
>                     elif [ $dir = 'securityext' ];
>                     then
>                         sed 's/\.test\./\./' <testdef/securitytests.xml
>> temp
>                         cat temp >testdef/securitytests.xml
> 
>                         sed 's/\.test\./\./'
> <testdef/data/SecurityTestData.xml >temp
>                         cat temp >testdef/data/SecurityTestData.xml
> 
>                     elif [ $dir = 'product' ];
>                     then
>                         sed 's/\.test\./\./' <testdef/FacilityTest.xml >temp
>                         cat temp >testdef/FacilityTest.xml
> 
>                     elif [ $dir = 'order' ];
>                     then
>                         sed 's/\.test\./\./' <servicedef/services.xml >temp
>                         cat temp >servicedef/services.xml
> 
>                         sed 's/\.test\./\./' <testdef/OrderTest.xml >temp
>                         cat temp >testdef/OrderTest.xml
> 
>                     elif [ $dir = 'content' ];
>                     then
>                         sed 's/\.test\./\./' <testdef/lucenetests.xml >temp
>                         cat temp >testdef/lucenetests.xml
> 
>                     elif [ $dir = 'accounting' ];
>                     then
>                         sed 's/\.test\./\./' <testdef/accountingtests.xml
>> temp
>                         cat temp >testdef/accountingtests.xml
> 
>                     fi
> 
> 
> 
> If you have any suggestions or any input to improve the script please let me
> know. You can find the full script in [1].
> 
>