[CRIU] [PATCH] zdtm.py: add option --ignore-fails

[Sergey Bronnikov]

On 18:21 Mon 14 Mar , Pavel Emelyanov wrote:
> On 03/14/2016 05:14 PM, Sergey Bronnikov wrote:
> > Introduce an option --ignore-fails to make ability to run all planned tests and
> > ignore failed tests. Also with this option zdtm.py will keep image files and
> > output log for each failed test:
> > 
> > [vagrant at fedora test]$ find sergeyb-xxx -maxdepth 3
> > sergeyb-xxx
> > sergeyb-xxx/zdtm_static_bridge_ns
> > sergeyb-xxx/zdtm_static_bridge_ns/images
> > sergeyb-xxx/zdtm_static_bridge_ns/images/1
> > sergeyb-xxx/zdtm_static_bridge_ns/output
> > sergeyb-xxx/zdtm_static_bridge_ns/images.0
> > sergeyb-xxx/zdtm_static_bridge_ns/images.0/1
> > sergeyb-xxx/zdtm_static_bridge_ns/output.0
> > sergeyb-xxx/zdtm_static_deleted_unix_sock_h
> > sergeyb-xxx/zdtm_static_deleted_unix_sock_h/output
> > sergeyb-xxx/zdtm_static_deleted_unix_sock_h/output.0
> > sergeyb-xxx/zdtm_static_deleted_unix_sock_h/output.1
> > 
> > Signed-off-by: Sergey Bronnikov <sergeyb at openvz.org>
> > ---
> >  test/zdtm.py | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/test/zdtm.py b/test/zdtm.py
> > index b523c62..4200f24 100755
> > --- a/test/zdtm.py
> > +++ b/test/zdtm.py
> > @@ -76,6 +76,8 @@ def add_to_report(path, tgt_name):
> >  		if os.path.isdir(path):
> >  			shutil.copytree(path, tgt_path)
> >  		else:
> > +			if not os.path.exists(os.path.dirname(tgt_path)):
> > +				os.mkdir(os.path.dirname(tgt_path))
> >  			shutil.copy2(path, tgt_path)
> >  
> >  
> > @@ -973,12 +975,15 @@ def do_run_test(tname, tdesc, flavs, opts):
> >  			print_sep("Test %s FAIL at %s" % (tname, e.step), '#')
> >  			t.print_output()
> >  			t.kill()
> > -			add_to_report(cr_api.logs(), "cr_logs")
> > +			if cr_api.logs():
> 
> Why did you add this if?

add_to_report fails when path from 1st argument doesn't exist.
For example right now TC zdtm/static/deleted_unix_sock fails for me
without creating image files and add_to_report() fails too.

> > +				add_to_report(cr_api.logs(), tname.replace('/', '_') + "_" + f + "/images")
> >  			if opts['keep_img'] == 'never':
> >  				cr_api.cleanup()
> > -			# This exit does two things -- exits from subprocess and
> > -			# aborts the main script execution on the 1st error met
> > -			sys.exit(1)
> > +			# When option --ignore-fails not specified this exit
> > +			# does two things: exits from subprocess and aborts the
> > +			# main script execution on the 1st error met
> > +			index = list(flavs).index(f) + 128
> 
> Please, introduce global encode_flav() and decode_flav() helpers.

OK

> > +			sys.exit(index)
> >  		else:
> >  			if opts['keep_img'] != 'always':
> >  				cr_api.cleanup()
> > @@ -1033,7 +1038,7 @@ class launcher:
> >  		sub = subprocess.Popen(["./zdtm_ct", "zdtm.py"], \
> >  				env = dict(os.environ, CR_CT_TEST_INFO = arg ), \
> >  				stdout = log, stderr = subprocess.STDOUT)
> > -		self.__subs[sub.pid] = { 'sub': sub, 'log': logf }
> > +		self.__subs[sub.pid] = { 'sub': sub, 'log': logf, 'name': name, 'flavor': flavor }
> 
> You en/de-code flavor index using per-test flavort list. Why not just use the global one?

Will do.

> >  
> >  		if test_flag(desc, 'excl'):
> >  			self.wait()
> > @@ -1045,7 +1050,8 @@ class launcher:
> >  			if status != 0:
> >  				self.__fail = True
> >  				if sub['log']:
> > -					add_to_report(sub['log'], "output")
> > +					failed_flavor = list(sub['flavor'])[os.WEXITSTATUS(status) - 128]
> > +					add_to_report(sub['log'], sub['name'].replace('/', '_') + "_" + failed_flavor + "/output")
> >  
> >  			if sub['log']:
> >  				print open(sub['log']).read()
> > @@ -1064,12 +1070,12 @@ class launcher:
> >  		while self.__subs:
> >  			if not self.__wait_one(os.WNOHANG):
> >  				break
> > -		if self.__fail:
> > +		if (not opts['ignore_fails'] and self.__fail):
> 
> Maybe it's better not to set self.__fail once in case of opts['ignore_fails']?
> 
> >  			raise test_fail_exc('')
> >  
> >  	def wait_all(self):
> >  		self.__wait_all()
> > -		if self.__fail:
> > +		if (not opts['ignore_fails'] and self.__fail):
> >  			raise test_fail_exc('')
> >  
> >  	def finish(self):
> > @@ -1388,7 +1394,7 @@ if os.environ.has_key('CR_CT_TEST_INFO'):
> >  			wpid, status = os.wait()
> >  			if wpid == pid:
> >  				if not os.WIFEXITED(status) or os.WEXITSTATUS(status) != 0:
> 
> The 'or os.WEXITSTATUS(status) check becomes unneeded here.
> 
> > -					status = 1
> > +					status = os.WEXITSTATUS(status)
> >  				break;
> >  
> >  	sys.exit(status)
> > @@ -1427,6 +1433,7 @@ rp.add_argument("--dry-run", help="Don't run tests, just pretend to", action='st
> >  rp.add_argument("-k", "--keep-img", help = "Whether or not to keep images after test",
> >  		choices = [ 'always', 'never', 'failed' ], default = 'failed')
> >  rp.add_argument("--report", help = "Generate summary report in directory")
> > +rp.add_argument("--ignore-fails", help = "Ignore fails and run all planned tests", action = 'store_true')
> 
> I don't insist, but Ivan's version of the option name (--keep-going) looks nicer for me.

It's up to you :) I don't mind.
Should I use description for option from Ivan's version or keep it?

> >  lp = sp.add_parser("list", help = "List tests")
> >  lp.set_defaults(action = list_tests)

-- 
sergeyb@