Improve error messages of Sqlite.jsm

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: jacob.jh.clark, Mentored)

Tracking

unspecified
mozilla37
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(1 attachment, 7 obsolete attachments)

In Sqlite.jsm, whenever the underlying library mozStorage returns an error, we hide the error message, which loses information for developers.

Fixing this would be quite simple. The error message is hardcoded as "Error(s) encountered during statement execution" in Sqlite.jsm, but local variable `errors` is an array of objects with a field `message` that contains additional information.

Therefore, it should be sufficient to replace the hardcoded message with
"Error(s) encountered during statement execution: " + [error.message for (error of errors)].join(", ")".

The code lives here: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm?from=Sqlite.jsm#685
Suggested patch for the above issue, please could I have a review and feedback?
Attachment #8536183 - Attachment is obsolete: true
Attachment #8536184 - Attachment is obsolete: true
Attachment #8536187 - Flags: review?(dteller)
(In the future, you should use `review` or `feedback` flags if you wish to request review/feedback)
No problem, thanks!
Comment on attachment 8536187 [details] [diff] [review]
Modifies error message within Sqllite.jsm to provide further details of error

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

This looks good. Let's take the opportunity to test the behavior.

You can find the relevant test in file test_sqlite.js, function `test_readOnly_clone` http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_sqlite.js#928-943

Let's replace the entire `try/catch` block with

Assert.rejects(
  () => clone.execute(...),
  /read-only/
);

This will check that `clone.execute(...)` throws an asynchronous error that contains the words "read-only", which should indeed appear in the string you have just modified.

To rebuild and test that it works, run
./mach build toolkit/modules
./mach xpcshell-test toolkit/modules/tests/xpcshell/test_sqlite.js
Attachment #8536187 - Flags: review?(dteller) → feedback+
New patch to include functionality tests
Attachment #8536187 - Attachment is obsolete: true
Attachment #8536218 - Flags: review+
Attachment #8536218 - Flags: feedback+
Attachment #8536218 - Flags: review?(dteller)
Attachment #8536218 - Flags: review+
Attachment #8536218 - Flags: feedback+
Attachment #8536218 - Attachment is obsolete: true
Attachment #8536218 - Flags: review?(dteller)
Comment on attachment 8536220 [details] [diff] [review]
Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

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

Almost there :)

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ -931,5 @@
>  
>    let clone = yield c.clone(true);
>    // Just check that it works.
>    yield clone.execute("SELECT 1");
> -  // But should not be able to write.

You removed the comment here, can you put it back?

@@ +934,5 @@
>    yield clone.execute("SELECT 1");
> +
> +  yield Assert.rejects(clone.execute("CREATE TABLE test (id INTEGER PRIMARY KEY)"),
> +                       /readonly/);
> +  

Nit: can you remove the whitespace on this line?
Attachment #8536220 - Flags: feedback+
Comment on attachment 8536225 [details] [diff] [review]
Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

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

This looks good.
Let's Try it: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59bc30e6eba0
Attachment #8536225 - Flags: review+
Looks like the Try was green :)
Let's go :)

By the way, do you have an account on Mozillians.org?
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/97af67305dab
Assignee: nobody → jacob.jh.clark
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/97af67305dab
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.