Re: [RFC 2/3] Perf test framework

[Yao Qi <yao@codesourcery.com>]

Doug, thanks for the review.

On 09/20/2013 03:09 AM, Doug Evans wrote:
>   > +class PerfTestConfig(object):
>   > +    """
>   > +    Create the right objects according to file perftest.ini.
>   > +    """
>   > +
>   > +    def __init__(self):
>   > +        self.config = ConfigParser.ConfigParser()
>   > +        self.config.read("perftest.ini")
>   > +
>   > +    def get_reporter(self):
>   > +        """Create an instance of class Reporter which is determined by
>   > +        the option 'type' in section '[Reporter]'."""
>   > +        if not self.config.has_section('Reporter'):
>   > +            return reporter.TextReporter()
>   > +        if not self.config.has_option('Reporter', 'type'):
>   > +            return reporter.TextReporter()
>   > +
>   > +        name = self.config.get('Reporter', 'type')
>   > +        cls = getattr(reporter, name)
>   > +        return cls()
>   > +
>   > +perftestconfig = PerfTestConfig()
>
> What do you see perftest.ini containing over time?
> While the file format is pretty trivial, it is another file format.
> Do we need it?

I was wondering that we can support json format, so I create class
PerfTestConfig and perftest.ini is used to determine which format to
be used.  I agree that we can remove PerfTestConfig since we
only support only one format (plain text) nowadays.

>   > +
>   > +    def invoke(self):
>   > +        """Call method 'execute_test' and '__report'."""
>
> As I read this, this comment just says what this function is doing.
> I'm guessing the point is to say that all such methods must, at the least,
> do these two things.  This should be spelled out.
> It would also be good to document that "invoke" is what GDB calls
> to perform this function.
>
> Also, I'm wondering why execute_test is public and
> __report(-> _report) is private?

_report is private to be used only in class TestCase.  I double checked 
that private method can be overridden in python, so execute_test can be 
private too.  I'll do it in V2.

>
>   > +
>   > +        self.execute_test()
>   > +        self.__report(perftestconfig.get_reporter())
>   > +        return "Done"
>   > +
>   > +class SingleVariableTestCase(TestCase):
>   > +    """Test case with a single variable."""
>
> I think this needs more documentation.
> What does "single variable" refer to?  A single statistic, like wall time?
>

Yes, like wall time.  How about "SingleStatisticTestCase" or
"SingleParameterTestCase"?

>   > +
>   > +class TestResult(object):
>   > +    """Base class to record or save test results."""
>   > +
>   > +    def __init__(self, name):
>   > +        self.name = name
>   > +
>   > +    def record (self, variable, result):
>   > +        raise RuntimeError("Abstract Method:record.")
>   > +
>   > +    def report (self, reporter):
>   > +        """Report the test results by reporter."""
>   > +        raise RuntimeError("Abstract Method:report.")
>   > +
>   > +class SingleVariableTestResult(TestResult):
>   > +    """Test results for the test case with a single variable. """
>   > +
>   > +    def __init__(self, name):
>   > +        super (SingleVariableTestResult, self).__init__ (name)
>   > +        self.results = dict ()
>   > +
>   > +    def record(self, variable, result):
>   > +        self.results[variable] = result
>
> As things read (to me anyway), the class only handles a single variable,
> but the "record" method makes the variable a parameter.
> There's a disconnect here.
>
> Maybe the "variable" parameter to "record" is misnamed.
> E.g., if testing the wall time of performing something over a range of values,
> e.g., 1 solib, 8, solibs, 256 solibs, "variable" would be 1,8,256?

Yes, for solib test, "variable" is the number of shared libraries, and
"result" is the time usage for loading or unloading such number of
shared libraries.

> If that's the case, please rename "variable".
> I realize it's what is being varied run after run, but
> it just doesn't read well.
>
> There are two "variables" (so to speak) here:
> 1) What one is changing run after run.  E.g. # solibs
> 2) What one is measuring.  E.g. wall time, cpu time, memory used.
>
> The name "variable" feels too ambiguous.
> OTOH, if the performance testing world has a well established convention
> for what the word "variable" means, maybe I could live with it.:-)
>

How about renaming "variable" by "parameter"?

-- 
Yao (齐尧)