Last Comment Bug 786258 - Catch exceptions thrown in endurance tests so incomplete reports can be sent
: Catch exceptions thrown in endurance tests so incomplete reports can be sent
Status: RESOLVED FIXED
[mozmill-endurance]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Dave Hunt (:davehunt)
:
:
Mentors:
Depends on:
Blocks: 772360 781238
  Show dependency treegraph
 
Reported: 2012-08-28 07:01 PDT by Dave Hunt (:davehunt)
Modified: 2012-09-11 01:39 PDT (History)
1 user (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
Catch endurance test failures. v1.0 (1006 bytes, patch)
2012-08-28 07:04 PDT, Dave Hunt (:davehunt)
hskupin: review-
Details | Diff | Splinter Review
Catch endurance test failures. v1.1 (1015 bytes, patch)
2012-08-28 10:14 PDT, Dave Hunt (:davehunt)
hskupin: review-
Details | Diff | Splinter Review
Catch endurance test failures. v2.0 (2.30 KB, patch)
2012-09-03 14:15 PDT, Dave Hunt (:davehunt)
hskupin: review+
Details | Diff | Splinter Review

Description Dave Hunt (:davehunt) 2012-08-28 07:01:53 PDT
Currently an exception in an endurance test causes the report to be lost. By surrounding this in a try/catch we should be able to submit the incomplete report for investigation into the failure.
Comment 1 Dave Hunt (:davehunt) 2012-08-28 07:04:02 PDT
Created attachment 655985 [details] [diff] [review]
Catch endurance test failures. v1.0
Comment 2 Henrik Skupin (:whimboo) 2012-08-28 09:02:33 PDT
Comment on attachment 655985 [details] [diff] [review]
Catch endurance test failures. v1.0

>+      } catch(e) {

nit: Add a blank after 'catch'.

>+        this._perfTracer.addCheckpoint("Test failure! " + e);

Don't add the whole object, but only e.message which will be enough.
Comment 3 Dave Hunt (:davehunt) 2012-08-28 10:14:18 PDT
Created attachment 656069 [details] [diff] [review]
Catch endurance test failures. v1.1
Comment 4 Henrik Skupin (:whimboo) 2012-08-29 01:29:12 PDT
Comment on attachment 656069 [details] [diff] [review]
Catch endurance test failures. v1.1

Review of attachment 656069 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/endurance.js
@@ +83,5 @@
> +      try {
> +        callback();
> +      } catch (e) {
> +        this._perfTracer.addCheckpoint("Test failure! " + e.message);
> +        throw e;

I have had an oversight yesterday. Sorry for not spotting it earlier. This will still not work due to the fact that we re-throw the exception at this point. We will add a checkpoint for sure but at the end we do not fire the enduranceResults event.

So I think a better approach here is to not add the test failure checkpoint but use a broader try/catch block so we can ensure to send the final data until the failure happened. Reason why we do not have to add the failure checkpoint is that this one is already present in the test failure report. So it's duplicated information.
Comment 5 Dave Hunt (:davehunt) 2012-09-03 14:15:34 PDT
Created attachment 657914 [details] [diff] [review]
Catch endurance test failures. v2.0

This time I'm using finally within the iteration to push the gathered checkpoints to the log, and then clear it. Outside the iteration I'm using finally to update the trigger the endurance results listener.

This change is dependant on a fix for the Mozmill Dashboard. A pull request for this can be found here: https://github.com/whimboo/mozmill-dashboard/pull/42

I also added iterations and currentIterations properties, which were useful in testing this fix.
Comment 6 Henrik Skupin (:whimboo) 2012-09-05 01:00:23 PDT
Comment on attachment 657914 [details] [diff] [review]
Catch endurance test failures. v2.0

Review of attachment 657914 [details] [diff] [review]:
-----------------------------------------------------------------

Looks way better now.
Comment 7 Dave Hunt (:davehunt) 2012-09-05 08:52:49 PDT
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/a1ec7c125a2d (default)
Comment 8 Henrik Skupin (:whimboo) 2012-09-07 06:37:23 PDT
Dave, can you please land the patch on the other branches?
Comment 10 Henrik Skupin (:whimboo) 2012-09-10 11:49:56 PDT
It has not been landed on release and esr10 yet. Dave, please check your faulty commit and redo. Thanks.
Comment 11 Dave Hunt (:davehunt) 2012-09-10 14:21:26 PDT
Looks fine to me. Please check again.
Comment 12 Henrik Skupin (:whimboo) 2012-09-10 15:46:42 PDT
Well, you finally pushed them now. Looks like that last step hasn't been done when you first did it. Thanks.
Comment 13 Dave Hunt (:davehunt) 2012-09-11 01:39:41 PDT
Hmm.. I'm not sure what happened then.

Note You need to log in before you can comment on or make changes to this bug.