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

VERIFIED FIXED in Q3 11 - Serrano

Status

P4
normal
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: dsaracin, Assigned: cpeyer)

Tracking

unspecified
Q3 11 - Serrano
Bug Flags:
flashplayer-qrb +
flashplayer-bug -
flashplayer-triage +

Details

Attachments

(5 attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

8 years ago
Created attachment 496245 [details] [diff] [review]
Patch containing proposed changes

Updated

8 years ago
Attachment #496245 - Flags: review?(cpeyer)
(Assignee)

Comment 2

8 years ago
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.
Attachment #496245 - Flags: review?(cpeyer) → review-
(Assignee)

Comment 3

8 years ago
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
(Assignee)

Comment 4

8 years ago
+            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.
(Assignee)

Comment 5

8 years ago
another nit - no need for parens around if clauses:

if( not returncode == 0 ):

just use:

if not returncode == 0:
(Reporter)

Comment 6

8 years ago
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
(Reporter)

Comment 7

8 years ago
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
(Reporter)

Comment 8

8 years ago
Created attachment 497555 [details] [diff] [review]
Changes relevant to comment 6
(Reporter)

Comment 9

8 years ago
Created attachment 497557 [details] [diff] [review]
Missed one more change.  These are relevant to comment 6
(Assignee)

Comment 10

8 years ago
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
(Reporter)

Comment 11

8 years ago
Created attachment 497677 [details] [diff] [review]
Take 4: contains recent round of code review

Contains most recent round of code reviews (comment 10)
(Assignee)

Updated

8 years ago
Attachment #497677 - Flags: review?(cpeyer)
(Assignee)

Updated

8 years ago
Attachment #497677 - Flags: review?(cpeyer) → review+
(Reporter)

Comment 12

8 years ago
Created attachment 497819 [details] [diff] [review]
One more fix: chomp avm_args

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

Updated

8 years ago
Attachment #497819 - Flags: review?(cpeyer)

Updated

8 years ago
Assignee: nobody → cpeyer
Flags: flashplayer-triage+
Flags: flashplayer-qrb+
Flags: flashplayer-bug-
Priority: -- → P4
Target Milestone: --- → flash10.x-Serrano
(Assignee)

Updated

8 years ago
Attachment #497819 - Flags: review?(cpeyer) → review+

Comment 13

8 years ago
Chris, please review and push if possible.
(Assignee)

Comment 14

8 years ago
Patch stale, and not needed anymore according to David Saracino.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.