* [Fuego] Resending patches
@ 2018-07-25 5:23 Daniel Sangorrin
2018-07-25 5:23 ` [Fuego] [PATCH] ftc: improve test class and defaults handling Daniel Sangorrin
2018-07-31 19:59 ` [Fuego] Resending patches Tim.Bird
0 siblings, 2 replies; 11+ messages in thread
From: Daniel Sangorrin @ 2018-07-25 5:23 UTC (permalink / raw)
To: fuego
Hello Tim,
I have a lot of unmerged code and I would like to
send it to you again in smaller chunks that you can
"digest" better.
[PATCH] ftc: improve test class and defaults handling
This patch improves how test flags (reboot, timeout etc) are
handled in ftc.
Instead of sending a long patch series I would rather
wait for your feedback on this one before sending the next
one (complete the test flags implementation, fix timeouts, etc)
Thanks,
Daniel Sangorrin
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-07-25 5:23 [Fuego] Resending patches Daniel Sangorrin
@ 2018-07-25 5:23 ` Daniel Sangorrin
2018-07-25 5:54 ` Daniel Sangorrin
2018-07-25 5:55 ` Daniel Sangorrin
2018-07-31 19:59 ` [Fuego] Resending patches Tim.Bird
1 sibling, 2 replies; 11+ messages in thread
From: Daniel Sangorrin @ 2018-07-25 5:23 UTC (permalink / raw)
To: fuego
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
Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
engine/scripts/ftc | 170 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 95 insertions(+), 75 deletions(-)
diff --git a/engine/scripts/ftc b/engine/scripts/ftc
index cc1556d..fb07c1d 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'
+ }
+ 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
+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
- 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
+ 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,7 @@ 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
if '-p' in options:
phase_map = { "p": "pre_test",
@@ -3153,7 +3173,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 +3201,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-07-25 5:23 ` [Fuego] [PATCH] ftc: improve test class and defaults handling Daniel Sangorrin
@ 2018-07-25 5:54 ` Daniel Sangorrin
2018-07-31 21:09 ` Tim.Bird
2018-07-25 5:55 ` Daniel Sangorrin
1 sibling, 1 reply; 11+ messages in thread
From: Daniel Sangorrin @ 2018-07-25 5:54 UTC (permalink / raw)
To: fuego
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
Signed-off-by: Daniel Sangorrin <daniel.sangorrin@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'
+ }
+ 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
+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
- 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
+ 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-07-25 5:23 ` [Fuego] [PATCH] ftc: improve test class and defaults handling Daniel Sangorrin
2018-07-25 5:54 ` Daniel Sangorrin
@ 2018-07-25 5:55 ` Daniel Sangorrin
1 sibling, 0 replies; 11+ messages in thread
From: Daniel Sangorrin @ 2018-07-25 5:55 UTC (permalink / raw)
To: fuego
Sorry, I missed a line when preparing the patch.
I just resend it.
> -----Original Message-----
> From: fuego-bounces@lists.linuxfoundation.org
> <fuego-bounces@lists.linuxfoundation.org> On Behalf Of Daniel Sangorrin
> Sent: Wednesday, July 25, 2018 2:24 PM
> To: fuego@lists.linuxfoundation.org
> Subject: [Fuego] [PATCH] ftc: improve test class and defaults handling
>
> 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
>
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
> engine/scripts/ftc | 170
> ++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 95 insertions(+), 75 deletions(-)
>
> diff --git a/engine/scripts/ftc b/engine/scripts/ftc
> index cc1556d..fb07c1d 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'
> + }
> + 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
> +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
>
> - 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
> + 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,7 @@ 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
>
> if '-p' in options:
> phase_map = { "p": "pre_test",
> @@ -3153,7 +3173,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 +3201,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
>
> _______________________________________________
> Fuego mailing list
> Fuego@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/fuego
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Fuego] Resending patches
2018-07-25 5:23 [Fuego] Resending patches Daniel Sangorrin
2018-07-25 5:23 ` [Fuego] [PATCH] ftc: improve test class and defaults handling Daniel Sangorrin
@ 2018-07-31 19:59 ` Tim.Bird
1 sibling, 0 replies; 11+ messages in thread
From: Tim.Bird @ 2018-07-31 19:59 UTC (permalink / raw)
To: daniel.sangorrin, fuego
> -----Original Message-----
> From: Daniel Sangorrin
>
> Hello Tim,
>
> I have a lot of unmerged code and I would like to
> send it to you again in smaller chunks that you can
> "digest" better.
Sounds good. Thanks for breaking things up.
>
> [PATCH] ftc: improve test class and defaults handling
>
> This patch improves how test flags (reboot, timeout etc) are
> handled in ftc.
>
> Instead of sending a long patch series I would rather
> wait for your feedback on this one before sending the next
> one (complete the test flags implementation, fix timeouts, etc)
OK - sorry to not respond sooner. I was out-of-office quite
a bit in July, but my traveling days are (mostly) over until
October, so I should be able to respond more quickly the
next few months.
-- Tim
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-07-25 5:54 ` Daniel Sangorrin
@ 2018-07-31 21:09 ` Tim.Bird
2018-08-01 2:22 ` Daniel Sangorrin
0 siblings, 1 reply; 11+ messages in thread
From: Tim.Bird @ 2018-07-31 21:09 UTC (permalink / raw)
To: daniel.sangorrin, fuego
> -----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?
I'm a bit confused by where all the flags come from. flags can come from"
1) the command line
2) the testplan file
3) the spec file
4) ftc built-in 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.
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-07-31 21:09 ` Tim.Bird
@ 2018-08-01 2:22 ` Daniel Sangorrin
2018-08-01 3:56 ` Tim.Bird
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Sangorrin @ 2018-08-01 2:22 UTC (permalink / raw)
To: Tim.Bird, fuego
> -----Original Message-----
> From: Tim.Bird@sony.com <Tim.Bird@sony.com>
> Sent: Wednesday, August 1, 2018 6:09 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@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@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-08-01 2:22 ` Daniel Sangorrin
@ 2018-08-01 3:56 ` Tim.Bird
2018-08-01 4:38 ` Daniel Sangorrin
0 siblings, 1 reply; 11+ messages in thread
From: Tim.Bird @ 2018-08-01 3:56 UTC (permalink / raw)
To: daniel.sangorrin, fuego
[-- Attachment #1: Type: text/plain, Size: 17057 bytes --]
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@toshiba.co.jp>
Date: Jul 31, 2018, 7:23 PM
To: "Bird, Timothy" <Tim.Bird@sony.com>,fuego@lists.linuxfoundation.org
> -----Original Message-----
> From: Tim.Bird@sony.com <Tim.Bird@sony.com>
> Sent: Wednesday, August 1, 2018 6:09 AM
> To: daniel.sangorrin@toshiba.co.jp; fuego@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@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@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/fuego
[-- Attachment #2: Type: text/html, Size: 31481 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-08-01 3:56 ` Tim.Bird
@ 2018-08-01 4:38 ` Daniel Sangorrin
2018-08-01 5:24 ` Tim.Bird
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Sangorrin @ 2018-08-01 4:38 UTC (permalink / raw)
To: Tim.Bird, fuego
> -----Original Message-----
> From: Tim.Bird@sony.com <Tim.Bird@sony.com>
> 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.
Oops sorry, somehow I missed them because my screen was too narrow and was wrapping your comments.
> > > +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.
OK.
Do you want fuego.conf parameters to override DEFAULT_DEFAULTS or actually create DEFAULT_DEFAULTS from fuego.conf?
> > > + # 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?
test_dict: the flags that come from the command line
test_flags: the flags for a test entry in a testplan (includes default_xxx flags and per-test flags)
I can change the variable names:
test_dict => test_flags_cli
test_flags => test_flags_testplan
testName is special because it can be set by the command line (when you add a single job) or from inside ftc (when you add a testplan)
> > > - 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.
OK, thanks.
> > 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'.
> >
Oh I see. I think the correct change should be:
- job_name = "%s.%s.%s" % (board_name, spec_name, test_name)
+ job_name = "%s.%s.%s" % (board_name, test.spec, test_name)
> > 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.
I see, I can change the names to add _name.
> >
> > 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.
Yes, and they will be removed again when I submit the argparse patches ;P
Thanks,
Daniel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-08-01 4:38 ` Daniel Sangorrin
@ 2018-08-01 5:24 ` Tim.Bird
2018-08-01 16:40 ` Tim.Bird
0 siblings, 1 reply; 11+ messages in thread
From: Tim.Bird @ 2018-08-01 5:24 UTC (permalink / raw)
To: daniel.sangorrin, fuego
> -----Original Message-----
> From: Daniel Sangorrin
>
> > -----Original Message-----
> > From: Tim.Bird@sony.com <Tim.Bird@sony.com>
> > 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.
>
> Oops sorry, somehow I missed them because my screen was too narrow and
> was wrapping your comments.
>
> > > > +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.
>
> OK.
> Do you want fuego.conf parameters to override DEFAULT_DEFAULTS or
> actually create DEFAULT_DEFAULTS from fuego.conf?
create DEFAULT_DEFAULTS from fuego.conf.
>
> > > > + # 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?
>
> test_dict: the flags that come from the command line
> test_flags: the flags for a test entry in a testplan (includes default_xxx flags
> and per-test flags)
>
> I can change the variable names:
> test_dict => test_flags_cli
> test_flags => test_flags_testplan
I think those names would help me understand the code better.
So I'd be in favor of those variable renames.
>
> testName is special because it can be set by the command line (when you
> add a single job) or from inside ftc (when you add a testplan)
>
> > > > - 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.
>
> OK, thanks.
>
> > > 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'.
> > >
>
> Oh I see. I think the correct change should be:
> - job_name = "%s.%s.%s" % (board_name, spec_name, test_name)
> + job_name = "%s.%s.%s" % (board_name, test.spec, test_name)
That's a better fix than what I did. Somehow I missed the act that the
very next line instantiated the 'test' instance. I was worried that 'test' might not
have been ready to use yet.
>
> > > 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.
>
> I see, I can change the names to add _name.
I think it's helpful. Please do so.
>
> > >
> > > 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.
>
> Yes, and they will be removed again when I submit the argparse patches ;P
Sounds good. It's always good to keep things running even during
interim versions.
-- Tim
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Fuego] [PATCH] ftc: improve test class and defaults handling
2018-08-01 5:24 ` Tim.Bird
@ 2018-08-01 16:40 ` Tim.Bird
0 siblings, 0 replies; 11+ messages in thread
From: Tim.Bird @ 2018-08-01 16:40 UTC (permalink / raw)
To: Tim.Bird, daniel.sangorrin, fuego
> -----Original Message-----
> From: Tim.Bird@sony.com
> > -----Original Message-----
> > From: Daniel Sangorrin
> >
> > > -----Original Message-----
> > > From: Tim.Bird@sony.com <Tim.Bird@sony.com>
> > > 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.
> >
> > Oops sorry, somehow I missed them because my screen was too narrow
> and
> > was wrapping your comments.
> >
> > > > > +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.
> >
> > OK.
> > Do you want fuego.conf parameters to override DEFAULT_DEFAULTS or
> > actually create DEFAULT_DEFAULTS from fuego.conf?
> create DEFAULT_DEFAULTS from fuego.conf.
Thinking about this more, I was wrong. In the case where fuego.conf
does not have the default flag values specified, ftc will have to have
some values in DEFAULT_DEFAULTS anyway. There must always
be some default value for a flag, even if it's not specified anywhere
else. And the correct place for that is inside ftc itself.
So, your first option is the correct one: fuego.conf parameters should
override DEFAULT_DEFAULTS.
So the run/job flag precedence should be:
command line flags >
testplan per-test flags >
testplan default_xxx flags >
fuego.conf flags >
DEFAULT_DEFAULTS (ftc built-in flags)
-- Tim
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-08-01 16:40 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-25 5:23 [Fuego] Resending patches Daniel Sangorrin
2018-07-25 5:23 ` [Fuego] [PATCH] ftc: improve test class and defaults handling Daniel Sangorrin
2018-07-25 5:54 ` Daniel Sangorrin
2018-07-31 21:09 ` Tim.Bird
2018-08-01 2:22 ` Daniel Sangorrin
2018-08-01 3:56 ` Tim.Bird
2018-08-01 4:38 ` Daniel Sangorrin
2018-08-01 5:24 ` Tim.Bird
2018-08-01 16:40 ` Tim.Bird
2018-07-25 5:55 ` Daniel Sangorrin
2018-07-31 19:59 ` [Fuego] Resending patches Tim.Bird
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.