Closed
Bug 1111284
Opened 11 years ago
Closed 11 years ago
Improve error messages of Sqlite.jsm
Categories
(Toolkit :: Async Tooling, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: Yoric, Assigned: jacob.jh.clark, Mentored)
Details
(Whiteboard: [lang=js][good first bug])
Attachments
(1 file, 7 obsolete files)
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
Assignee | ||
Comment 1•11 years ago
|
||
Suggested patch for the above issue, please could I have a review and feedback?
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8536183 -
Attachment is obsolete: true
Attachment #8536184 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8536186 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #8536187 -
Flags: review?(dteller)
Reporter | ||
Comment 5•11 years ago
|
||
(In the future, you should use `review` or `feedback` flags if you wish to request review/feedback)
Assignee | ||
Comment 6•11 years ago
|
||
No problem, thanks!
Reporter | ||
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
New patch to include functionality tests
Attachment #8536187 -
Attachment is obsolete: true
Attachment #8536218 -
Flags: review+
Attachment #8536218 -
Flags: feedback+
Reporter | ||
Updated•11 years ago
|
Attachment #8536218 -
Flags: review?(dteller)
Attachment #8536218 -
Flags: review+
Attachment #8536218 -
Flags: feedback+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8536218 -
Attachment is obsolete: true
Attachment #8536218 -
Flags: review?(dteller)
Reporter | ||
Comment 10•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8536220 -
Attachment is obsolete: true
Attachment #8536224 -
Attachment is obsolete: true
Reporter | ||
Comment 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
Looks like the Try was green :)
Reporter | ||
Comment 15•11 years ago
|
||
Let's go :)
By the way, do you have an account on Mozillians.org?
Keywords: checkin-needed
Comment 16•11 years ago
|
||
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]
Comment 17•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 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.
Description
•