Closed Bug 480077 Opened 11 years ago Closed 11 years ago

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

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

(Keywords: fixed1.9.1, regression, Whiteboard: [fixed1.9.1b4])

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #364029 - Flags: review?(jwalden+bmo)
Comment on attachment 364029 [details] [diff] [review]
(Av1) Don't forget to return |start|

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

Test before landing, of course.  :-)
Attachment #364029 - Flags: review?(jwalden+bmo) → review+
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)...
(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| !?
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))
}
Jeff, do you agree with my comment 6 proposal ?
Iirc, Ted said he doesn't care much, but would accept this proposal :-)
Comment 6 proposal, instead of Av1.
(Untested, but +/- trivial)
Attachment #365556 - Flags: review?(jwalden+bmo)
Attachment #365556 - Flags: review?(jwalden+bmo) → review+
Attachment #364029 - Attachment is obsolete: true
Bv1, but timing the actual application only.
Attachment #365556 - Attachment is obsolete: true
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
Attachment #365681 - Attachment description: (Bv1a) Replace external times by internal duration ++ → (Bv1a) Replace external times by internal duration ++ [Checkin: Comment 11]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Depends on: 476163
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing: after bug 476163]
Whiteboard: [needs 1.9.1 landing: after bug 476163] → [needs 1.9.1 landing: after bug 460515]
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4fb4287c24a0
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing: after bug 460515]
Attachment #365681 - Attachment description: (Bv1a) Replace external times by internal duration ++ [Checkin: Comment 11] → (Bv1a) Replace external times by internal duration ++ [Checkin: See comment 11 & 12]
Whiteboard: [fixed1.9.1b4]
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
Attachment #365681 - Attachment description: (Bv1a) Replace external times by internal duration ++ [Checkin: See comment 11 & 12] → (Bv1a) Replace external times by internal duration ++ [Checkin: See comment 11 & 12+13]
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.