Tests: do_throw(e); doesn't work properly

RESOLVED FIXED in mozilla23

Status

RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: BenB, Assigned: aceman)

Tracking

Trunk
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Actual:
...
} catch (e) { do_throw(e); }

That only reports the line where you throw, because the parameters are do_throw(text, stack). That's not very useful, because I don't know which line actually threw.

Fix:
Either:
1) do_throw(e, e.stack); or similar
2) fix mozilla/testing/xpcshell/head.js similar to:
function do_throw(text, stack) {
  if (!stack)
  {
    if (text.stack) // |text| is an exception object
      stack = text.stack;
    else
      stack = Components.stack.caller;
  }
Assignee: bugzilla → nobody
Keywords: helpwanted
Whiteboard: [good first bug]

Comment 1

9 years ago
Created attachment 419756 [details] [diff] [review]
Patch
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #419756 - Flags: review?(neil)

Updated

9 years ago
Attachment #419756 - Flags: review?(neil)
(Assignee)

Comment 2

6 years ago
This seems abandoned. Saint Wesonga was not responding to me also in other bug.
Assignee: wesongathedeveloper → acelists
Component: Composition → XPCShell Harness
Product: MailNews Core → Testing
Version: 1.9.1 Branch → Trunk
(Assignee)

Comment 3

6 years ago
Created attachment 746048 [details] [diff] [review]
patch v2

Well, it seems text.stack is string, but Components.stack is nsIStackFrame so we can't use them interchangeably. So I rolled a separate stack display for the string version.

Would this be acceptable? Does this same change need to be done to the do_throw_todo function ?
Attachment #419756 - Attachment is obsolete: true
Attachment #746048 - Flags: review?(ted)
Attachment #746048 - Flags: review?(neil)

Comment 4

6 years ago
Comment on attachment 746048 [details] [diff] [review]
patch v2

Does it make sense for stackLines[0] to be the "filename"?
(Assignee)

Comment 5

5 years ago
What do you mean? It seems all the stackLines[n] contain function@file:line . So I used stackLines[0] as the filename of the exception occurrence.
Comment on attachment 746048 [details] [diff] [review]
patch v2

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

This looks pretty good, but I have a few suggestions.

::: testing/xpcshell/head.js
@@ +462,5 @@
>      }
>    }, Components.interfaces.nsIThread.DISPATCH_NORMAL);
>  }
>  
>  function do_throw(text, stack) {

Can you add a comment here indicating that |text| can be either an error message or an Error object? Also you might want to just rename |text| to |error|.

@@ +464,5 @@
>  }
>  
>  function do_throw(text, stack) {
> +  if (!stack) {
> +    if (text.stack) // |text| is an exception object

This could probably be if (text instanceof Error) instead.

@@ +480,5 @@
> +      frame = frame.caller;
> +    }
> +  } else if (typeof stack == "string") {
> +    let stackLines = text.stack.split("\n");
> +    _dump("TEST-UNEXPECTED-FAIL | " + stackLines[0] + " | " + text +

I think you want to use text.fileName here, since that should correspond to where the error was thrown, but also match the other case better.

@@ +482,5 @@
> +  } else if (typeof stack == "string") {
> +    let stackLines = text.stack.split("\n");
> +    _dump("TEST-UNEXPECTED-FAIL | " + stackLines[0] + " | " + text +
> +          " - See following stack:\n");
> +    for (let i = 1; i < stackLines.length; i++) {

for (l of stackLines) {
  _dump(l + "\n");
Attachment #746048 - Flags: review?(ted) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 746555 [details] [diff] [review]
patch v3

Thanks, done.
Attachment #746048 - Attachment is obsolete: true
Attachment #746048 - Flags: review?(neil)
Attachment #746555 - Flags: review?(ted)
Attachment #746555 - Flags: review?(ted) → review+
(Assignee)

Updated

5 years ago
Keywords: helpwanted → checkin-needed
Whiteboard: [good first bug]

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6ce9acf7e78f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.