[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