Re: [Piglit] Require Signed-off-by for patches?
On Wed, Nov 13, 2013 at 5:12 PM, Matt Turner wrote: > On Wed, Nov 13, 2013 at 12:06 PM, Jordan Justen wrote: >> What are the arguments against just following the kernel's >> Signed-off-by practice? > > What are the arguments for it? > > The kernel's submitting patches documentation says that > > - Signed-off-by: this is a developer's certification that he or she has >the right to submit the patch for inclusion into the kernel. It is an >agreement to the Developer's Certificate of Origin, the full text of >which can be found in Documentation/SubmittingPatches. Code without a >proper signoff cannot be merged into the mainline. > > and > > The Signed-off-by: tag indicates that the signer was involved in the > development of the patch, or that he/she was in the patch's delivery path. That text contains one of the arguments. Signed-of-by is a way for the contributor to assert that they are confident the open source project can use the code they've submitted. In other words, they didn't just copy it off some web-site, or out of some closed source code. Yes, this is usually implied by submitting the patch, but with Signed-of-by, they are specifically calling it out. I assume the kernel folks thought about this a fair amount before developing the signature process, so it seems like a good idea to just leverage that. Another argument would be that developers are somewhat likely to already be familiar with the process from the kernel and other open source projects. Frank's feedback on Paul's patch is an example of this. When I started contributing to mesa/piglit I also assumed the same thing since we used all the other parts of the kernel's *-by process. Adding -s to git commit just doesn't seem to be much of burden, nor much of change in process. -Jordan ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH v2 1/2] summary.py: Treat subtests as groups
On Wed, Nov 13, 2013 at 08:51:49AM -0800, Dylan Baker wrote:
> This patch causes tests with subtests to be treated as a group, rather
> than as a test. This means that the status the test itself stores will
> be overwritten by those in the subtest.
>
> There is one oddity about this to be aware of; a test with subtests that
> crashes or fails before any of the subtests run will report a fraction
> of 0/1 with the appropriate color, even though all of the subtests will
> report Not Run.
>
> v2: - Add subtests to the results file as full tests (the internal view
> of the json), without this they will not appear in changes, fixes,
> etc
> - Render the background color of Not Run tests correctly in HTML
> - Apply subtest fractions down the stack
>
> Signed-off-by: Dylan Baker
Both patches are:
Tested-by: Tom Stellard
This is a nice improvement, thanks.
-Tom
> ---
> framework/summary.py | 73
> ++--
> 1 file changed, 59 insertions(+), 14 deletions(-)
>
> diff --git a/framework/summary.py b/framework/summary.py
> index a587712..d31be19 100644
> --- a/framework/summary.py
> +++ b/framework/summary.py
> @@ -161,8 +161,17 @@ class HTMLIndex(list):
> # is a KeyError (a result doesn't contain a particular test),
> # return Not Run, with clas skip for highlighting
> for each in summary.results:
> +# If the "group" at the top of the key heirachy contains
> +# 'subtest' then it is really not a group, link to that page
> try:
> -self._testResult(each.name, key,
> each.tests[key]['result'])
> +if each.tests[path.dirname(key)]['subtest']:
> +href = path.dirname(key)
> +except KeyError:
> +href = key
> +
> +try:
> +self._testResult(each.name, href,
> + summary.status[each.name][key])
> except KeyError:
> self.append({'type': 'other',
> 'text': 'Not Run'})
> @@ -221,8 +230,14 @@ class HTMLIndex(list):
> displaying pass/fail/crash/etc and formatting the cell to the
> correct color.
> """
> +# "Not Run" is not a valid class, if it apears set the class to skip
> +if isinstance(text, so.NotRun):
> +css = 'skip'
> +else:
> +css = text
> +
> self.append({'type': 'testResult',
> - 'class': text,
> + 'class': css,
> 'href': path.join(group, href + ".html"),
> 'text': text})
>
> @@ -277,19 +292,49 @@ class Summary:
> fraction = self.fractions[results.name]
> status = self.status[results.name]
>
> +# store the results to be appeneded to results. Adding them in
> the
> +# loop will cause a RuntimeError
> +temp_results = {}
> +
> for key, value in results.tests.iteritems():
> -#FIXME: Add subtest support
> -
> -# Walk the test name as if it was a path, at each level
> update
> -# the tests passed over the total number of tests
> (fractions),
> -# and update the status of the current level if the status of
> -# the previous level was worse, but is not skip
> -while key != '':
> -fgh(key, value['result'])
> -key = path.dirname(key)
> -
> -# when we hit the root update the 'all' group and stop
> -fgh('all', value['result'])
> +# Treat a test with subtests as if it is a group, assign the
> +# subtests' statuses and fractions down to the test, and then
> +# proceed like normal.
> +try:
> +for (subt, subv) in value['subtest'].iteritems():
> +subt = path.join(key, subt)
> +subv = so.status_lookup(subv)
> +
> +# Add the subtest to the fractions and status lists
> +fraction[subt] = subv.fraction
> +status[subt] = subv
> +temp_results.update({subt: {'result': subv}})
> +
> +self.tests['all'].add(subt)
> +while subt != '':
> +fgh(subt, subv)
> +subt = path.dirname(subt)
> +fgh('all', subv)
> +
> +# remove the test from the 'all' list, this will cause to
> +# be treated as a group
> +self.tests['all'].discard(key)
> +except KeyError:
> +# Walk the test name as if it was a path, at each lev
Re: [Piglit] Require Signed-off-by for patches?
On Wed, Nov 13, 2013 at 12:06 PM, Jordan Justen wrote: > What are the arguments against just following the kernel's > Signed-off-by practice? What are the arguments for it? The kernel's submitting patches documentation says that - Signed-off-by: this is a developer's certification that he or she has the right to submit the patch for inclusion into the kernel. It is an agreement to the Developer's Certificate of Origin, the full text of which can be found in Documentation/SubmittingPatches. Code without a proper signoff cannot be merged into the mainline. and The Signed-off-by: tag indicates that the signer was involved in the development of the patch, or that he/she was in the patch's delivery path. I think we're just getting into Parkinson's law of triviality... ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] framework: Add support for a test timeout
On Wed, Nov 13, 2013 at 10:03 PM, Dylan Baker wrote: >> +class Timeout(Status): >> +name = 'timeout' >> +value = 50 >> +fraction = (0, 0) > > Are you sure that you want this to be (0, 0)? That means that tests with this > status will not be counted toward the total number of tests. I don't think > you want to change this from the default (0, 1), but if you're sure I wont > argue :) Indeed, that's just copypaste fail. Fixed and patches pushed, thanks for your review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] Require Signed-off-by for patches?
On Wed, Nov 13, 2013 at 12:08 PM, Paul Berry wrote: > On 13 November 2013 12:06, Jordan Justen wrote: >> >> On Wed, Nov 13, 2013 at 11:26 AM, Paul Berry >> wrote: >> > On 13 November 2013 11:01, Frank Henigman wrote: >> >> >> >> I'd like to see an explanation of "signed-off-by," "reviewed-by" etc. >> >> Maybe as simple as: >> >> >> >> "Use the tags signed-off-by, reviewed-by, tested-by, acked-by as for >> >> linux >> >> kernel patches >> >> (see https://www.kernel.org/doc/Documentation/SubmittingPatches)." >> > >> > >> > That seems reasonable. Note, however, that Piglit doesn't consistently >> > use >> > "signed-off-by": >> > >> > $ git log master | grep '^commit' | wc >> >48859770 234480 >> > $ git log master | grep -i 'signed-off-by' | wc >> >12414969 70925 >> > >> > (If you'd like to encourage us to start using "signed-off-by" >> > consistently, >> > I'm happy to have a policy discussion about that, but the discussion >> > should >> > happen in its own email thread rather than here, so that more people >> > will >> > see it). >> >> What are the arguments against just following the kernel's >> Signed-off-by practice? >> >> It can't be difficultly since 'git commit -s' makes this trivial. :) >> > I don't have a particularly strong opinion either way. I just wanted to > make sure that if we decide to require it, the decision happens in the open > rather than in the reply to a patch, where it might get missed by a lot of > people. Whoops. I guess I needed a bit of a transition before that question. :) When I asked "What are the arguments against just following the kernel's Signed-off-by practice?" I was directing the question to the list. In other words, lets have the discussion you were referring to. -Jordan ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 1/2] framework/summary.py: Set constants for directories
On Wed, Nov 13, 2013 at 12:51:27PM -0800, Dylan Baker wrote: > PIGLIT_SOURCE_DIR is always set in core.py in an way nearly identical to the > way you're looking for piglitDir, which we import, so I don't think this > should be an issue Oh, in that case, looks good to me. You probably want a r-b tag from someone knowing piglit better though, I've just looked at it for the first time today for our usage in intel-gpu-tools. In any case, that's what I want and done as well, making the html summary script run with an installed piglit. Reviewed-by: Damien Lespiau -- Damien ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 1/2] framework/summary.py: Set constants for directories
On Wednesday, November 13, 2013 12:51:27 PM Dylan Baker wrote: > On Wednesday, November 13, 2013 07:46:31 PM Damien Lespiau wrote: > > On Wed, Nov 13, 2013 at 11:12:24AM -0800, Dylan Baker wrote: > > > This uses python's tempfile module to get a platform dependent place to > > > put temporary files, which should allow html summary to work even when > > > installed somewhere read-only. > > > > > > Signed-off-by: Dylan Baker > > > --- > > > > > > framework/summary.py | 25 +++-- > > > 1 file changed, 15 insertions(+), 10 deletions(-) > > > > > > diff --git a/framework/summary.py b/framework/summary.py > > > index a587712..bbb423f 100644 > > > --- a/framework/summary.py > > > +++ b/framework/summary.py > > > @@ -24,6 +24,7 @@ import os.path as path > > > > > > import itertools > > > import shutil > > > import collections > > > > > > +import tempfile > > > > > > from mako.template import Template > > > > > > # a local variable status exists, prevent accidental overloading by > > > renaming> > > > > > > @@ -234,6 +235,8 @@ class Summary: > > > uses methods to generate various kinds of output. The reference > > > implementation is HTML output through mako, aptly named > > > generateHTML(). > > > """ > > > > > > +TEMP_DIR = path.join(tempfile.gettempdir(), "piglit/html-summary") > > > +TEMPLATE_DIR = path.join(os.environ['PIGLIT_SOURCE_DIR'], > > > 'templates') > > > > I had something a bit less manual here: > > > > +piglitDir = > > path.normpath(path.join(path.dirname(path.realpath(__file__)), os.pardir)) > > +templatesDir = path.join(piglitDir, "templates") > > > > Can we have PIGLIT_SOURCE_DIR as an override of top of this? > > PIGLIT_SOURCE_DIR is always set in core.py in an way nearly identical to the > way you're looking for piglitDir, which we import, so I don't think this > should be an issue I forgot, the reason that we're using tempdir for the templates is that the install directory should not be assumed to be writable, since on modern OSes system install paths are not. signature.asc Description: This is a digitally signed message part. ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] framework: Add support for a test timeout
On Wednesday, November 13, 2013 06:16:34 PM Daniel Vetter wrote:
> i-g-t tests can take a long time, and for a bunch of reasons (bad
> tuning on disparate set of platforms, stuck in the kernel which often
> can be recovered by interrupting with a signal, ...) that sometimes
> extends to a good approximation of forever.
>
> Hence this adds a per-test timeout value and a bit of infrastructure
> to adjust the results. Test results adjusting is done after calling
> interpretResult so that we don't have to replicate this all over the
> place. This might need to be adjusted for the piglit-native subtest
> stuff, but otoh igt is a bit special with it's crazy long-running
> tests. So I think we can fix this once it's actually needed.
>
> The default timeout is None, so this is purely opt-in.
>
> Note on the implementation: SIG_ALARM doesn't exists on Windows and
> stackoverflow overwhelmingly recommended to go with this thread-based
> approach here. But only tested on my Linux box here.
>
> I've also timed quick.tests run a bit and the overhead due to the
> additional thread we launch seems to be in the wash. So I didn't opt
> to make the thread launching optional if there's no timeout limit.
>
> v2: Also add all the boilerplate needed to handle the new test status
> in summaries. For the color I've opted for two shades of light blue,
> they nicely stick out from the current selection.
>
> v3: Fix GLSLParserTest and ShaderTest. They both derive from
> PlainExecTest but only call the __init__ function of the Test
> baseclass. I haven't really figured why this is and also not really
> what command I should pass to PlainExecTest.__init__, so just
> replicated the init code for now.
This should be fine, I'm working on refactoring the Test, ExecTest, and
PlainExecTest classes anyway, I'd like to get down to just one or two of them
>
> v4: Initialize timeout earlier for tests where we use the ENOENT
> handling to skip them.
>
> Fix up the out, err passing from the thread. Apparently my idea of how
> python closures work was completely misguided - like with a function
> call a closure captures a copy of of a reference pointing at the
> original object. So assinging anything to them won't have any effect
> outside of the lambda. Hence we need to jump through hoops and pass an
> array around. Nicer fix would be to create a class or something, but
> that seems like overkill.
>
> Cc: Dylan Baker
> Signed-off-by: Daniel Vetter
> ---
> framework/exectest.py | 28 +---
> framework/glsl_parser_test.py | 1 +
> framework/shader_test.py | 1 +
> framework/status.py | 39 ++-
> templates/index.css | 5 -
> 5 files changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/framework/exectest.py b/framework/exectest.py
> index 986d8032cb37..e239940fe891 100644
> --- a/framework/exectest.py
> +++ b/framework/exectest.py
> @@ -23,6 +23,7 @@
> import errno
> import os
> import subprocess
> +import threading
> import shlex
> import types
> import re
> @@ -72,6 +73,7 @@ class ExecTest(Test):
> self.command = command
> self.split_command = os.path.split(self.command[0])[1]
> self.env = {}
> +self.timeout = None
>
> if isinstance(self.command, basestring):
> self.command = shlex.split(str(self.command))
> @@ -113,7 +115,7 @@ class ExecTest(Test):
> else:
> if env.dmesg:
> old_dmesg = read_dmesg()
> -(out, err, returncode) = \
> +(out, err, returncode, timeout) = \
> self.get_command_result(command, fullenv)
> if env.dmesg:
> dmesg_diff = get_dmesg_diff(old_dmesg,
> read_dmesg()) @@ -172,6 +174,9 @@ class ExecTest(Test):
> elif returncode != 0:
> results['note'] = 'Returncode was {0}'.format(returncode)
>
> +if timeout:
> +results['result'] = 'timeout'
> +
> if env.valgrind:
> # If the underlying test failed, simply report
> # 'skip' for this valgrind test.
> @@ -196,6 +201,7 @@ class ExecTest(Test):
> results['returncode'] = returncode
> results['command'] = ' '.join(self.command)
> results['dmesg'] = dmesg_diff
> +results['timeout'] = timeout
>
> self.handleErr(results, err)
>
> @@ -217,13 +223,29 @@ class ExecTest(Test):
>
> def get_command_result(self, command, fullenv):
> try:
> +timeout = False
> proc = subprocess.Popen(command,
> stdout=subprocess.PIPE,
> stderr=subprocess.PIPE,
> env=fullenv,
> universal_newlines=True)
> -
Re: [Piglit] Integrating external test suites with piglit
On Wednesday, November 13, 2013 01:00:39 PM Tom Stellard wrote: > Hi, > > I've been doing a lot of testing lately with the OpenCV library > (http://opencv.org). It has its own gtest based regression test suite, > and so far I've been trying to port some of these tests over to piglit. > Unfortunately, this has been very difficult and time consuming. Mostly, > because it is nearly impossible to extract the input data and the expected > output data from these tests. > > As a result, I've been thinking about trying to integrate the opencv test > program into piglit. My initial idea was to add a CMake configuration > option so you could specify a path to opencv's test program and then > write some sort of piglit-gtest module for parsing the output. > > I'm curious if anyone has tried to do something similar or if > someone has some other suggestions for how to make this work. > > Thanks, > Tom > ___ > Piglit mailing list > Piglit@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/piglit We already have support for three external suites (intel gpu tools, es3conform, and oglconform), generally the suggestion is to just provide a symlink to the binaries in bin, and then subclass exectest.ExecTest with the appropriate interpret_results() method. signature.asc Description: This is a digitally signed message part. ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 1/2] framework/summary.py: Set constants for directories
On Wednesday, November 13, 2013 07:46:31 PM Damien Lespiau wrote: > On Wed, Nov 13, 2013 at 11:12:24AM -0800, Dylan Baker wrote: > > This uses python's tempfile module to get a platform dependent place to > > put temporary files, which should allow html summary to work even when > > installed somewhere read-only. > > > > Signed-off-by: Dylan Baker > > --- > > > > framework/summary.py | 25 +++-- > > 1 file changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/framework/summary.py b/framework/summary.py > > index a587712..bbb423f 100644 > > --- a/framework/summary.py > > +++ b/framework/summary.py > > @@ -24,6 +24,7 @@ import os.path as path > > > > import itertools > > import shutil > > import collections > > > > +import tempfile > > > > from mako.template import Template > > > > # a local variable status exists, prevent accidental overloading by > > renaming> > > @@ -234,6 +235,8 @@ class Summary: > > uses methods to generate various kinds of output. The reference > > implementation is HTML output through mako, aptly named > > generateHTML(). > > """ > > > > +TEMP_DIR = path.join(tempfile.gettempdir(), "piglit/html-summary") > > +TEMPLATE_DIR = path.join(os.environ['PIGLIT_SOURCE_DIR'], > > 'templates') > > I had something a bit less manual here: > > +piglitDir = > path.normpath(path.join(path.dirname(path.realpath(__file__)), os.pardir)) > +templatesDir = path.join(piglitDir, "templates") > > Can we have PIGLIT_SOURCE_DIR as an override of top of this? PIGLIT_SOURCE_DIR is always set in core.py in an way nearly identical to the way you're looking for piglitDir, which we import, so I don't think this should be an issue signature.asc Description: This is a digitally signed message part. ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] Require Signed-off-by for patches?
On 13 November 2013 12:06, Jordan Justen wrote: > On Wed, Nov 13, 2013 at 11:26 AM, Paul Berry > wrote: > > On 13 November 2013 11:01, Frank Henigman wrote: > >> > >> I'd like to see an explanation of "signed-off-by," "reviewed-by" etc. > >> Maybe as simple as: > >> > >> "Use the tags signed-off-by, reviewed-by, tested-by, acked-by as for > linux > >> kernel patches > >> (see https://www.kernel.org/doc/Documentation/SubmittingPatches)." > > > > > > That seems reasonable. Note, however, that Piglit doesn't consistently > use > > "signed-off-by": > > > > $ git log master | grep '^commit' | wc > >48859770 234480 > > $ git log master | grep -i 'signed-off-by' | wc > >12414969 70925 > > > > (If you'd like to encourage us to start using "signed-off-by" > consistently, > > I'm happy to have a policy discussion about that, but the discussion > should > > happen in its own email thread rather than here, so that more people will > > see it). > > What are the arguments against just following the kernel's > Signed-off-by practice? > > It can't be difficultly since 'git commit -s' makes this trivial. :) > > -Jordan > I don't have a particularly strong opinion either way. I just wanted to make sure that if we decide to require it, the decision happens in the open rather than in the reply to a patch, where it might get missed by a lot of people. ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] Require Signed-off-by for patches?
On Wed, Nov 13, 2013 at 11:26 AM, Paul Berry wrote: > On 13 November 2013 11:01, Frank Henigman wrote: >> >> I'd like to see an explanation of "signed-off-by," "reviewed-by" etc. >> Maybe as simple as: >> >> "Use the tags signed-off-by, reviewed-by, tested-by, acked-by as for linux >> kernel patches >> (see https://www.kernel.org/doc/Documentation/SubmittingPatches)." > > > That seems reasonable. Note, however, that Piglit doesn't consistently use > "signed-off-by": > > $ git log master | grep '^commit' | wc >48859770 234480 > $ git log master | grep -i 'signed-off-by' | wc >12414969 70925 > > (If you'd like to encourage us to start using "signed-off-by" consistently, > I'm happy to have a policy discussion about that, but the discussion should > happen in its own email thread rather than here, so that more people will > see it). What are the arguments against just following the kernel's Signed-off-by practice? It can't be difficultly since 'git commit -s' makes this trivial. :) -Jordan ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 1/2] framework/summary.py: Set constants for directories
On Wed, Nov 13, 2013 at 11:12:24AM -0800, Dylan Baker wrote: > This uses python's tempfile module to get a platform dependent place to > put temporary files, which should allow html summary to work even when > installed somewhere read-only. > > Signed-off-by: Dylan Baker > --- > framework/summary.py | 25 +++-- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/framework/summary.py b/framework/summary.py > index a587712..bbb423f 100644 > --- a/framework/summary.py > +++ b/framework/summary.py > @@ -24,6 +24,7 @@ import os.path as path > import itertools > import shutil > import collections > +import tempfile > from mako.template import Template > > # a local variable status exists, prevent accidental overloading by renaming > @@ -234,6 +235,8 @@ class Summary: > uses methods to generate various kinds of output. The reference > implementation is HTML output through mako, aptly named generateHTML(). > """ > +TEMP_DIR = path.join(tempfile.gettempdir(), "piglit/html-summary") > +TEMPLATE_DIR = path.join(os.environ['PIGLIT_SOURCE_DIR'], 'templates') I had something a bit less manual here: +piglitDir = path.normpath(path.join(path.dirname(path.realpath(__file__)), os.pardir)) +templatesDir = path.join(piglitDir, "templates") Can we have PIGLIT_SOURCE_DIR as an override of top of this? -- Damien ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH 2/2] CMakeLists.txt: install templates directory
On Wed, Nov 13, 2013 at 11:12:25AM -0800, Dylan Baker wrote: > Without this piglit-summary-html will not work when piglit is installed. > > Signed-off-by: Dylan Baker I had the same patch here, so, yes please! Reviewed-by: Damien Lespiau -- Damien ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] [PATCH] Add a comment to HACKING describing the use of "*-by" in commit messages.
Suggested-by: Frank Henigman --- HACKING | 6 ++ 1 file changed, 6 insertions(+) diff --git a/HACKING b/HACKING index 9eff4ef..738ae18 100644 --- a/HACKING +++ b/HACKING @@ -159,6 +159,12 @@ a good place for general discussion about piglit development, such as future plans for the project, and coordinating work between developers. +Note that Piglit patches use the terms "Reviewed-by", "Tested-by", and +"Acked-by" in the same way as they are used for Linux kernel patches +(see https://www.kernel.org/doc/Documentation/SubmittingPatches). You +are also welcome to add a "Signed-off-by" line to your patch, but it +is not required. + For developers who are new to piglit: when submitting a patch, it is helpful to add a note (after the "---" line in the patch file) indicating that you are new to the project and don't have commit -- 1.8.4.2 ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] Add a "Contributing Patches" section to the HACKING file.
On 13 November 2013 11:01, Frank Henigman wrote: > I'd like to see an explanation of "signed-off-by," "reviewed-by" etc. > Maybe as simple as: > > "Use the tags signed-off-by, reviewed-by, tested-by, acked-by as for linux > kernel patches > (see https://www.kernel.org/doc/Documentation/SubmittingPatches)." > That seems reasonable. Note, however, that Piglit doesn't consistently use "signed-off-by": $ git log master | grep '^commit' | wc 48859770 234480 $ git log master | grep -i 'signed-off-by' | wc 12414969 70925 (If you'd like to encourage us to start using "signed-off-by" consistently, I'm happy to have a policy discussion about that, but the discussion should happen in its own email thread rather than here, so that more people will see it). I'll follow up with a patch taking your suggestion into account. ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] [PATCH 2/2] CMakeLists.txt: install templates directory
Without this piglit-summary-html will not work when piglit is installed. Signed-off-by: Dylan Baker --- CMakeLists.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3cfd76d..9bbffe9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -379,6 +379,11 @@ install ( ) install ( +DIRECTORY templates +DESTINATION . +) + +install ( DIRECTORY tests DESTINATION . FILES_MATCHING REGEX ".*\\.(tests|program_test|shader_test|frag|vert|geom|ktx|cl|txt)$" -- 1.8.3.2 ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] [PATCH 1/2] framework/summary.py: Set constants for directories
This uses python's tempfile module to get a platform dependent place to
put temporary files, which should allow html summary to work even when
installed somewhere read-only.
Signed-off-by: Dylan Baker
---
framework/summary.py | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/framework/summary.py b/framework/summary.py
index a587712..bbb423f 100644
--- a/framework/summary.py
+++ b/framework/summary.py
@@ -24,6 +24,7 @@ import os.path as path
import itertools
import shutil
import collections
+import tempfile
from mako.template import Template
# a local variable status exists, prevent accidental overloading by renaming
@@ -234,6 +235,8 @@ class Summary:
uses methods to generate various kinds of output. The reference
implementation is HTML output through mako, aptly named generateHTML().
"""
+TEMP_DIR = path.join(tempfile.gettempdir(), "piglit/html-summary")
+TEMPLATE_DIR = path.join(os.environ['PIGLIT_SOURCE_DIR'], 'templates')
def __init__(self, resultfiles):
"""
@@ -346,18 +349,20 @@ class Summary:
"""
# Copy static files
-shutil.copy("templates/index.css", path.join(destination, "index.css"))
-shutil.copy("templates/result.css", path.join(destination,
"result.css"))
+shutil.copy(path.join(self.TEMPLATE_DIR, "index.css"),
+path.join(destination, "index.css"))
+shutil.copy(path.join(self.TEMPLATE_DIR, "result.css"),
+path.join(destination, "result.css"))
# Create the mako object for creating the test/index.html file
-testindex = Template(filename="templates/testrun_info.mako",
+testindex = Template(filename=path.join(self.TEMPLATE_DIR,
"testrun_info.mako"),
output_encoding="utf-8",
- module_directory=".makotmp")
+ module_directory=self.TEMP_DIR)
# Create the mako object for the individual result files
-testfile = Template(filename="templates/test_result.mako",
+testfile = Template(filename=path.join(self.TEMPLATE_DIR,
"test_result.mako"),
output_encoding="utf-8",
-module_directory=".makotmp")
+module_directory=self.TEMP_DIR)
result_css = path.join(destination, "result.css")
index = path.join(destination, "index.html")
@@ -404,13 +409,13 @@ class Summary:
index=path.relpath(index, temp_path)))
# Finally build the root html files: index, regressions, etc
-index = Template(filename="templates/index.mako",
+index = Template(filename=path.join(self.TEMPLATE_DIR, "index.mako"),
output_encoding="utf-8",
- module_directory=".makotmp")
+ module_directory=self.TEMP_DIR)
-empty_status = Template(filename="templates/empty_status.mako",
+empty_status = Template(filename=path.join(self.TEMPLATE_DIR,
"empty_status.mako"),
output_encoding="utf-8",
-module_directory=".makotmp")
+module_directory=self.TEMP_DIR)
pages = ('changes', 'problems', 'skipped', 'fixes', 'regressions')
--
1.8.3.2
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] Integrating external test suites with piglit
Hi, I've been doing a lot of testing lately with the OpenCV library (http://opencv.org). It has its own gtest based regression test suite, and so far I've been trying to port some of these tests over to piglit. Unfortunately, this has been very difficult and time consuming. Mostly, because it is nearly impossible to extract the input data and the expected output data from these tests. As a result, I've been thinking about trying to integrate the opencv test program into piglit. My initial idea was to add a CMake configuration option so you could specify a path to opencv's test program and then write some sort of piglit-gtest module for parsing the output. I'm curious if anyone has tried to do something similar or if someone has some other suggestions for how to make this work. Thanks, Tom ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] [PATCH] framework: Add support for a test timeout
i-g-t tests can take a long time, and for a bunch of reasons (bad
tuning on disparate set of platforms, stuck in the kernel which often
can be recovered by interrupting with a signal, ...) that sometimes
extends to a good approximation of forever.
Hence this adds a per-test timeout value and a bit of infrastructure
to adjust the results. Test results adjusting is done after calling
interpretResult so that we don't have to replicate this all over the
place. This might need to be adjusted for the piglit-native subtest
stuff, but otoh igt is a bit special with it's crazy long-running
tests. So I think we can fix this once it's actually needed.
The default timeout is None, so this is purely opt-in.
Note on the implementation: SIG_ALARM doesn't exists on Windows and
stackoverflow overwhelmingly recommended to go with this thread-based
approach here. But only tested on my Linux box here.
I've also timed quick.tests run a bit and the overhead due to the
additional thread we launch seems to be in the wash. So I didn't opt
to make the thread launching optional if there's no timeout limit.
v2: Also add all the boilerplate needed to handle the new test status
in summaries. For the color I've opted for two shades of light blue,
they nicely stick out from the current selection.
v3: Fix GLSLParserTest and ShaderTest. They both derive from
PlainExecTest but only call the __init__ function of the Test
baseclass. I haven't really figured why this is and also not really
what command I should pass to PlainExecTest.__init__, so just
replicated the init code for now.
v4: Initialize timeout earlier for tests where we use the ENOENT
handling to skip them.
Fix up the out, err passing from the thread. Apparently my idea of how
python closures work was completely misguided - like with a function
call a closure captures a copy of of a reference pointing at the
original object. So assinging anything to them won't have any effect
outside of the lambda. Hence we need to jump through hoops and pass an
array around. Nicer fix would be to create a class or something, but
that seems like overkill.
Cc: Dylan Baker
Signed-off-by: Daniel Vetter
---
framework/exectest.py | 28 +---
framework/glsl_parser_test.py | 1 +
framework/shader_test.py | 1 +
framework/status.py | 39 ++-
templates/index.css | 5 -
5 files changed, 57 insertions(+), 17 deletions(-)
diff --git a/framework/exectest.py b/framework/exectest.py
index 986d8032cb37..e239940fe891 100644
--- a/framework/exectest.py
+++ b/framework/exectest.py
@@ -23,6 +23,7 @@
import errno
import os
import subprocess
+import threading
import shlex
import types
import re
@@ -72,6 +73,7 @@ class ExecTest(Test):
self.command = command
self.split_command = os.path.split(self.command[0])[1]
self.env = {}
+self.timeout = None
if isinstance(self.command, basestring):
self.command = shlex.split(str(self.command))
@@ -113,7 +115,7 @@ class ExecTest(Test):
else:
if env.dmesg:
old_dmesg = read_dmesg()
-(out, err, returncode) = \
+(out, err, returncode, timeout) = \
self.get_command_result(command, fullenv)
if env.dmesg:
dmesg_diff = get_dmesg_diff(old_dmesg, read_dmesg())
@@ -172,6 +174,9 @@ class ExecTest(Test):
elif returncode != 0:
results['note'] = 'Returncode was {0}'.format(returncode)
+if timeout:
+results['result'] = 'timeout'
+
if env.valgrind:
# If the underlying test failed, simply report
# 'skip' for this valgrind test.
@@ -196,6 +201,7 @@ class ExecTest(Test):
results['returncode'] = returncode
results['command'] = ' '.join(self.command)
results['dmesg'] = dmesg_diff
+results['timeout'] = timeout
self.handleErr(results, err)
@@ -217,13 +223,29 @@ class ExecTest(Test):
def get_command_result(self, command, fullenv):
try:
+timeout = False
proc = subprocess.Popen(command,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
env=fullenv,
universal_newlines=True)
-out, err = proc.communicate()
+output = ['', '']
+
+def thread_fn():
+output[0], output[1] = proc.communicate()
+
+thread = threading.Thread(target=thread_fn)
+thread.start()
+
+thread.join(self.timeout)
+
+if thread.is_alive():
+proc.terminate()
+thread.join()
+timeout = True
+
retu
[Piglit] [PATCH v2 1/2] summary.py: Treat subtests as groups
This patch causes tests with subtests to be treated as a group, rather
than as a test. This means that the status the test itself stores will
be overwritten by those in the subtest.
There is one oddity about this to be aware of; a test with subtests that
crashes or fails before any of the subtests run will report a fraction
of 0/1 with the appropriate color, even though all of the subtests will
report Not Run.
v2: - Add subtests to the results file as full tests (the internal view
of the json), without this they will not appear in changes, fixes,
etc
- Render the background color of Not Run tests correctly in HTML
- Apply subtest fractions down the stack
Signed-off-by: Dylan Baker
---
framework/summary.py | 73 ++--
1 file changed, 59 insertions(+), 14 deletions(-)
diff --git a/framework/summary.py b/framework/summary.py
index a587712..d31be19 100644
--- a/framework/summary.py
+++ b/framework/summary.py
@@ -161,8 +161,17 @@ class HTMLIndex(list):
# is a KeyError (a result doesn't contain a particular test),
# return Not Run, with clas skip for highlighting
for each in summary.results:
+# If the "group" at the top of the key heirachy contains
+# 'subtest' then it is really not a group, link to that page
try:
-self._testResult(each.name, key, each.tests[key]['result'])
+if each.tests[path.dirname(key)]['subtest']:
+href = path.dirname(key)
+except KeyError:
+href = key
+
+try:
+self._testResult(each.name, href,
+ summary.status[each.name][key])
except KeyError:
self.append({'type': 'other',
'text': 'Not Run'})
@@ -221,8 +230,14 @@ class HTMLIndex(list):
displaying pass/fail/crash/etc and formatting the cell to the
correct color.
"""
+# "Not Run" is not a valid class, if it apears set the class to skip
+if isinstance(text, so.NotRun):
+css = 'skip'
+else:
+css = text
+
self.append({'type': 'testResult',
- 'class': text,
+ 'class': css,
'href': path.join(group, href + ".html"),
'text': text})
@@ -277,19 +292,49 @@ class Summary:
fraction = self.fractions[results.name]
status = self.status[results.name]
+# store the results to be appeneded to results. Adding them in the
+# loop will cause a RuntimeError
+temp_results = {}
+
for key, value in results.tests.iteritems():
-#FIXME: Add subtest support
-
-# Walk the test name as if it was a path, at each level update
-# the tests passed over the total number of tests (fractions),
-# and update the status of the current level if the status of
-# the previous level was worse, but is not skip
-while key != '':
-fgh(key, value['result'])
-key = path.dirname(key)
-
-# when we hit the root update the 'all' group and stop
-fgh('all', value['result'])
+# Treat a test with subtests as if it is a group, assign the
+# subtests' statuses and fractions down to the test, and then
+# proceed like normal.
+try:
+for (subt, subv) in value['subtest'].iteritems():
+subt = path.join(key, subt)
+subv = so.status_lookup(subv)
+
+# Add the subtest to the fractions and status lists
+fraction[subt] = subv.fraction
+status[subt] = subv
+temp_results.update({subt: {'result': subv}})
+
+self.tests['all'].add(subt)
+while subt != '':
+fgh(subt, subv)
+subt = path.dirname(subt)
+fgh('all', subv)
+
+# remove the test from the 'all' list, this will cause to
+# be treated as a group
+self.tests['all'].discard(key)
+except KeyError:
+# Walk the test name as if it was a path, at each level
update
+# the tests passed over the total number of tests
(fractions),
+# and update the status of the current level if the status
of
+# the previous level was worse, but is not skip
+while key != '':
+fgh(key, value['result'])
+key = path
[Piglit] [PATCH v2 2/2] core.py: don't write subtests as full entries
This stops subtests from being written as full entries in the json.
Instead, in the summary code the test with subtests is treated as a
group, and the subtests as real tests. This has the added benefit of
reducing the size of the json by 80%.
Signed-off-by: Dylan Baker
---
framework/core.py | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/framework/core.py b/framework/core.py
index 2d5d0dc..e7767c2 100644
--- a/framework/core.py
+++ b/framework/core.py
@@ -483,12 +483,7 @@ class Test:
status(result['result'])
-if 'subtest' in result and len(result['subtest'].keys()) > 1:
-for test in result['subtest'].keys():
-result['result'] = result['subtest'][test]
-json_writer.write_dict_item(os.path.join(path, test),
result)
-else:
-json_writer.write_dict_item(path, result)
+json_writer.write_dict_item(path, result)
else:
status("dry-run")
--
1.8.1.5
___
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit
Re: [Piglit] [PATCH] Add a "Contributing Patches" section to the HACKING file.
On 11/13/2013 05:55 AM, Paul Berry wrote: --- I received an email this morning asking for information about how to contribute to Piglit, and I couldn't find adequate documentation in the source tree, so I figured I'd add some more information to the "HACKING" file. HACKING | 30 ++ 1 file changed, 30 insertions(+) diff --git a/HACKING b/HACKING index d96b994..9eff4ef 100644 --- a/HACKING +++ b/HACKING @@ -146,3 +146,33 @@ RELEASE and create an appropriate tag in the git repository. This tag is the official way of marking a release, so the tarballs provided automatically by the cgit frontend are official release tarballs. + +\ Contributing Patches + - + +If you want to contribute patches, please subscribe to the piglit +mailing list (https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/piglit&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=lGQMzzTgII0I7jefp2FHq7WtZ%2BTLs8wadB%2BiIj9xpBY%3D%0A&m=5liIstzzghc5x8sm5euyqBlsvvZsbAoGTFGO778q8NQ%3D%0A&s=9c4a1270e6380211bc5fc93ef94b6fe390a1c3fca0a5a1d402bbe94028e3daf4) +and then send them to piglit@lists.freedesktop.org using "git +send-email". One of the core piglit developers should respond with +comments and suggested improvements. The piglit mailing list is also +a good place for general discussion about piglit development, such as +future plans for the project, and coordinating work between +developers. + +For developers who are new to piglit: when submitting a patch, it is +helpful to add a note (after the "---" line in the patch file) +indicating that you are new to the project and don't have commit +access; that way once your patch has been revised to meet our +standards of correctness and coding style, we will know that we should +commit it for you. If we forget, please remind us! Once you have +successfully contributed a handful of patches, feel free to apply for +commit access usind the process described here: +https://urldefense.proofpoint.com/v1/url?u=http://www.freedesktop.org/wiki/AccountRequests/&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=lGQMzzTgII0I7jefp2FHq7WtZ%2BTLs8wadB%2BiIj9xpBY%3D%0A&m=5liIstzzghc5x8sm5euyqBlsvvZsbAoGTFGO778q8NQ%3D%0A&s=b555f7c32a42d12c7479c1480608272e7975df3cfc9014e7556fdaf2aba8219e + +Please be patient--most of us develop graphics drivers (such as Mesa) +as our primary job, so we have limited time to respond to your patches +on the piglit mailing list. If your patch hasn't received a reply in +a week, send a follow-up email to make sure we haven't missed it. If +you have questions that are better discussed in real time, many piglit +developers can also be found in the #dri-devel channel on Freenode +IRC. Thanks, Paul! Reviewed-by: Brian Paul ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
[Piglit] [PATCH] Add a "Contributing Patches" section to the HACKING file.
--- I received an email this morning asking for information about how to contribute to Piglit, and I couldn't find adequate documentation in the source tree, so I figured I'd add some more information to the "HACKING" file. HACKING | 30 ++ 1 file changed, 30 insertions(+) diff --git a/HACKING b/HACKING index d96b994..9eff4ef 100644 --- a/HACKING +++ b/HACKING @@ -146,3 +146,33 @@ RELEASE and create an appropriate tag in the git repository. This tag is the official way of marking a release, so the tarballs provided automatically by the cgit frontend are official release tarballs. + +\ Contributing Patches + - + +If you want to contribute patches, please subscribe to the piglit +mailing list (http://lists.freedesktop.org/mailman/listinfo/piglit) +and then send them to piglit@lists.freedesktop.org using "git +send-email". One of the core piglit developers should respond with +comments and suggested improvements. The piglit mailing list is also +a good place for general discussion about piglit development, such as +future plans for the project, and coordinating work between +developers. + +For developers who are new to piglit: when submitting a patch, it is +helpful to add a note (after the "---" line in the patch file) +indicating that you are new to the project and don't have commit +access; that way once your patch has been revised to meet our +standards of correctness and coding style, we will know that we should +commit it for you. If we forget, please remind us! Once you have +successfully contributed a handful of patches, feel free to apply for +commit access usind the process described here: +http://www.freedesktop.org/wiki/AccountRequests/ + +Please be patient--most of us develop graphics drivers (such as Mesa) +as our primary job, so we have limited time to respond to your patches +on the piglit mailing list. If your patch hasn't received a reply in +a week, send a follow-up email to make sure we haven't missed it. If +you have questions that are better discussed in real time, many piglit +developers can also be found in the #dri-devel channel on Freenode +IRC. -- 1.8.4.2 ___ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
