1459218 - Remove DBConn() and other synchronous mozStorage calls from Places tests

Status: NEW

(Toolkit :: Places, enhancement, P3)

Description

[Marco Bonardo [:mak]]

Quite some Places tests are still using synchronous mozStorage APIs and accessors:
DBConn() from head_common.js should be removed, then the consumers:
https://searchfox.org/mozilla-central/search?q=DBConn%28%29&path=places%2Ftests&case=false&regexp=false
... should be replaced with Sqlite.jsm syntax (using await PlacesUtils.promiseDBConnection() if it's a read or PlacesUtils.withConnectionWrapper for writes)

Comment 1

[akanksha.t05]

Hello, can I please work on this? Thank you.

Comment 2

[Marco Bonardo [:mak]]

Bugs are assigned automatically when a patch is attached.

The scope of the bug is to remove DBConn() from https://searchfox.org/mozilla-central/rev/5f7ad06a2cffb7eda69229238b15198994b548e0/toolkit/components/places/tests/head_common.js#101-134 and then fix all the callers using Sqlite.sys.mjs syntax, that means one of:
Read-only SELECT query:

let db = await PlacesUtils.promiseDatabaseConnection();
let rows = await db.execute(<sql_query>, <query_params>);

Read/write query:

await PlacesUtils.withConnectionWrapper(<unique_label>, async db => {
  await db.execute(<sql_query>, <query_params>);
});

There are many examples, you can search on searchfox.org

After modifying the code, you can ./mach build faster and ./mach test toolkit/components/places to verify

Comment 3

[Abhishek[: unavailable till 11 May]]

Sir, I tried my best to understand this bug and made an example change. I will be thankful if you can review this and guide me whether I am doing correct or not - https://paste.mozilla.org/Efo68igt

Comment 4

[Marco Bonardo [:mak]]

(In reply to Abhishek from comment #3)

Sir, I tried my best to understand this bug and made an example change. I will be thankful if you can review this and guide me whether I am doing correct or not - https://paste.mozilla.org/Efo68igt

It's mostly ok, though row should be rows because execute may indeed return 0, 1 or multiple rows.
There's also no .dispose() since that's just an array, it will be garbage collected. So you don't need the try/finally.
Then of course the function was synchronous, the new one is asynchronous, so the callers must be fixed.

Comment 5

[Abhishek[: unavailable till 11 May]]

Attachment: Bug 1459218 - Remove DBConn() and other synchronous mozStorage calls from Places tests r=mak

Comment 6

[Abhishek[: unavailable till 11 May]]

I placed this under WIP because until now, I only modified one file, and I am not sure if everything is correct, especially about the dump_table function. Please have a quick review and I will update other files also

Comment 7

[Abhishek[: unavailable till 11 May]]

Sir, my end-semester exams are going to start this week. Once my exams will over, I will continue with this bug. Thanks

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.