[Fuego] [PATCH] ftc: improve test class and defaults handling
Tim.Bird at sony.com
Tim.Bird at sony.com
Wed Aug 1 03:56:19 UTC 2018
Daniel,
Did you see my other comments in the atch review? Especially please look at my last one, and review my setup of spec_name, when no -s option is specified.
- - Tim
-------- Original Message --------
Subject: Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
From: Daniel Sangorrin <daniel.sangorrin at toshiba.co.jp>
Date: Jul 31, 2018, 7:23 PM
To: "Bird, Timothy" <Tim.Bird at sony.com>,fuego at lists.linuxfoundation.org
> -----Original Message-----
> From: Tim.Bird at sony.com <Tim.Bird at sony.com>
> Sent: Wednesday, August 1, 2018 6:09 AM
> To: daniel.sangorrin at toshiba.co.jp; fuego at lists.linuxfoundation.org
> Subject: RE: [Fuego] [PATCH] ftc: improve test class and defaults handling
>
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > Rename plantest_class to test_class because it represents
> > any test, not just a test in a testplan.
> >
> > Move the test flags default values to the test_class to
> > have them shared (see DEFAULT_DEFAULTS).
> >
> > Enforce the priority order of test flags (timeout, reboot, etc)
> > to satisfy the following:
> > commandline flags > per-test flags > default_xxx flags > DEFAULT_DEFAULTS
>
> What about environment variables? Should those be supported as well?
It is possible, but I think that command parameters are enough.
You can always do this:
export FUEGO_REBOOT="true"
ftc run-test -b bbb -t Benchmark.Dhrystone --reboot $FUEGO_REBOOT
Do you know of a special use case where that wouldn't work for you?
> I'm a bit confused by where all the flags come from. flags can come from"
> 1) the command line
Yes
> 2) the testplan file
Yes, there are two.
The default flags for the testplan (default_xxx, see testplan_default.json) and the
per-test flags that can override the testplan defaults.
> 3) the spec file
No.
> 4) ftc built-in defaults.
Yes (what I called DEFAULT_DEFAULTS)
> It would be nice if ftc read defaults from /fuego-ro/conf/fuego.conf, but
> that can be added in another patch if you agree.
Good idea.
I was planning to submit a patch that replaces the code that manages configuration files with code using python configparser. I can implement that functionality on top of that patch if you like.
Thanks,
Daniel
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin at toshiba.co.jp>
> > ---
> > engine/scripts/ftc | 173
> ++++++++++++++++++++++++++++++----------------
> > -------
> > 1 file changed, 98 insertions(+), 75 deletions(-)
> >
> > diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> > index cc1556d..92546bb 100755
> > --- a/engine/scripts/ftc
> > +++ b/engine/scripts/ftc
> > @@ -610,17 +610,28 @@ class board_class:
> > # FIXTHIS - board_class should have methods to read board and dist file
> >
> >
> > -class plantest_class:
> > - def __init__(self, test_dict, defaults):
> > +class test_class:
> > + DEFAULT_DEFAULTS = {
> > + 'timeout' : '30m',
> > + 'spec' : 'default',
> > + 'reboot' : 'false',
> > + 'rebuild' : 'false',
> > + 'precleanup' : 'true',
> > + 'postcleanup' : 'true'
> > + }
>
> Would be nice to read these from fuego.conf.
>
> > + def __init__(self, test_dict, test_flags={}):
> > + merged_defaults = dict(self.DEFAULT_DEFAULTS)
> > + for key, value in test_flags.iteritems():
> > + merged_defaults[key] = value
> > self.name = str(test_dict["testName"])
> > self.test_type = self.name.split(".")[0]
> > self.base_name = self.name.split(".")[1]
> > - self.spec = str(test_dict.get("spec", defaults['spec']))
> > - self.timeout = str(test_dict.get("timeout", defaults['timeout']))
> > - self.reboot = str(test_dict.get("reboot", defaults['reboot']))
> > - self.rebuild = str(test_dict.get("rebuild", defaults['rebuild']))
> > - self.precleanup = str(test_dict.get("precleanup",
> > defaults['precleanup']))
> > - self.postcleanup = str(test_dict.get("postcleanup",
> > defaults['postcleanup']))
> > + self.spec = str(test_dict.get("spec", merged_defaults['spec']))
> > + self.timeout = str(test_dict.get("timeout",
> > merged_defaults['timeout']))
> > + self.reboot = str(test_dict.get("reboot", merged_defaults['reboot']))
> > + self.rebuild = str(test_dict.get("rebuild", merged_defaults['rebuild']))
> > + self.precleanup = str(test_dict.get("precleanup",
> > merged_defaults['precleanup']))
> > + self.postcleanup = str(test_dict.get("postcleanup",
> > merged_defaults['postcleanup']))
> >
> >
> > class run_class:
> > @@ -1371,42 +1382,54 @@ def create_batch_job(board, testplan,
> > plan_tests):
> > sys.exit(1)
> >
> >
> > -# returns a list of plantest_class instances
> > -def parse_testplan(testplan, defaults):
> > +# parse the testplan and returns a list of test_class instances where
> > +# test flags (timeout, reboot, etc) have been merged in this order
> > +# commandline flags > per-test flags > default_xxx flags >
> > DEFAULT_DEFAULTS
> My mailer wrapped this line wrong in this response. The patch looks OK.
>
> > +def parse_testplan(testplan, test_dict):
> > abspath = '/fuego-core/engine/overlays/testplans/' + testplan + '.json'
> >
> > - plan_tests = []
> > with open(abspath, "r") as f:
> > plan = json.load(f)
> >
> > - if 'default_timeout' in plan:
> > - defaults['timeout'] = plan['default_timeout']
> > + plan_tests = []
> > + for plan_test_dict in plan['tests']:
> > + # set testplan flags
> > + test_flags = dict()
> > + if 'default_timeout' in plan:
> > + test_flags['timeout'] = plan['default_timeout']
> >
> > - if 'default_spec' in plan:
> > - defaults['spec'] = plan['default_spec']
> > + if 'default_spec' in plan:
> > + test_flags['spec'] = plan['default_spec']
> >
> > - if 'default_reboot' in plan:
> > - defaults['reboot'] = plan['default_reboot']
> > + if 'default_reboot' in plan:
> > + test_flags['reboot'] = plan['default_reboot']
> >
> > - if 'default_rebuild' in plan:
> > - defaults['rebuild'] = plan['default_rebuild']
> > + if 'default_rebuild' in plan:
> > + test_flags['rebuild'] = plan['default_rebuild']
> >
> > - if 'default_precleanup' in plan:
> > - defaults['precleanup'] = plan['default_precleanup']
> > + if 'default_precleanup' in plan:
> > + test_flags['precleanup'] = plan['default_precleanup']
> >
> > - if 'default_postcleanup' in plan:
> > - defaults['postcleanup'] = plan['default_postcleanup']
> > + if 'default_postcleanup' in plan:
> > + test_flags['postcleanup'] = plan['default_postcleanup']
> >
> > + # override with testplan per-test flags
> > + for key, value in plan_test_dict.iteritems():
> > + if key == "testName":
> > + test_dict[key] = value
> > + continue
> > + test_flags[key] = value
> The distinction between test_dict and test_flags is hard
> to follow here. Why does "testName" get special handling?
>
> >
> > - for test_dict in plan['tests']:
> > - # test_dict is a dictionary with values for the test
> > - test = plantest_class(test_dict, defaults)
> > + # test_class overrides flags with those in test_dict and applies
> > + # DEFAULT_DEFAULTS for non-specified flags
>
> I would rephrase this comment. I think test_class defines the test instance
> using the values from test_dict, with defaults from test_flags and
> DEFAULTS_DEFAULTS
> when values are not defined in test_dict.
>
> > + test = test_class(test_dict, test_flags)
> > plan_tests.append(test)
> >
> > return plan_tests
> >
> >
> > def do_add_jobs(conf, options):
> > + test_dict = {}
> > if '-b' in options:
> > try:
> > board = options[options.index('-b') + 1]
> > @@ -1414,16 +1437,15 @@ def do_add_jobs(conf, options):
> > error_out("Board not provided after -b.")
> > options.remove('-b')
> > options.remove(board)
> > -
> > + test_dict["board"] = board
> > boards = board.split(",")
> > board_list = get_fuego_boards(conf).keys()
> > for board in boards:
> > if board not in board_list:
> > - raise Exception("Board '%s' not found." % board)
> > + error_out("Board '%s' not found." % board)
> > else:
> > - raise Exception("No board name supplied.")
> > + error_out("No board name supplied.")
> >
> > - rebuild = ''
> > if '--rebuild' in options:
> > try:
> > rebuild = options[options.index('--rebuild') + 1]
> > @@ -1431,9 +1453,13 @@ def do_add_jobs(conf, options):
> > error_out('Rebuild option not provided after --rebuild')
> > if rebuild not in ['true', 'false']:
> > error_out("Invalid rebuild option '%s'" % rebuild)
> > + options.remove(rebuild)
> > + options.remove('--rebuild')
> > + test_dict["rebuild"] = rebuild
> >
> > - timeout = '30m'
> > - if '-p' in options:
> > + if '-p' in options and '-t' in options:
> > + error_out('Testplan (-p) and test (-t) options are mutually exclusive.')
> > + elif '-p' in options:
> > try:
> > testplan = options[options.index('-p') + 1]
> > except IndexError:
> > @@ -1447,52 +1473,43 @@ def do_add_jobs(conf, options):
> > elif '-t' in options:
> > # FIXTHIS: have add-jobs support wildcards in the test name
> > test_name, options = get_test_arg("Add-jobs", conf, options)
> > - if '-s' in options:
> > - try:
> > - spec = options[options.index('-s') + 1]
> > - except IndexError:
> > - error_out('Testspec not provided after -s.')
> > - specnames = get_specs(conf, test_name)
> > - if spec not in specnames:
> > - error_out('Unknown spec %s' % spec)
> > - options.remove('-s')
> > - options.remove(spec)
> > - else:
> > - spec = 'default'
> > - if '-k' in options:
> > - try:
> > - timeout = options[options.index('-k') + 1]
> > - options.remove('-k')
> > - except IndexError:
> > - error_out('No timeout specified after -k')
> > -
> > + test_dict["testName"] = test_name
> > else:
> > - error_out('No testplan or testcase supplied.')
> > + error_out('No testplan (-p) or test (-t) supplied.')
> >
> > - defaults = {
> > - 'timeout' : '30m',
> > - 'spec' : 'default',
> > - 'reboot' : 'false',
> > - 'rebuild' : 'false',
> > - 'precleanup' : 'true',
> > - 'postcleanup' : 'true'
> > - }
> > + if '-s' in options:
> > + if test_name is None:
> > + error_out("-s option requires the test option (-t) to be set")
> > + try:
> > + spec = options[options.index('-s') + 1]
> > + except IndexError:
> > + error_out('Testspec not provided after -s.')
> > + specnames = get_specs(conf, test_name)
> > + if spec not in specnames:
> > + error_out('Unknown spec %s' % spec)
> > + options.remove('-s')
> > + options.remove(spec)
> > + test_dict["spec"] = spec
> > +
> > + if '-k' in options:
> > + try:
> > + timeout = options[options.index('-k') + 1]
> > + except IndexError:
> > + error_out('No timeout specified after -k')
> > + if re.match('^\d+[dhms]', timeout) is None:
> > + error_out('%s: Timeout format not supported.' % timeout)
> > + options.remove('-k')
> > + options.remove(timeout)
> > + test_dict["timeout"] = timeout
> >
> > if test_name:
> > - # FIXTHIS - we could parse more parameters for the job here, from the
> > ftc command line
> > - # use all defaults, except for the spec
> > - tp_dict = {"testName": test_name, "timeout": timeout, "spec": spec }
> > - if rebuild:
> > - tp_dict["rebuild"] = rebuild
> > - test = plantest_class(tp_dict, defaults)
> > + test = test_class(test_dict)
> > for board in boards:
> > create_job(board, test)
> > else:
> > - plan_tests = parse_testplan(testplan, defaults)
> > + plan_tests = parse_testplan(testplan, test_dict)
> > for board in boards:
> > for test in plan_tests:
> > - if rebuild:
> > - test.rebuild = rebuild
> > create_job(board, test)
> > create_batch_job(board, testplan, plan_tests)
> > sys.exit(0)
> > @@ -3102,10 +3119,14 @@ def do_run_test(conf, options):
> > board, options = get_board_arg("Run-test", conf, options)
> > board_name = board.name
> >
> > - test_name = None
> > + test_dict = {}
> > +
> > if '-t' in options:
> > # FIXTHIS: have run-test support wildcards in the test name
> > test_name, options = get_test_arg("Run-test", conf, options)
> > + test_dict["testName"] = test_name
> > + else:
> > + error_out("Run-test command requires a test name")
> >
> > if '-s' in options:
> > try:
> > @@ -3117,8 +3138,10 @@ def do_run_test(conf, options):
> > error_out('Unknown spec %s' % spec_name)
> > options.remove(spec_name)
> > options.remove('-s')
> > - else:
> > - spec_name = 'default'
> > + test_dict["spec"] = spec_name
> > +
> > + # apply defaults for test flags that were not specified on the command
> > line
> > + test = test_class(test_dict=test_dict)
> >
> > if '-p' in options:
> > phase_map = { "p": "pre_test",
> > @@ -3153,7 +3176,7 @@ def do_run_test(conf, options):
> > fuego_caller = "ftc"
> > print "Notice: non-Jenkins test request detected"
> >
> > - print "Running test '%s' on board '%s' using spec '%s'" % (test_name,
> > board_name, spec_name)
> > + print "Running test '%s' on board '%s' using spec '%s'" % (test_name,
> > board_name, test.spec)
> >
> > # check if there's a Jenkins job that matches this request
> > # Job names have the syntax: <board>.<spec>.<test_name>
> > @@ -3181,9 +3204,9 @@ def do_run_test(conf, options):
> > build_data.board_name = board_name
> > build_data.test_name = test_name
> > build_data.testplan_name = "None"
> > - build_data.spec_name = spec_name
> > + build_data.spec_name = test.spec
> > build_data.testdir = test_name
> > - build_data.job_name = job_name
> > + build_data.job_name = "%s.%s.%s" % (board_name, test.spec,
> > test_name)
> > build_data.workspace = conf.FUEGO_RW+"/buildzone"
> > build_data.start_time = long(time.time() * 1000)
> >
> > --
> > 2.7.4
>
> When I tried this, I got this:
>
> # ftc run-test -b bbb -t Functional.hello_world
> Notice: non-Jenkins test request detected
> Running test 'Functional.hello_world' on board 'bbb' using spec 'default'
> Traceback (most recent call last):
> File "/usr/local/bin/ftc", line 4420, in <module>
> main()
> File "/usr/local/bin/ftc", line 4380, in main
> do_run_test(conf, options)
> File "/usr/local/bin/ftc", line 3183, in do_run_test
> job_name = "%s.%s.%s" % (board_name, spec_name, test_name)
> UnboundLocalError: local variable 'spec_name' referenced before assignment
>
> It looks like you missed adjusting a reference to spec_name. I was torn about
> whether
> to fix this or not, but decided to go ahead. But my fix doesn't read the
> default spec_name from DEFAULT_DEFAULTS, so you should review it. Instead, if
> no spec_name is specified on the command line, I default it to 'default'.
>
> In general, I tried to use the postfix '_name' (eg. test_name, spec_name), when
> the python variable had the string name of the item, and something else when
> the variable was a dictionary or instance. So, I'm a little uncomfortable with
> test.spec, as it doesn't indicate that this is the string name of the spec, rather
> than a spec instance.
>
> But overall, thanks for the patch. I've applied it, along with my fix for the bug I
> found above. I assume that subsequent patches will add options for specifying
> flags on the command line.
>
> Thanks!
> -- Tim
_______________________________________________
Fuego mailing list
Fuego at lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/fuego
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxfoundation.org/pipermail/fuego/attachments/20180801/eacce2e6/attachment-0001.html>
More information about the Fuego
mailing list