On 07/20/2011 12:46 PM, Pradeep Kumar wrote: Hi Pradeep, I have some comments to make on this wrapper, see below:
> From a8cc3242f292a532a05a38220c06d261a60d2c66 Mon Sep 17 00:00:00 2001 > From: pradeep<[email protected]> > Date: Wed, 20 Jul 2011 21:10:13 +0530 > Subject: [PATCH] [Autotest][Patch]: Add blast test ^ Please provide a proper patch title, such as: client: Adding blast test wrapper > Signed-off-by: Poornima Nayak<[email protected]> > new file: client/tests/blast/blast.py > new file: client/tests/blast/control > --- > client/tests/blast/blast.py | 49 > +++++++++++++++++++++++++++++++++++++++++++ > client/tests/blast/control | 15 +++++++++++++ > 2 files changed, 64 insertions(+), 0 deletions(-) > create mode 100755 client/tests/blast/blast.py > create mode 100755 client/tests/blast/control > > diff --git a/client/tests/blast/blast.py b/client/tests/blast/blast.py > new file mode 100755 > index 0000000..90e65d3 > --- /dev/null > +++ b/client/tests/blast/blast.py > @@ -0,0 +1,49 @@ > +import os, time, shutil > +from autotest_lib.client.bin import test, utils > + > + > +class blast(test.test): > + version = 1 > + > + version=1 ^ Duplicated version assignment > + def initialize(self): > + """ > + Sets the overall failure counter for the test. > + """ > + self.nfail = 0 > + > + def setup(self, tarball = 'blast.tar.gz'): > + """ > + Sets the source setup for test execution . > + """ Inconsistent blank lines in the code > + self.tarball = utils.unmap_url(self.bindir, tarball, self.tmpdir) > + utils.extract_tarball_to_dir(self.tarball, self.srcdir) > + os.chdir(self.srcdir) Single spacing between functions, against coding style. Also, there's a precompiled version of the test in the blast tarball. In order to be useful for more architectures, we need the full source code so the binary can be built. I can't accept the blast tarball the way it was shipped. > + def run_once(self, test_in_file = '', dura_min = 30 ): ^ Inconsistent spacing between variables and default values. PEP 08 recommends default value assignment in function headers to not have spaces. > + """ > + Runs the test, with the appropriate control file. > + """ > + > + try: > + if test_in_file == '': > + cmd = self.srcdir + '/blast.LxS_x64&' > + else: > + cmd = self.srcdir + '/blast.LxS_x64 ' + self.srcdir + '/' + > test_in_file + '&' > + os.system(cmd) > + time.sleep(dura_min*60) ^ dura_min could be better expressed as a test_length, value in seconds, for consistency with other tests. > + os.system('pkill blast.LxS_x64') Autotest has an API to run subcommands, utils.system or utils.run(). Please use it instead of system. Also, running it on background and then sleeping for the test duration is not necessary, as you can always pass a timeout param to utils.system or utils.run(). > + 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) ^ This is done by default in autotest, no need for this cleanup stage. > + if self.nfail != 0: > + raise error.TestFail('Blast test suite failed.') > diff --git a/client/tests/blast/control b/client/tests/blast/control > new file mode 100755 > index 0000000..208b429 > --- /dev/null > +++ b/client/tests/blast/control > @@ -0,0 +1,15 @@ > +AUTHOR = "Poornima Nayak<[email protected]" > +NAME = "blast" > +TEST_CATEGORY = "Stress" > +TEST_CLASS = "General" > +TEST_TYPE = "File system" > +TIME = 'MEDIUM' > +DOC=''' > +A filesystem stress test on mountd file system ^ Typo, mounted. > +''' > +#input_config_files=['blast.lst', 'blast.lst2'] > +input_config_files=['dotu_pass.lst'] > + > +for input in input_config_files: > + print "input file",input > + job.run_test('blast', test_in_file=input, dura_min=1 ) _______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
