480077 - automation.py.in : additional fix to bug 472706 for |runApp()| return value(s)

Status: RESOLVED FIXED

(Testing :: General, defect)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Don't forget to return |start|

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 364029 [details] [diff] [review]
(Av1) Don't forget to return |start|

(untested, but seems obvious)

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 364029 [details] [diff] [review]
(Av1) Don't forget to return |start|

Test before landing, of course.  :-)

Comment 4

[Serge Gautherie (:sgautherie)]

Actually, returning |start| (only) feels inconsistent to me:
I would expect |runApp()| to return either both start and stop times, or none.

In other words, we should either time the app itself or its caller, not start with the former and end with the latter, shouldn't we :-/

My case is runtests.py.in (for bug 469518 comment 5)...

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> In other words, we should either time the app itself or its caller,

In the former case, we could replace |start| by |duration| !?

Comment 6

[Serge Gautherie (:sgautherie)]

http://mxr.mozilla.org/mozilla-central/search?string=runApp%5C%28&regexp=on&case=on&find=%5C.py
{
Found 6 matching lines in 5 files

/build/automation.py.in

/build/leaktest.py.in
/build/pgo/profileserver.py.in
/layout/tools/reftest/runreftest.py
/testing/mochitest/runtests.py.in
}
out of which only runtests.py.in actually uses |start| yet.

So, my proposal is to merge
{
488   # print test run times
489   finish = datetime.now()
490   log.info(" started: %s", str(start))
491   log.info("finished: %s", str(finish))
}
into automation.runApp(), remove the |start| return value and print something like
{
log.info("application run for: %s", str(datetime.now() - start))
}

Comment 7

[Serge Gautherie (:sgautherie)]

Jeff, do you agree with my comment 6 proposal ?

Comment 8

[Serge Gautherie (:sgautherie)]

Iirc, Ted said he doesn't care much, but would accept this proposal :-)

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Replace external |stop| by internal |logDuration()| feature ++

Comment 6 proposal, instead of Av1.
(Untested, but +/- trivial)

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) Replace external times by internal duration ++ [Checkin: See comment 11 & 12+13]

Bv1, but timing the actual application only.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 365681 [details] [diff] [review]
(Bv1a) Replace external times by internal duration ++
[Checkin: See comment 11 & 12+13]


http://hg.mozilla.org/mozilla-central/rev/3622aadb857b

Comment 12

[(not currently active) Ted Mielczarek]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4fb4287c24a0

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 365681 [details] [diff] [review]
(Bv1a) Replace external times by internal duration ++
[Checkin: See comment 11 & 12+13]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1805229f044e
(Cv1-191) Resync' automation.py.in with m-c, missed part