[Piglit] [PATCH 1/7] framework_tests/summary.py: Tests Summary.py's assignment of statues
Kenneth Graunke
kenneth at whitecape.org
Mon Oct 14 07:51:50 CEST 2013
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))
>
More information about the Piglit
mailing list