680039 - Add stack traces to soft assertion failures

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Soft assertions (expects) don't currently have any stack traces when they fail.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1 (API refactor)

This patch adds stack information to the soft assertion failures. I strip away unwanted properties from the stack, e.g. languageName, which we don't really need. The stack looks a bit different to the one we get with Error().stack. That's something we should investigate later.

Comment 2

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 554424 [details] [diff] [review]
Patch v1 (API refactor)

The code's fine. 

The algorithm is confusing, though. If you don't realize frame and stack are the same structure via copy-by-reference you wonder why all this modification is going on with frame, then stack is returned. Plus you do the same thing with frame on one side and stack on the other, when it's really keeping two stacks in sync.

May help if you had an aFrame reference to aStack that you used inside the while so you're copying frame to frame instead of stack to frame. Comments could help too:

var stack = { };     // new stack to be returned
var frame = stack    // frame being built in new stack, start at base.
var aFrame = aStack; // frame being copied from old stack, start at base.

while (aFrame) {
  frame.caller = null;
  frame.filename = aFrame.filename;
  ...
  // If there's more stack to process, create the next frame

  if (aFrame.caller)
    frame.caller = { };

  // Go to the next frames in each stack
  aFrame = aFrame.caller;
  frame = frame.caller;
}

Even better, use oldFrame/newFrame/newStack instead of aFrame/frame/stack. stack vs. aStack doesn't have any meaning distinguishing them, same with frame vs. aFrame.

r=me with it made a little clearer. Should be pretty quick, and no re-review required from me unless the change is more complex than above.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2 (API refactor) [checked-in]

Thanks Geo. Yeah, sorry that I haven't really added some documentation beside the code. I now make use of better variable names. Taking over r+.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 555101 [details] [diff] [review]
Patch v2 (API refactor) [checked-in]

Landed as:
https://github.com/geoelectric/mozmill-api-refactor/commit/68ce4a248d066d7b41a3c6a76744efc8a18f3461

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1 (Old API)

Port to our current shared modules.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 555105 [details] [diff] [review]
Patch v1 (Old API)

I like the revised variable names--everything's much clearer. r+ for this patch, too.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/92a62ae4abb8 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/d21eb87854e4 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/cd784cbec430 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/87622da0810d (release)