Re: [PATCH 1/4] meta: introduce a small baserunner framework

[Joshua Lock <joshua.g.lock@linux.intel.com>]

Hi Jing, 

Thanks for your submission.

On Tue, 2016-09-13 at 09:17 +0800, jwang wrote:
> From: zjh <junhuix.zhang@intel.com>

There's no commit message here, which makes this harder to review as I
have to try and work out what it's for and why we want it. 

A useful commit message would tell me why we are introducing a
baserunner framework. This may also make a useful comment in the code
itself.

Also, the patch prefix for the series should be "lib/oeqa", i.e

"[PATCH 3/4] lib/oeqa: use baserunner in oetest"

> Signed-off-by: zjh <junhuix.zhang@intel.com>
> ---
>  meta/lib/base/baserunner.py | 60
> +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100755 meta/lib/base/baserunner.py
> 
> diff --git a/meta/lib/base/baserunner.py
> b/meta/lib/base/baserunner.py
> new file mode 100755
> index 0000000..56b838e
> --- /dev/null
> +++ b/meta/lib/base/baserunner.py
> @@ -0,0 +1,60 @@
> +#!/usr/bin/env python

We're defaulting to Python3 in OE-Core nowadays, do you really want
Python2 here (which is the default Python on most Linux distros)

> +# Copyright (C) 2013 Intel Corporation
> +#
> +# Released under the MIT license (see COPYING.MIT)
> +
> +# Base unittest module used by testrunner
> +# This provides the common test runner functionalities including
> manifest input,
> +# xunit output, timeout, tag filtering.
> +
> +"""Base testrunner"""
> +
> +from __future__ import absolute_import
> +import os
> +import sys
> +import time
> +import unittest
> +import shutil
> +
> +class TestContext(object):
> +    '''test context which inject into testcase'''
> +    def __init__(self):
> +        self.target = None
> +
> +class FakeOptions(object):
> +    '''This class just use for configure's defualt arg.
> +       Usually, we use this object in a non comandline
> environment.'''
> +    timeout = 0
> +    def __getattr__(self, name):
> +        return None

What's the purpose of overloading __getattr__() here?

> +
> +class TestRunnerBase(object):
> +    '''test runner base '''

This comment isn't very useful, same for other similar comments in this
series.

> +    def __init__(self, context=None):
> +        self.tclist = []
> +        self.runner = None
> +        self.context = context if context else TestContext()

This would probably be better as: 
self.context = context or TestContext()

or better yet, pass TestContext() as the default param for context,
rather than None?

> +        self.test_result = None
> +        self.run_time = None
> +
> +
> +    def configure(self, options=FakeOptions()):
> +        '''configure before testing'''
> +        pass
> +
> +    def result(self):
> +        '''output test result '''
> +        pass
> +
> +    def loadtest(self, names=None):
> +        '''load test suite'''
> +        pass
> +
> +    def runtest(self, testsuite):
> +        '''run test suite'''
> +        pass
> +
> +    def start(self, testsuite):
> +        '''start testing'''
> +        pass
> +
> -- 
> 2.1.4
>