Well, a method called setup on a test object is a lot less ambiguous than a plain old function; I don't know what exactly it does, but I at least know what it's setting up: the test.
Most of your examples are different; they're cases where a module and a class in it have the same name. Plus, they're generic names because they're for something generic. Having a test class in a test module is fine when it's a generic abstract class which can be used for any kind of test. On the other hand, if test.test was only for defining filesystem tests, then test.test would be a terrible name. If this code is specifically for setting up client tests (or client jobs) then it should say so somewhere in either the module name or the function name. On Mon, Mar 29, 2010 at 3:34 PM, Eric Li(李咏竹) <[email protected]> wrote: > I admit this is not the best naming. I think that is because: > 1. The setup module is some wrapper around test.setup(), and I just want to > follow that name. > 2. There is enough module.class naming inside the code: base. > base_job.base_job, harness.harness, profiler.profiler, test.test. I think I > simply followed the style. > > > John, > I am open and what is your suggestion? > > > On Mon, Mar 29, 2010 at 3:15 PM, John Admanski <[email protected]>wrote: > >> When writing functions (and modules) to set up a specific type of thing, >> can we please not call it "setup"? Seeing a "setup.setup" call tells me >> nothing about what that call is doing. If a function is setting up >> something, I should at least be able to guess at what it's setting up from >> the name. >> >> -- John >> >> On Mon, Mar 29, 2010 at 2:58 PM, Eric Li(李咏竹) <[email protected]>wrote: >> >>> Finished code review with Martin. Attached is the patch set generated on >>> codereview.chromium.org. >>> global_config.ini is excluded in this patch. >>> >>> This change does not fully work with Martin's experimental feature >>> "hierarchical test structure". More test and code change is needed. But for >>> plain test directory it should work. >>> >>> Risk: Low. >>> Visibility: The existing user should not see any regression from this >>> change. >>> >>> >>> >>> Index: client/bin/base_utils.py >>> diff --git a/client/bin/base_utils.py b/client/bin/base_utils.py >>> index >>> 96d1fc06fa4f616433bae03cff85ec0053cb4279..7c5d26b11996363277180757c9049b9559b95cd1 >>> 100644 >>> --- a/client/bin/base_utils.py >>> +++ b/client/bin/base_utils.py >>> @@ -63,7 +63,10 @@ def extract_tarball_to_dir(tarball, dir): >>> the top level of a tarball is - useful for versioned directory >>> names, etc >>> """ >>> if os.path.exists(dir): >>> - raise NameError, 'target %s already exists' % dir >>> + if os.path.isdir(dir): >>> + shutil.rmtree(dir) >>> + else: >>> + os.remove(dir) >>> pwd = os.getcwd() >>> os.chdir(os.path.dirname(os.path.abspath(dir))) >>> newdir = extract_tarball(tarball) >>> Index: client/bin/setup_job.py >>> diff --git a/client/bin/setup_job.py b/client/bin/setup_job.py >>> index >>> bf07fdb16bc65df0916892eb53abfaf009a23e1f..a7088b22196b7fa1247622e5d6b8c5441ed0b819 >>> 100644 >>> --- a/client/bin/setup_job.py >>> +++ b/client/bin/setup_job.py >>> @@ -30,15 +30,62 @@ class setup_job(client_job.job): >>> See all options defined on client/bin/autotest. >>> """ >>> base_job.base_job.__init__(self, options=options) >>> - logging_manager.configure_logging( >>> - client_logging_config.ClientLoggingConfig(), >>> - results_dir=self.resultdir, >>> - verbose=options.verbose) >>> self._cleanup_results_dir() >>> self.pkgmgr = packages.PackageManager( >>> self.autodir, run_function_dargs={'timeout':3600}) >>> >>> >>> +def init_test(options, testdir): >>> + """ >>> + Instantiate a client test object from a given test directory. >>> + >>> + @param options Command line options passed in to instantiate a >>> setup_job >>> + which associates with this test. >>> + @param testdir The test directory. >>> + @returns A test object or None if failed to instantiate. >>> + """ >>> + >>> + locals_dict = locals().copy() >>> + globals_dict = globals().copy() >>> + >>> + locals_dict['testdir'] = testdir >>> + >>> + job = setup_job(options=options) >>> + locals_dict['job'] = job >>> + >>> + test_name = os.path.split(testdir)[-1] >>> + outputdir = os.path.join(job.resultdir, test_name) >>> + try: >>> + os.makedirs(outputdir) >>> + except OSError: >>> + pass >>> + locals_dict['outputdir'] = outputdir >>> + >>> + sys.path.insert(0, testdir) >>> + client_test = None >>> + try: >>> + try: >>> + import_stmt = 'import %s' % test_name >>> + init_stmt = ('auto_test = %s.%s(job, testdir, outputdir)' % >>> + (test_name, test_name)) >>> + exec import_stmt + '\n' + init_stmt in locals_dict, >>> globals_dict >>> + client_test = globals_dict['auto_test'] >>> + except ImportError, e: >>> + # skips error if test is control file without python test >>> + if re.search(test_name, str(e)): >>> + pass >>> + # give the user a warning if there is an import error. >>> + else: >>> + logging.error('%s import error: %s. Skipping %s' % >>> + (test_name, e, test_name)) >>> + except Exception, e: >>> + # Log other errors (e.g., syntax errors) and collect the test. >>> + logging.error("%s: %s", test_name, e) >>> + finally: >>> + sys.path.pop(0) # pop up testbindir >>> + return client_test >>> + >>> + >>> def load_all_client_tests(options): >>> """ >>> Load and instantiate all client tests. >>> @@ -60,46 +107,51 @@ def load_all_client_tests(options): >>> for test_base_dir in ['tests', 'site_tests']: >>> testdir = os.path.join(os.environ['AUTODIR'], test_base_dir) >>> for test_name in os.listdir(testdir): >>> - job = setup_job(options=options) >>> - testbindir = os.path.join(testdir, test_name) >>> - local_namespace['testbindir'] = testbindir >>> - >>> - outputdir = os.path.join(job.resultdir, test_name) >>> - try: >>> - os.makedirs(outputdir) >>> - except OSError: >>> - pass >>> - >>> - local_namespace['job'] = job >>> - local_namespace['outputdir'] = outputdir >>> - >>> - sys.path.insert(0, testbindir) >>> - try: >>> - try: >>> - exec("import %s" % test_name, local_namespace, >>> - global_namespace) >>> - exec("auto_test = %s.%s(job, testbindir, outputdir)" >>> % >>> - (test_name, test_name), local_namespace, >>> - global_namespace) >>> - client_test = global_namespace['auto_test'] >>> - all_tests.append(client_test) >>> - except ImportError, e: >>> - # skips error if test is control file without python >>> test >>> - if re.search(test_name, str(e)): >>> - pass >>> - # give the user a warning if there is an import >>> error. >>> - else: >>> - logging.warning("%s import error: %s. Skipping >>> %s" \ >>> - % (test_name, e, test_name)) >>> - except Exception, e: >>> - # Log other errors (e.g., syntax errors) and collect the >>> test. >>> - logging.error("%s: %s", test_name, e) >>> - broken_tests.append(test_name) >>> - finally: >>> - sys.path.pop(0) # pop up testbindir >>> + client_test = init_test(options, os.path.join(testdir, >>> test_name)) >>> + if client_test: >>> + all_tests.append(client_test) >>> + else: >>> + broken_tests.append(test_name) >>> return all_tests, broken_tests >>> >>> >>> +def setup_test(client_test): >>> + """ >>> + Direct invoke test.setup() method. >>> + >>> + @returns A boolean to represent success or not. >>> + """ >>> + >>> + # TODO: check if its already build. .version? hash? >>> + test_name = client_test.__class__.__name__ >>> + cwd = os.getcwd() >>> + good_setup = False >>> + try: >>> + outputdir = os.path.join(client_test.job.resultdir, test_name) >>> + try: >>> + os.makedirs(outputdir) >>> + os.chdir(outputdir) >>> + except OSError: >>> + pass >>> + logging.info('setup %s.' % test_name) >>> + client_test.setup() >>> + >>> + # Touch .version file under src to prevent further setup on >>> client >>> + # host. See client/common_lib/utils.py update_version() >>> + if os.path.exists(client_test.srcdir): >>> + versionfile = os.path.join(client_test.srcdir, '.version') >>> + pickle.dump(client_test.version, open(versionfile, 'w')) >>> + good_setup = True >>> + except Exception, err: >>> + logging.error(err) >>> + raise error.AutoservError('Failed to build client test %s on >>> server.' % >>> + test_name) >>> + finally: >>> + # back to original working dir >>> + os.chdir(cwd) >>> + return good_setup >>> + >>> + >>> def setup_tests(options): >>> """ >>> Load and instantiate all client tests. >>> @@ -138,25 +190,10 @@ def setup_tests(options): >>> logging.error('### No test setup candidates ###') >>> raise error.AutoservError('No test setup candidates.') >>> >>> - for setup_test in need_to_setup: >>> - test_name = setup_test.__class__.__name__ >>> - try: >>> - outputdir = os.path.join(setup_test.job.resultdir, >>> test_name) >>> - try: >>> - os.makedirs(outputdir) >>> - os.chdir(outputdir) >>> - except OSError: >>> - pass >>> - logging.info('setup %s.' % test_name) >>> - setup_test.setup() >>> - # Touch .version file under src to prevent further setup on >>> client >>> - # host. See client/common_lib/utils.py update_version() >>> - if os.path.exists(setup_test.srcdir): >>> - versionfile = os.path.join(setup_test.srcdir, >>> '.version') >>> - pickle.dump(setup_test.version, open(versionfile, 'w')) >>> - except Exception, err: >>> - logging.error(err) >>> - failed_tests.append(test_name) >>> + for client_test in need_to_setup: >>> + good_setup = setup_test(client_test) >>> + if not good_setup: >>> + failed_tests.append(client_test.__class__.__name__) >>> >>> logging.info('############################# SUMMARY ' >>> '#############################') >>> Index: server/autotest.py >>> diff --git a/server/autotest.py b/server/autotest.py >>> index >>> c35f4a1ed1e3b695ef4218170d0466df5d7d5d81..250a83dcb132f7c726ce8320dafd182ba49c997d >>> 100644 >>> --- a/server/autotest.py >>> +++ b/server/autotest.py >>> @@ -2,7 +2,7 @@ >>> >>> import re, os, sys, traceback, subprocess, time, pickle, glob, tempfile >>> import logging, getpass >>> -from autotest_lib.server import installable_object, utils >>> +from autotest_lib.server import installable_object, setup, utils >>> from autotest_lib.client.common_lib import log, error, autotemp >>> from autotest_lib.client.common_lib import global_config, packages >>> from autotest_lib.client.common_lib import utils as client_utils >>> @@ -16,6 +16,11 @@ BOOT_TIME = 1800 >>> CRASH_RECOVERY_TIME = 9000 >>> >>> >>> +get_value = global_config.global_config.get_config_value >>> +autoserv_prebuild = get_value('AUTOSERV', 'enable_server_prebuild', >>> + type=bool, default=False) >>> + >>> + >>> class AutodirNotFoundError(Exception): >>> """No Autotest installation could be found.""" >>> >>> @@ -971,10 +976,17 @@ class client_logger(object): >>> name, pkg_type = self.job.pkgmgr.parse_tarball_name(pkg_name) >>> src_dirs = [] >>> if pkg_type == 'test': >>> - src_dirs += [os.path.join(self.job.clientdir, 'site_tests', >>> name), >>> - os.path.join(self.job.clientdir, 'tests', >>> name)] >>> + for test_dir in ['site_tests', 'tests']: >>> + src_dir = os.path.join(self.job.clientdir, test_dir, >>> name) >>> + if os.path.exists(src_dir): >>> + src_dirs += [src_dir] >>> + if autoserv_prebuild: >>> + setup.setup(self.job.clientdir, src_dir) >>> + break >>> elif pkg_type == 'profiler': >>> src_dirs += [os.path.join(self.job.clientdir, 'profilers', >>> name)] >>> + if autoserv_prebuild: >>> + setup.setup(self.job.clientdir, src_dir) >>> elif pkg_type == 'dep': >>> src_dirs += [os.path.join(self.job.clientdir, 'deps', name)] >>> elif pkg_type == 'client': >>> Index: server/setup.py >>> diff --git a/server/setup.py b/server/setup.py >>> new file mode 100644 >>> index >>> 0000000000000000000000000000000000000000..4d786050e0128315c6b076926f5fed20c40173ee >>> --- /dev/null >>> +++ b/server/setup.py >>> @@ -0,0 +1,64 @@ >>> +# Copyright 2010 Google Inc. Released under the GPL v2 >>> +# >>> +# Eric Li <[email protected]> >>> + >>> +import logging, os, pickle, re, sys >>> +import common >>> +from autotest_lib.client.bin import setup_job as client_setup_job >>> + >>> + >>> +def touch_init(parent_dir, child_dir): >>> + """ >>> + Touch __init__.py file all alone through from dir_patent to >>> child_dir. >>> + >>> + So client tests could be loaded as Python modules. Assume child_dir >>> is a >>> + subdirectory of parent_dir. >>> + """ >>> + >>> + if not child_dir.startswith(parent_dir): >>> + logging.error('%s is not a subdirectory of %s' % (child_dir, >>> + parent_dir)) >>> + return >>> + sub_parent_dirs = parent_dir.split(os.path.sep) >>> + sub_child_dirs = child_dir.split(os.path.sep) >>> + for sub_dir in sub_child_dirs[len(sub_parent_dirs):]: >>> + sub_parent_dirs.append(sub_dir) >>> + path = os.path.sep.join(sub_parent_dirs) >>> + init_py = os.path.join(path, '__init__.py') >>> + open(init_py, 'a').close() >>> + >>> + >>> +def init_test(testdir): >>> + """ >>> + Instantiate a client test object from a given test directory. >>> + >>> + @param testdir The test directory. >>> + @returns A test object or None if failed to instantiate. >>> + """ >>> + >>> + class options: >>> + tag = '' >>> + verbose = None >>> + cont = False >>> + harness = 'autoserv' >>> + hostname = None >>> + user = None >>> + log = True >>> + return client_setup_job.init_test(options, testdir) >>> + >>> + >>> +def setup(autotest_client_dir, client_test_dir): >>> + """ >>> + Setup prebuild of a client test. >>> + >>> + @param autotest_client_dir: The autotest/client base directory. >>> + @param client_test_dir: The actual test directory under client. >>> + """ >>> + >>> + os.environ['AUTODIR'] = autotest_client_dir >>> + touch_init(autotest_client_dir, client_test_dir) >>> + >>> + # instantiate a client_test instance. >>> + client_test = init_test(client_test_dir) >>> + client_setup_job.setup_test(client_test) >>> + >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> >>> On Thu, Mar 25, 2010 at 10:50 AM, Eric Li(李咏竹) <[email protected]>wrote: >>> >>>> I kept working on server side test prebuild. Now it becomes a part of >>>> autoserv, and a flag is introduced so prebuild will be triggered before >>>> client tests/deps/profilers would be tarballed on the server side. >>>> >>>> http://codereview.chromium.org/1317002 >>>> >>>> Please code review it first and after I got LTGM, I will send a diff >>>> patch to Martin to apply. >>>> >>>> I had passed util/run_pylint.py and utils/unittest_suite.py, and I am >>>> not expecting this change should introduce any regression or behavior >>>> change >>>> for existing use cases. >>>> >>>> Thanks. >>>> -- >>>> Eric Li >>>> 李咏竹 >>>> Google Kirkland >>>> >>>> >>>> >>> >>> >>> -- >>> Eric Li >>> 李咏竹 >>> Google Kirkland >>> >>> >>> >>> _______________________________________________ >>> Autotest mailing list >>> [email protected] >>> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest >>> >>> >> > > > -- > Eric Li > 李咏竹 > Google Kirkland > > >
_______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
