Closed Bug 1383623 Opened 2 years ago Closed 2 years ago

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

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: markh, Assigned: Lina)

References

()

Details

Attachments

(1 file)

Our xpcshell tests still use the sync bookmarks API - we should convert them to use the async API
Priority: -- → P3
Straightforward, just tedious. :-) Should help with the async transactions work, too.
Priority: P3 → P2
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.
Assignee: nobody → kit
Status: NEW → ASSIGNED
Priority: P2 → P1
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.
Attachment #8927081 - Flags: review?(tchiovoloni) → review+
(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.
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3388bb8d7917
Remove synchronous Places API usage from the Sync unit tests. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/3388bb8d7917
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.