Closed
Bug 782598
Opened 12 years ago
Closed 12 years ago
jstests.py runs forever when tests exceed their timeout
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(2 files)
1.21 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
4.11 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Since updating recently I've been seeing jstests running forever. Looking at what was running it seems that there are a few tests that run for a very long time and which are not being timed out.
Assignee | ||
Comment 1•12 years ago
|
||
I think what's happening is that the function total_seconds() is returning zero which get_max_wait() interprets as meaning wait for ever. The fix for bug 781445 replaced the call to timedelta.total_seconds() with the function total_seconds() that the python documentation describes as "equivalent ... with true division enabled". True division turns out to be a python feature where integer division can return a float rather than rounding floating point results down to the nearest int, and it is not enabled here. The patch just forces this function to return a float, as presumably timedelta.total_seconds() does. Apparently true division can be enabled with "from __future__ import division" but to me it's less clear what that is doing.
Assignee: general → jcoppeard
Attachment #651727 -
Flags: review?(terrence)
Assignee | ||
Comment 2•12 years ago
|
||
While looking at this I noticed that the "remaining > wait" condition in get_max_wait() seems to be the wrong way round, unless I've misunderstood what's going on here. Also, I can't see why this function would ever want to return None / wait forever - there are always tasks running when it is called. Here's a patch and tidyup for this.
Attachment #651793 -
Flags: review?(terrence)
Comment 3•12 years ago
|
||
Comment on attachment 651727 [details] [diff] [review] Proposed fix Review of attachment 651727 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! I should have read the docs more carefully. ::: js/src/tests/lib/tasks_unix.py @@ +45,5 @@ > def total_seconds(td): > + """ > + Return the total number of seconds contained in the duration as a float > + """ > + return (1.0 * td.microseconds + (td.seconds + td.days * 24 * 3600) * 10**6) / 10**6 I think float(td.microseconds) would make the intention clearer.
Attachment #651727 -
Flags: review?(terrence) → review+
Comment 4•12 years ago
|
||
Comment on attachment 651793 [details] [diff] [review] Tidyup for get_max_wait() Review of attachment 651793 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jon Coppeard (:jonco) from comment #2) > While looking at this I noticed that the "remaining > wait" condition in > get_max_wait() seems to be the wrong way round, unless I've misunderstood > what's going on here. My version starts at 0 and pushes wait up to the maximum required. Your version starts at the maximum possible wait and then pulls it down to what is needed. I think they are equivalent. > Also, I can't see why this function would ever want > to return None / wait forever - there are always tasks running when it is > called. Like the comment says, if you pass |-t 0| to the harness, that means to disable timing out the test. As you noticed, the code doesn't implement that behavior :-). I think it should be |if timeout == 0: wait = None|. > Here's a patch and tidyup for this. It's a nice cleanup, let's get it committed!
Attachment #651793 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/92f022b4ae92 https://hg.mozilla.org/integration/mozilla-inbound/rev/726c3013ab9f
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #4) Cheers for the review, comments addressed and pushed to inbound.
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/726c3013ab9f https://hg.mozilla.org/mozilla-central/rev/92f022b4ae92
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•