On 2 April 2014 18:15, Dylan Baker <[email protected]> wrote:
> On Wednesday, April 02, 2014 16:12:23 Thomas Wood wrote:
>> Signed-off-by: Thomas Wood <[email protected]>
>> ---
>> tests/igt.py | 41 +++++++++++++++++++++++++----------------
>> 1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/tests/igt.py b/tests/igt.py
>> index f80a6c4..4640814 100644
>> --- a/tests/igt.py
>> +++ b/tests/igt.py
>> @@ -98,21 +98,30 @@ class IGTTest(ExecTest):
>>
>> def listTests(listname):
>> oldDir = os.getcwd()
>> +
>> + lines = None
>
> I have a comment below about this.
>
>> try:
>> - os.chdir(igtTestRoot)
>> - proc = subprocess.Popen(
>> - ['make', listname ],
>> - stdout=subprocess.PIPE,
>> - stderr=subprocess.PIPE,
>> - env=os.environ.copy(),
>> - universal_newlines=True
>> - )
>> - out, err = proc.communicate()
>> - returncode = proc.returncode
>> - finally:
>> - os.chdir(oldDir)
>> -
>> - lines = out.split('\n')
>> + with open(path.join(igtTestRoot, listname + '.txt')) as f:
>
> you should set a mode for this. Probably 'r'
'r' is the default, but it can be specified explicitly if you prefer.
>
>> + lines = [ line.rstrip() for line in f.readlines() ]
>
> Python convention is to never have spaces on the inside of brackets.
> Also since you only iterate over lines once, why not use a generator:
> lines = (l.rstrip() for l in f.readlines())
>
>> + except IOError:
>> + pass
>
> rather than setting lines to None and doing an if check why not make the
> block below the except IOError instead of pass, it would allow you to cut the
> if check and assigning lines = None above.
>
>> +
>> + if lines is None:
>> + try:
>> + os.chdir(igtTestRoot)
>> + proc = subprocess.Popen(
>> + ['make', 'list-' + listname ],
>
> no space inside the bracket after listname
>
>> + stdout=subprocess.PIPE,
>> + stderr=subprocess.PIPE,
>> + env=os.environ.copy(),
>> + universal_newlines=True
>> + )
>> + out, err = proc.communicate()
>> + lines = out.split('\n')
>
> suggestion: I think it's cleaner to use .splitlines() than .split('\n')
>
>> + returncode = proc.returncode
>
> with the way you're using Popen I'd suggest using subprocess.check_output, a
> convenience wrapper for Popen:
> lines = subprocess.check_output(['make', 'list-' + listname],
> env=os.environ.copy(),
> universal_newlnies=True).splitlines()
>
> I recommend this since you don't actually use err or returncode at all.
Most of the above code has not changed since before the patch (it just
moved into a new block). Should the changes you suggest be made in a
separate patch?
>
>> + finally:
>> + os.chdir(oldDir)
>> +
>> found_header = False
>> progs = ""
>>
>> @@ -126,7 +135,7 @@ def listTests(listname):
>>
>> return progs
>>
>> -singleTests = listTests("list-single-tests")
>> +singleTests = listTests("single-tests")
>>
>> for test in singleTests:
>> profile.test_list[path.join('igt', test)] = IGTTest(test)
>> @@ -150,7 +159,7 @@ def addSubTestCases(test):
>> profile.test_list[path.join('igt', test, subtest)] = \
>> IGTTest(test, ['--run-subtest', subtest])
>>
>> -multiTests = listTests("list-multi-tests")
>> +multiTests = listTests("multi-tests")
>>
>> for test in multiTests:
>> addSubTestCases(test)
_______________________________________________
Piglit mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/piglit
