RE: [RFC 2/3] Perf test framework

["Agovic, Sanimir" <sanimir.agovic@intel.com>]

lgtm, however I cannot approve your patch.

Some optional comments below.

 -Sanimir

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf
> Of Yao Qi
> Sent: Wednesday, August 28, 2013 06:17 AM
> To: gdb-patches@sourceware.org
> Subject: [RFC 2/3] Perf test framework
> 
> ---
>  gdb/testsuite/gdb.perf/lib/perftest/config.py     |   40 +++++++++++++++++
>  gdb/testsuite/gdb.perf/lib/perftest/perftest.py   |   49 +++++++++++++++++++++
>  gdb/testsuite/gdb.perf/lib/perftest/reporter.py   |   38 ++++++++++++++++
>  gdb/testsuite/gdb.perf/lib/perftest/testresult.py |   42 ++++++++++++++++++
>
I would put a copyright header and a module __doc__ into __init__.py

> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/perftest.py
> b/gdb/testsuite/gdb.perf/lib/perftest/perftest.py
> new file mode 100644
> index 0000000..b15fd39
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/perftest.py
> @@ -0,0 +1,49 @@
[...]
> +
> +class TestCase(gdb.Function):
> +    """Base class of all performance testing cases.  It registers a GDB
> +    convenience function 'perftest'.  Invoke this convenience function
> +    in GDB will call method 'invoke'."""
> +
> +    def __init__(self, result):
> +        # Each test case registers a convenience function 'perftest'.
> +        super (TestCase, self).__init__ ("perftest")
> +        self.result = result
> +
> +    def execute_test(self):
> +        """Abstract method  to do the actual tests."""
> +        raise RuntimeError("Abstract Method.")
>
You may use module abc instead:
| class TestCase(gdb.Function):
|  __metaclass__ = abc.ABCMeta
|  @abc.abstractmethod
|  def execute_test(self): pass

Instantiating TestCase and subclasses without overriding execute_test will fail.

> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/reporter.py
> b/gdb/testsuite/gdb.perf/lib/perftest/reporter.py
> new file mode 100644
> index 0000000..e27b2ae
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/reporter.py
> @@ -0,0 +1,38 @@
> +# Copyright (C) 2013 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +class Reporter(object):
> +    """Base class of reporter, which is about reporting test results in
> +    different formatss."""
> +
> +    def report(self, arg1, arg2, arg3):
>
Using *args should handle most cases well.

> +        raise RuntimeError("Abstract Method.")
> +
> +    def end(self):
> +        """Invoked when reporting is done.  Usually it can be overridden
> +        to do some cleanups, such as closing file descriptors."""
> +        raise RuntimeError("Abstract Method:end.")
>
The doc state _can be overridden_ thus you should provide a default implementation
instead of raising an exception, or write _must be overridden_.

> +
> +class TextReporter(Reporter):
> +    """Report results in plain text 'perftest.log'."""
> +
> +    def __init__(self):
> +        self.txt_log = open ("perftest.log", 'a+');
> +
> +    def report(self, arg1, arg2, arg3):
> +        print >>self.txt_log, '%s %s %s' % (arg1, arg2, arg3)
>
Afaik >> is deprecated, self.txt_log.write(' '.join(args))

> +
> +    def end(self):
> +        self.txt_log.close ()
> diff --git a/gdb/testsuite/gdb.perf/lib/perftest/testresult.py
> b/gdb/testsuite/gdb.perf/lib/perftest/testresult.py
> new file mode 100644
> index 0000000..9912326
> --- /dev/null
> +++ b/gdb/testsuite/gdb.perf/lib/perftest/testresult.py
> @@ -0,0 +1,42 @@
[...]
> +
> +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
> +
> +    def report(self, reporter):
> +        for key in sorted(self.results.iterkeys()):
> +            reporter.report (self.name, key, self.results[key])
> +        reporter.end ()
>
You may use: for key,value in sorted(self.results.iteritems(), key=itemgetter(0))

 -Sanimir
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052