551821 - Change xpcom.throwFriendlyError so that it returns an Error rather than throwing it

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

See bug 547417 comment 9.

Comment 1

[Drew Willcoxon :adw]

Attachment: patch

Has a test, which could check that each message for all the supported XPCOM errors is as expected, but that seems kind of unnecessary.

Comment 2

[Atul Varma [:atul]]

Hmm, on the one hand I'd love all our tests to have 100% code coverage and actually exercise all parts of the codebase, but on the other hand, as your last comment implies, writing tests in this context feels like a DRY violation. (DRY stands for "Don't repeat yourself", one of the mantras of Pragmatic Programming.)

I guess DRY violations are actually one of the criticisms of TDD, and I'm not sure how to respond to that. On one hand I have something of an allergic reaction to DRY violations, but on the other hand I feel like it's sometimes necessary to repeat myself to make sure that the code I wrote does what I think it does.

One practical advantage I see of writing tests to ensure that all the supported XPCOM errors are as expected is that we have the freedom to refactor this code if necessary, and be certain that it still works as expected.

I'm pinging mhanson to chime in here to see what he has to say.

Comment 3

[Drew Willcoxon :adw]

Attachment: patch 2

That's reasonable, and it's not a big deal to update the patch to check, so here's a new patch. :)

Comment 4

[Atul Varma [:atul]]

Whoa, thanks.

The only other test I can think of is making sure that the 'traceback' module can generate proper stack tracebacks from the kind of exception that friendlyError() returns. This could be something like:

  exports.testFriendlyErrorsHaveTracebacks = function(test) {
    var exc = xpcom.friendlyError(Cr.NS_ERROR_FILE_IS_DIRECTORY);
    var tb = require("traceback").fromException(exc);
    test.assert(tb.length > 1);
  }

That's not really a unit test per say, but it'd be useful to have... It might be more appropriate to put it in the traceback module, I'm not sure.

Aside from that, the only thing I'd suggest is keeping documentation for the various keys that the 'opts' argument supports, which right now I think is just 'filename'.

(We should actually document the xpcom module itself in the SDK docs, I think, but that sounds like a matter for another bug!)

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 3

>     var tb = require("traceback").fromException(exc);
>     test.assert(tb.length > 1);

This patch does exactly that, and tests pass, so I guess it works?  I added these checks to the xpcom test, since it's testing a property of the objects returned by friendlyError in particular.

> Aside from that, the only thing I'd suggest is keeping documentation for the
> various keys that the 'opts' argument supports, which right now I think is just
> 'filename'.

I used the @prop syntax suggested by Aza, but I'm not sure everyone ever agreed on it.

> (We should actually document the xpcom module itself in the SDK docs, I think,
> but that sounds like a matter for another bug!)

I'd file this bug, but I don't know how doc tasks are being tracked.

Comment 6

[Drew Willcoxon :adw]

Attachment: patch 3.1

Sorry, small change in the test.

Comment 7

[Atul Varma [:atul]]

Nice. I'm gonna r+ this, but the following suggestions are nits that you don't need to follow if you don't want:

* It's preferable to have a third string param to all the asserts that explains "in plain english" (or somewhat close to it) what's being asserted.

* I don't think the docs for opts.filename really need to list out all the NS_ERROR_* constants it matches; it could just say "the name of a file being accessed when the exception was thrown, if any"--and then we could just use our own discretion to include its information whenever it might be useful.

* Is there a particular reason Cr.NS_ERROR_FILE_IS_DIRECTORY doesn't use opts.filename if it's available? Seems like it could be useful there...

Lastly, we're not currently tracking documentation tasks any differently from coding tasks, so feel free to just file a bug in the Jetpack SDK component! Thanks!

Comment 8

[Drew Willcoxon :adw]

Attachment: patch 4

Thanks Atul.  This fixes those three nits.  I filed bug 552109 for the docs.

Comment 9

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack-sdk/rev/dc984074ff20

Comment 10

[Atul Varma [:atul]]

Awesome, thanks Drew!

Comment 11

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".