On 01/16/2012 09:43 AM, Madhuri Appana wrote:
> From: Madhuri Appana<[email protected]>
>
> More information can be found under cpan.org

Hi Madhuri, thanks for your patch, I do have some comments and 
questions, see below:

> Signed-off-by: Madhuri Appana<[email protected]>
> ---
>   client/tests/perlfs/control    |   11 ++++
>   client/tests/perlfs/perlfs.py  |  109 
> ++++++++++++++++++++++++++++++++++++++++
>   client/tests/perlfs/perlfs.tgz |  Bin 0 ->  1259520 bytes
>   3 files changed, 120 insertions(+), 0 deletions(-)
>   create mode 100644 client/tests/perlfs/control
>   create mode 100755 client/tests/perlfs/perlfs.py
>   create mode 100644 client/tests/perlfs/perlfs.tgz

^ perlfs.tgz is actually a tar file, rather than a tar.gz file. Although 
autotest will detect this and work just fine, it confuses people and we 
can save size in the repo with a .tar.bz2 file. Also, I've checked the 
contents and the permissions are all scrambled and inconsistent so they 
must be normalized and fixed (I recommend to re-download the pristine 
upstream tarballs and do not mess with them in any way).

>
> diff --git a/client/tests/perlfs/control b/client/tests/perlfs/control
> new file mode 100644
> index 0000000..3ac209d
> --- /dev/null
> +++ b/client/tests/perlfs/control
> @@ -0,0 +1,11 @@
> +AUTHOR = "Madhuri Appana<[email protected]>"
> +NAME = "PERLFS"

^ You don't need to use all caps on the test suite's name.

> +TEST_CATEGORY = "Filesystem FVT"
> +TEST_CLASS = "General"
> +TEST_TYPE = "client"
> +TIME = 'MEDIUM'
> +DOC='''
> +Test suite to test General Filesystem tests.
> +'''
> +
> +job.run_test('perlfs',profile='test.pl')
> diff --git a/client/tests/perlfs/perlfs.py b/client/tests/perlfs/perlfs.py
> new file mode 100755
> index 0000000..7b5bceb
> --- /dev/null
> +++ b/client/tests/perlfs/perlfs.py
> @@ -0,0 +1,109 @@
> +import os, shutil, glob, logging
> +from autotest_lib.client.bin import test, utils
> +from autotest_lib.client.common_lib import error
> +
> +class perlfs(test.test):
> +    """
> +    perlfs test is a testsuite which can run on
> +    both BSD and System V based systems. The tests.init file
> +    has to be modified based on the OS in which this test is run.
> +    The tar file in this dir has init file which works for Linux
> +    platform. As well as it has some minor fixes for the testsuite
> +
> +    @author Madhuri Appana([email protected])(original code)
> +    """
> +
> +    version=1
> +    def initialize(self):
> +        """
> +        Sets the overall failure counter for the test.
> +        """
> +        self.nfail = 0
> +
> +    def run_once(self, tarball = None, profile = None, iteration=1):
> +        """
> +        Runs the test, with the appropriate control file.
> +        """
> +
> +        if (not os.path.exists('perlfs')):
> +            if not tarball:
> +                tarball = 'perlfs.tgz'
> +            perlfs_tarball = utils.unmap_url(self.bindir, tarball, 
> self.tmpdir)
> +            utils.extract_tarball_to_dir(perlfs_tarball, self.srcdir)
> +
> +            os.chdir(self.srcdir)
> +         self.srcdir += '/Readonly-1.03'


^ I don't think it is a good idea to install this bundle in a compulsory 
way, system wide. A better approach would be to:

1) Check whether the capabilities (ie, the libraries) are present. I 
believe a simple perl program(s) can help you to get the information.
2) If some capability is missing, install the bundle in a directory 
inside the autotest tree (say, client/tests/perltest/build/) then set 
the PERL5LIB environment variable to reference that directory.

You can give information on how to install those dependencies in a 
Fedora/Ubuntu/Debian/OpenSUSE box as well, so people who don't want to 
install the bundle can have their distro packages installed.

> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +     
> +         self.srcdir += '/../Sub-Uplevel-0.22'
> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +     
> +         self.srcdir += '/../Test-Class-0.36'
> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +     
> +         self.srcdir += '/../Test-Exception-0.29'
> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +     
> +         self.srcdir += '/../Test-Simple-0.94'
> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +
> +         self.srcdir += '/../MRO-Compat-0.11'
> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +     
> +         self.srcdir += '/../List-MoreUtils-0.17'
> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +
> +         self.srcdir += '/../Test-Virtual-Filesystem-0.13'
> +            os.chdir(self.srcdir)
> +            utils.system('perl Makefile.PL')
> +            utils.system('make')
> +            utils.system('make install')
> +        else:
> +            os.chdir(self.srcdir)
> +
> +        try:
> +            if not profile:
> +                profile = 'profile_everything'
> +
> +         args = '%s ' % profile
> +            cmd = self.srcdir + '/' + args

In reality here you only want to execute test.pl of the 
Test-Virtual-Filesystem-0.13 tarball, but you've had appended 
self.srcdir with a lot of strings generating this large and unnecessary 
command line:

01/16 15:35:34 DEBUG|base_utils:0074| Running 
'/home/lmr/Code/autotest.git/client/tests/perlfs/src/Readonly-1.03/../Sub-Uplevel-0.22/../Test-Class-0.36/../Test-Exception-0.29/../Test-Simple-0.94/../MRO-Compat-0.11/../List-MoreUtils-0.17/../Test-Virtual-Filesystem-0.13/test.pl
 
'

So please fix that and don't use self.srcdir when you are installing the 
bundle (see more on *when* to install the bundle below). Also, in all 
directories of the bundle you repeat the steps:

 > +            os.chdir(self.srcdir)
 > +            utils.system('perl Makefile.PL')
 > +            utils.system('make')
 > +            utils.system('make install')

This can be perfectly refactored into a utility function, then a for 
loop can iterate through all the directories.

> +         self.results = utils.system_output(cmd)
> +            self.results_path = os.path.join(self.resultsdir,
> +                                         'raw_output_%s' % self.iteration)
> +            self.analysisdir = os.path.join(self.resultsdir,
> +                                        'analysis_%s' % self.iteration)
> +            utils.open_write_close(self.results_path, self.results)
> +        except Exception, e:
> +            self.nfail += 1
> +            logging.error("Test failed: %s", e)
> +
> +
> +    def cleanup(self):
> +        """
> +        Cleans up source directory and raises on failure.
> +        """
> +        if os.path.isdir(self.srcdir):
> +            shutil.rmtree(self.srcdir)
> +        if self.nfail != 0:
> +            raise error.TestFail('perlfs test suite failed.')

I need all those issues fixed. Thanks for your contribution!
_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Reply via email to