1383623 - Remove use of sync bookmarks API from sync's xpcshell tests.

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Our xpcshell tests still use the sync bookmarks API - we should convert them to use the async API

Comment 1

[Lina Butler [:lina]]

Straightforward, just tedious. :-) Should help with the async transactions work, too.

Comment 2

[Mark Banner (:standard8)]

Just to note, when I was looking at the sync APIs the other day I noticed that test_bookmark_tracker.js is set up to test the sync and the async APIs.

So that test can probably just be cleaned up when we remove the actual APIs. It is the other ones that will need changing.

Comment 3

[Lina Butler [:lina]]

Attachment: Bug 1383623 - Remove synchronous Places API usage from the Sync unit tests.

Review commit: https://reviewboard.mozilla.org/r/198294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198294/

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8927081 [details]
Bug 1383623 - Remove synchronous Places API usage from the Sync unit tests.

https://reviewboard.mozilla.org/r/198294/#review203804

Looks good! I mean, the tests are still awkward, but it's progress!

Just to check, do we have existing in-tree uses of the synchronous API? If so, is it worth having a couple tests that ensure that we pick up changes made with that API, or no? (I don't really know how likely it is that we'd break only in that case, since you wrote the tracker you probably have a much better idea)

::: services/sync/tests/unit/test_bookmark_engine.js:196
(Diff revision 1)
>      ok(!!error);
>  
>      // Verify that the bookmark order has been applied.
> -    folder1_record = await store.createRecord(folder1_guid);
> +    folder1_record = await store.createRecord(folder1.guid);
>      let new_children = folder1_record.children;
> -    do_check_eq(new_children.length, 2);
> +    do_check_matches(new_children,

TIL do_check_matches. And then I learned that while it used to be something a bit different, now it seems to be just a synonym for deepEquals. Weird.

Comment 5

[Lina Butler [:lina]]

(In reply to Thom Chiovoloni [:tcsc] from comment #4)
> Just to check, do we have existing in-tree uses of the synchronous API? If
> so, is it worth having a couple tests that ensure that we pick up changes
> made with that API, or no?

I kept the synchronous APIs in the tracker tests, like Standard8 suggested in comment 2. (I think `test_sync_fields` and `test_sync_utils` in Places exercise both APIs, too). We can clean them up when we're ready to remove the APIs.

> TIL do_check_matches. And then I learned that while it used to be something
> a bit different, now it seems to be just a synonym for deepEquals. Weird.

Same, I didn't realize they were aliases until a few weeks ago. I'd like to switch us over to using `deepEquals` and the other `Assert.*` methods instead of `do_check_*` eventually, but I'm not sure it's worth the churn, and definitely not part of this bug.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3388bb8d7917
Remove synchronous Places API usage from the Sync unit tests. r=tcsc

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/3388bb8d7917