[Piglit] [PATCH 1/7] framework_tests/summary.py: Tests Summary.py's assignment of statues

[Kenneth Graunke]

On 09/27/2013 02:39 PM, Dylan Baker wrote:
> This code tests the way Summary.py assigns statuses to tests.
> Specifically it tests regressions, fixes, changes, and problems,
> assuring that only the correct permutations of these tests are added to
> the list.

I pulled your latest 'status-update-v2' branch, where you fixed a bunch
of things I was about to complain about :)  The following comments still
apply to that branch.

Typo in the patch title: s/statues/statuses/

> 
> It uses python's builtin unittest framework, and a magical decorator to
> limit code duplication.
>
> 
> This already reveals ~20 problems in the current implementation
> 
> Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> ---
>  framework_tests/__init__.py |   0
>  framework_tests/helpers.py  | 105 +++++++++++++++++
>  framework_tests/summary.py  | 282 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 387 insertions(+)
>  create mode 100644 framework_tests/__init__.py
>  create mode 100644 framework_tests/helpers.py
>  create mode 100644 framework_tests/summary.py
> 
> diff --git a/framework_tests/__init__.py b/framework_tests/__init__.py
> new file mode 100644
> index 0000000..e69de29
> diff --git a/framework_tests/helpers.py b/framework_tests/helpers.py
> new file mode 100644
> index 0000000..0aea2e1
> --- /dev/null
> +++ b/framework_tests/helpers.py
> @@ -0,0 +1,105 @@
> +#!/usr/bin/env python
> +
> +# Copyright (c) 2013 Intel Corporation
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a
> +# copy of this software and associated documentation files (the "Software"),
> +# to deal in the Software without restriction, including without limitation
> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice (including the next
> +# paragraph) shall be included in all copies or substantial portions of the
> +# Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> +# IN THE SOFTWARE.
> +
> +
> +import sys
> +
> +import framework.core as core
> +
> +
> +def test_iterations(*parameters):
> +    """ Magic that allows a single method to create a whole bunch of functions.
> +
> +    This is desireable becasue of the way unittest works: Each method gets a
> +    name in the output, and each method stops on the first error. This makes
> +    using a loop useless, if 10/20 iterations should fail, the first failure
> +    stops the loop.  The solution other than using a decorator is to create a
> +    test for each iteration, which could result in hundresds of tests.
> +
> +    """
> +
> +    def decorator(method, parameters=parameters):
> +        def tuplify(x):
> +            if not isinstance(x, tuple):
> +                return (x, )
> +            return x
> +
> +        for parameter in map(tuplify, parameters):
> +            name_for_parameter = "{0}({1})".format(method.__name__,
> +                                                   ", ".join(map(repr, parameter)))
> +            frame = sys._getframe(1)  # pylint: disable-msg=W0212
> +            frame.f_locals[name_for_parameter] = \
> +                lambda self, m=method, p=parameter: m(self, *p)
> +        return None
> +    return decorator
> +
> +
> +def create_testresult(name, lspci="fake lspci", glxinfo="fake glxinfo",
> +                      tests={}):
> +    """ Helper function that returns a complete TestResults file
> +
> +    This takes one required argument
> +    :name: This must be set, it names the run. If two runs have the same name
> +           there can be problems in the summary code.
> +
> +    This function also takes three optional arguments
> +    :lspci:   This is the lspci information in the file. Default="fake lspci"
> +    :glxinfo: glxinfo in the file. Default="fake glxinfo"
> +    :tests:   A dictionary of tests
> +
> +    """
> +    assert(isinstance(tests, dict))
> +
> +    return core.TestResult({"options": {"profile": "fake",
> +                                        "filter": [],
> +                                        "exclude_filter": []},
> +                            "name": "{0}".format(name),
> +                            "lspci": "{0}".format(lspci),
> +                            "glxinfo": "{0}".format(glxinfo),
> +                            "time_elapsed": 10.23456789,
> +                            "tests": tests})
> +
> +
> +def create_test(name, result, info="fake info", returncode=0, time=0.123456789,
> +                command="fake command"):
> +    """ Helper function that takes input and returns a dictionary of results
> +    for a single tests

s/a single tests/a single test/

> +
> +    Takes two manditory arguments:

s/manditory/mandatory/

> +    :name: The name of the tests
> +    :result: The result the test returned
> +
> +    Additionally takes four optional arguments:
> +    :info:       The info entry. Default="fake info"
> +    :returncode: The returncode of the test. Default=0
> +    :time:       The amount of time the test ran. Default=0.123456789
> +    :command:    The command that was executed. Default="fake command"
> +
> +    """
> +    #FIXME: This should be able to create other entries for failed tests
> +
> +    return {name: {"info": info,
> +                   "returncode": returncode,
> +                   "time": time,
> +                   "command": command,
> +                   "result": result}}
> diff --git a/framework_tests/summary.py b/framework_tests/summary.py
> new file mode 100644
> index 0000000..c6631a8
> --- /dev/null
> +++ b/framework_tests/summary.py
> @@ -0,0 +1,282 @@
> +#!/usr/bin/env python
> +
> +# Copyright (c) 2013 Intel Corporation
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a
> +# copy of this software and associated documentation files (the "Software"),
> +# to deal in the Software without restriction, including without limitation
> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice (including the next
> +# paragraph) shall be included in all copies or substantial portions of the
> +# Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> +# IN THE SOFTWARE.
> +
> +
> +import os
> +import os.path as path
> +import json
> +import unittest
> +import itertools
> +
> +import framework.summary as summary
> +from helpers import test_iterations, create_testresult, create_test
> +
> +
> +# These lists normally would reside within a the class itself, however, these
> +# are meant to be fed to decorators, and decorates do not have access to class
> +# variables.

Maybe "fed to decorators, which don't have access to class variables."?

> +
> +""" ordering heirarchy from best to worst

s/heirarchy/hierarchy/

Or maybe just: "Status ordering, from best to worst:"

> +
> +pass
> +dmesg-warn
> +warn
> +dmesg-fail
> +fail
> +crash
> +skip
> +
> +
> +The following are regressions:
> +
> +pass|warn|dmesg-warn|fail|dmesg-fail|Crash -> Skip
> +pass|warn|dmesg-warn|fail|dmesg-fail -> Crash|Skip
> +pass|warn|dmesg-warn|fail -> dmesg-fail|Crash|Skip
> +pass|warn|dmesg-warn -> fail|dmesg-fail|Crash|Skip
> +pass|warn -> dmesg-warn|fail|dmesg-fail|Crash|Skip
> +pass -> warn|dmesg-warn|fail|dmesg-fail|Crash|Skip
> +
> +
> +The following are fixes:
> +
> +Skip -> pass|warn|dmesg-warn|fail|dmesg-fail|Crash
> +Crash|Skip - >pass|warn|dmesg-warn|fail|dmesg-fail
> +dmesg-fail|Crash|Skip -> pass|warn|dmesg-warn|fail
> +fail|dmesg-fail|Crash|Skip -> pass|warn|dmesg-warn
> +dmesg-warn|fail|dmesg-fail|Crash|Skip -> pass|warn
> +warn|dmesg-warn|fail|dmesg-fail|Crash|Skip -> pass

Not sure why Crash and Skip are capitalized when nothing else is.

> +
> +NotRun -> * and * -> NotRun is a change, but not a fix or a regression. This is
> +because NotRun is not a status, but a representation of an unkown status.

s/unkown/unknown/

> +
> +"""
> +
> +# List of possible statuses. Add new statuses before 'notrun' or break all
> +# kinds of things.

I think this would be easier to read as:
# List of possible statuses.
# 'notrun' must be last, or all kinds of things will break.

> +STATUSES = ['pass', 'fail', 'skip', 'warn', 'crash', 'dmesg-warn',
> +            'dmesg-fail', 'notrun']
> +
> +# list of possible regresssions
> +REGRESSIONS = (("pass", "warn"),
> +               ("pass", "dmesg-warn"),
> +               ("pass", "fail"),
> +               ("pass", "dmesg-fail"),
> +               ("pass", "skip"),
> +               ("pass", "crash"),
> +               ("dmesg-warn", "warn"),
> +               ("dmesg-warn", "dmesg-fail"),
> +               ("dmesg-warn", "fail"),
> +               ("dmesg-warn", "crash"),
> +               ("dmesg-warn", "skip"),
> +               ("warn", "fail"),
> +               ("warn", "crash"),
> +               ("warn", "skip"),
> +               ("warn", "dmesg-fail"),
> +               ("dmesg-fail", "crash"),
> +               ("dmesg-fail", "skip"),
> +               ("dmesg-fail", "fail"),
> +               ("fail", "crash"),
> +               ("fail", "skip"),
> +               ("crash", "skip"))

At least conceptually, this looks like it ought to be a list of tuples
rather than one giant tuple of tuples.  I realize in Python they're
basically interchangeable, but using [("skip", "crash"), ...] would
probably make more sense.

> +
> +# List of possible fixes
> +FIXES = (("skip", "crash"),
> +         ("skip", "fail"),
> +         ("skip", "dmesg-fail"),
> +         ("skip", "warn"),
> +         ("skip", "dmesg-warn"),
> +         ("skip", "pass"),
> +         ("crash", "fail"),
> +         ("crash", "dmesg-fail"),
> +         ("crash", "warn"),
> +         ("crash", "dmesg-warn"),
> +         ("crash", "pass"),
> +         ("fail", "dmesg-fail"),
> +         ("fail", "warn"),
> +         ("fail", "dmesg-warn"),
> +         ("fail", "pass"),
> +         ("dmesg-fail", "warn"),
> +         ("dmesg-fail", "dmesg-warn"),
> +         ("dmesg-fail", "pass"),
> +         ("warn", "dmesg-warn"),
> +         ("warn", "pass"),
> +         ("dmesg-warn", "pass"))

Ditto.

> +
> +# List of statuses that should be problems.
> +PROBLEMS = ("crash", "fail", "warn", "dmesg-warn", "dmesg-fail")

Likewise, shouldn't this be a list of strings?

> +
> +class StatusTest(unittest.TestCase):
> +    """ Test for summaries handling of status assignments

How about: "Test for status comparisons."?

> +
> +    This test creates summary objects with one or two results that it
> +    generates. These results have a single test in them forming known status
> +    pairs. These pairs are explicitly a fix, regression, or change. The code
> +    then creates a summary and ensures that the status has been added to the
> +    propper field, and not to any additional fields.
> +
> +    """
> +
> +    tmpdir = "/tmp/piglit"
> +
> +    @classmethod
> +    def setUpClass(cls):
> +        # A list of files that should be cleaned up after testing is done
> +        cls.files = []
> +
> +        # Create a directory to store files in
> +        cls.tmpdir = "/tmp/piglit"

Normally, you would use Python's tempfile module for things like this.
It can make you a temporary file (or I think directory) with unique
names.  It's important to use it for a few reasons:

1. People may already have a /tmp/piglit folder containing some other
things, and it's kind of rude to trash their stuff.

2. On non-UNIX systems, /tmp doesn't exist.

3. On a multi-user system, two people might run this at the same time.
Unlikely, but...just another thing using tempfile would solve.

Could you look into fixing this?

> +        try:
> +            os.makedirs(cls.tmpdir)
> +        except OSError:
> +            # os.mkdir (and by extension os.makedirs) quite annoyingly throws
> +            # an error if the directory exists
> +            pass
> +
> +        # Create temporary files in the /tmp/piglit directory
> +        for result in STATUSES[:-1]:  # notrun cannot be generated here
> +            file = path.join(cls.tmpdir, result, "main")
> +            try:
> +                os.mkdir(path.join(cls.tmpdir, result))
> +            except OSError:
> +                pass

pass?  Will it actually work if this doesn't get created?  I'm guessing
you need to abort the test (or just don't catch the exception)...

> +
> +            with open(file, 'w') as f:
> +                json.dump(create_testresult(name=result,
> +                                            tests=create_test("test", result)),
> +                          f, indent=4)
> +
> +            # Add the file to the list of files to be removed in tearDownClass
> +            cls.files.append(file)
> +
> +        # Create an additional file to test a Not Run status. This needs to be
> +        # generated sepeartely since the test result should not include the
> +        # test that will have the not run status.
> +        try:
> +            os.mkdir(path.join(cls.tmpdir, "notrun"))
> +        except OSError:
> +            pass

Ditto...pass?

> +
> +        file = path.join(cls.tmpdir, "notrun", "main")
> +
> +        with open(file, 'w') as f:
> +            # If there are not tests buildDictionary fails
> +            json.dump(create_testresult(name="notrun", 
> +                                        tests=create_test("diff", "pass")),
> +                      f, indent=4)
> +
> +        cls.files.append(path.join(cls.tmpdir, "notrun/main"))
> +
> +    @classmethod
> +    def tearDownClass(cls):
> +        # Remove any folders which were created and are empty
> +        for branch in cls.files:
> +            os.remove(branch)
> +            while branch != '/tmp':
> +                branch = path.dirname(branch)
> +                try:
> +                    os.rmdir(branch)
> +                except OSError:
> +                    # If there is an OSError the directory is not empty, so
> +                    # every additional attempt to call os.rmdir will fail.
> +                    break

I'm glad to see this uses rmdir, so it won't accidentally rm -rf my
system if the path handling goes awry due to some bug.

> +    def _build_lists(self, files):
> +        result = summary.Summary([path.join(self.tmpdir, i) for i in files])
> +        result._Summary__generate_lists(['changes', 'problems', 'skipped',
> +                                         'fixes', 'regressions'])
> +        return result
> +
> +    @test_iterations(*REGRESSIONS)
> +    def test_is_regression(self, x, y):
> +        result = self._build_lists([x, y])
> +        self.assertEqual(
> +            len(result.tests['regressions']), 1,
> +            "{0} -> {1} is not a regression but should be".format(x, y))
> +
> +    # This ugly mother tests every iteration of statuses when the status is not
> +    # equal and if the status is not a valid regression

s/ugly mother// (it's not /that/ ugly)

Also, you say "when the status is not equal", but I don't see anything
enforcing that.  Am I missing something?

I think it's fine to have them be equal; equal statuses should not be a
regression, and that's actually worth testing.

> +    @test_iterations(*itertools.ifilter(lambda x: x not in REGRESSIONS,
> +                                        itertools.product(STATUSES, STATUSES)))
> +    def test_is_not_regression(self, x, y):
> +        result = self._build_lists([x, y])
> +        self.assertEqual(
> +            len(result.tests['regressions']), 0,
> +            "{0} -> {1} is a regression but should not be".format(x, y))
> +
> +    # Test fixes based on the FIXES list
> +    @test_iterations(*FIXES)
> +    def test_is_fix(self, x, y):
> +        result = self._build_lists([x, y])
> +        self.assertEqual(
> +            len(result.tests['fixes']), 1,
> +            "{0} -> {1} is not a fix but should be".format(x, y))
> +
> +
> +    # Test that all combinations taht are not explicitely a fix are not treated
> +    # as fixes
> +    @test_iterations(*itertools.ifilter(lambda x: x not in FIXES,
> +                                        itertools.product(STATUSES, STATUSES)))
> +    def test_is_not_fix(self, x, y):
> +        result = self._build_lists([x, y])
> +        self.assertEqual(
> +            len(result.tests['fixes']), 0,
> +            "{0} -> {1} is a fix but should not be".format(x, y))
> +
> +    #XXX: is "notrun" -> not(notrun) a change?
> +    @test_iterations(*itertools.ifilter(lambda x: x[0] != x[1],
> +                                        itertools.product(STATUSES[:-1],
> +                                                          STATUSES[:-1])))
> +    def test_is_change(self, x, y):
> +        result = self._build_lists([x, y])
> +        self.assertEqual(
> +            len(result.tests['changes']), 1,
> +            "{0} -> {1} is a not a change but should be".format(x, y))
> +
> +    # Tests that two equal statues are not changes
> +    @test_iterations(*itertools.izip(STATUSES, STATUSES))
> +    def test_is_not_change(self, x, y):
> +        result = self._build_lists([x, y])
> +        self.assertEqual(
> +            len(result.tests['changes']), 0,
> +            "{0} -> {1} is a change but should not be".format(x, y))
> +
> +    # only statuses in the PROBLEMS list shoudl be added to problems
> +    @test_iterations(*PROBLEMS)
> +    def test_is_problem(self, x):
> +        result = self._build_lists([x])
> +        self.assertEqual(
> +            len(result.tests['problems']), 1,
> +            "{0} is not a problem but should be".format(x))
> +
> +    # Any statuses not in the PROBLEMS list should not be a problem
> +    @test_iterations(*(i for i in STATUSES if i not in PROBLEMS))
> +    def test_is_not_problem(self, x):
> +        result = self._build_lists([x])
> +        self.assertEqual(
> +            len(result.tests['problems']), 0,
> +            "{0} is a problem but should not be".format(x))
>