617706 - Test harness improvements (remoting tests); modifications to testconfig for AOT

Status: VERIFIED FIXED

(Tamarin Graveyard :: Tools, defect, P4)

Description

[David Saracino]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5
Build Identifier: 

Added better support for remoting tests.  Also modified the testconfig file for AOT tests.

Reproducible: Always

Comment 1

[David Saracino]

Attachment: Patch containing proposed changes

Comment 2

[Chris Peyer]

Comment on attachment 496245 [details] [diff] [review]
Patch containing proposed changes

Overall looks good but there are some issues:

- runtests and all support files now need to be python3 compatible.  Please test against python3 (we've also noticed a significant speedup with 3.2).
- You switched from importing the time module to importing the specific time methods, this breaks a normal (non-aot) run due to time.tzname not being recognized (the tzname var will also have to be renamed to tz_name).  Please test against the regular shell too.
- In general, we would like to start following PEP8 standards, so new code should try to follow that.  Really the main thing is to keep lines < 100 chars

- Threads should not be an option for the performance/runtests as having that option available may confuse users to use threads during a performance run.  This will interfere with performance results which must be run one test at a time.  If your using threads to send the tests to multiple devices, lets use some sort of other command line argument to indicate that's what you are doing.

nits:
- no need to explicitly check for None, instead of:
if self.remoteip != None:
use
if self.remoteip:

- what is the purpose of safe_mkdir?  I don't see why os.mkdir can't be used directly.

Thanks for the testTimeout code, the parameter was put in a long while back but the functionality at some point got lost.

Comment 3

[Chris Peyer]

I get the following error when running with testtimeout set:

./runtests.py --testtimeout 1 regress/
Tamarin tests started: 2010-12-09 10:02:27.200276
Traceback (most recent call last):
  File "./runtests.py", line 684, in <module>
    runtest = AcceptanceRuntest()
  File "./runtests.py", line 71, in __init__
    RuntestBase.__init__(self)
  File "../util/runtestBase.py", line 192, in __init__
    self.run()
  File "./runtests.py", line 147, in run
    self.determineConfig()
  File "../util/runtestBase.py", line 429, in determineConfig
    (f,err,exitcode) = self.run_pipe('%s -Dversion' % self.avm)
  File "../util/runtestBase.py", line 1258, in run_pipe
    if output:
UnboundLocalError: local variable 'output' referenced before assignment

Comment 4

[Chris Peyer]

+            elif o in ('--aotsdk',):
+                self.aotsdk = v
+            elif o in ('--aotout',):
+                self.aotout = v
+            elif o in ('--aotargs',):
+                self.aotextraargs = v

This (and associated parsing code) could be moved to runtestBase since it is now shared in both acceptance and performance runtests.

Comment 5

[Chris Peyer]

another nit - no need for parens around if clauses:

if( not returncode == 0 ):

just use:

if not returncode == 0:

Comment 6

[David Saracino]

python3 compatibility:
  verified with the -3 option

import time method:
  reverted back to old way

Lines less than 100 characters:
  New code all complies

safe_mkdir:
  os.mkdir throws if the directory already exists.  Since we're building on multiple threads now, there's a race condition between when check if the directory exists and then call mkdir.  safe_mkdir doesn't throw.
  Renamed it mkdir_nothrow so it's clearer

nits:
  fixed!

Performance threads:
  AFIK this script only builds the tests - it doesn't run them.  avm-metrics.py runs them.  Therefore we should be safe with the threads argument.

--aotsdk arg (and others):
  promoted as requested

output error:
  fixed

Comment 7

[David Saracino]

python3 compatibility:
  verified with the -3 option

import time method:
  reverted back to old way

Lines less than 100 characters:
  New code all complies

safe_mkdir:
  os.mkdir throws if the directory already exists.  Since we're building on multiple threads now, there's a race condition between when check if the directory exists and then call mkdir.  safe_mkdir doesn't throw.
  Renamed it mkdir_nothrow so it's clearer

nits:
  fixed!

Performance threads:
  AFIK this script only builds the tests - it doesn't run them.  avm-metrics.py runs them.  Therefore we should be safe with the threads argument.

--aotsdk arg (and others):
  promoted as requested

output error:
  fixed

Comment 8

[David Saracino]

Attachment: Changes relevant to comment 6

Comment 9

[David Saracino]

Attachment: Missed one more change. These are relevant to comment 6

Comment 10

[Chris Peyer]

Comment on attachment 497557 [details] [diff] [review]
Missed one more change.  These are relevant to comment 6

r+ with some nits:

- For python3, print is a method and parens must be used: print(...)
- In runtestBase the aotsdk helpstring is missing a space: ... tostandalone executables.
- change: if( self.iterations < 1): to if self.iterations < 1:
- string.replace is a deprecated python method.  Either use str.replace or directly call the replace method on the string
- Place spaces around the = in var assignments: err = 1

Comment 11

[David Saracino]

Attachment: Take 4: contains recent round of code review

Contains most recent round of code reviews (comment 10)

Comment 12

[David Saracino]

Attachment: One more fix: chomp avm_args

Only difference between this and last: we chmop the avm_args.  Newline was breaking tests that took args.

Comment 13

[Trevor Baker]

Chris, please review and push if possible.

Comment 14

[Chris Peyer]

Patch stale, and not needed anymore according to David Saracino.