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.dyla...@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)) > _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit