557588 - code audit and create unit test plan for engines.js

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1: Tests for EngineMangerSvc, Engine and SyncEngine sans sync()

This is part one containing tests for everything in engine.js apart from SyncEngine.sync(), or rather the methods it calls to perform the sync. These methods rely upon various things to be set up (e.g. private/public key objects, a reasonably complete store implementation, various server methods, etc.). They are probably best tested separately, and it might make sense to come up with some generic test helpers for similar tests (e.g. for service.js).

Comment 2

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2: Tests for SyncEngine.sync(), incl some additions to harness

Now part two containing

* a few clarifying comments regarding various constants,

* a FakeCryptoService implementation (it basically doesn't encrypt ;)),

* complete server side (httpd.js) implementations for WBOs and Collections,

* exhaustive tests for SyncEngine.sync(), or rather its individual parts.

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 447196 [details] [diff] [review]
Part 1: Tests for EngineMangerSvc, Engine and SyncEngine sans sync()


>+function test_enabled() {
>+  _("Engine.enabled corresponds to preference");
>+  let engine = new SteamEngine();
>+  try {
>+    do_check_false(!!engine.enabled);
>+    Svc.Prefs.set("engine.steam", true);
>+    do_check_true(!!engine.enabled);

do we really need the !! here and elsewhere?  engine.enabled should return true or false already.

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> do we really need the !! here and elsewhere?  engine.enabled should return true
> or false already.

Actually, since the value comes from a preference setting, it will initially be null. So at least the do_check_false() check would need the !!. Using do_check_false() felt more appropriate than explicitly checking for equality to null, since engine.enabled is generally thought to be of boolean nature. You're right about the do_check_true() case, though. I'll get rid of that one.

Comment 5

[Ed Lee :Mardak]

Do we just want to make the enabled api return a true/false for sure? Do we use it anywhere assuming undefined?/null

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to comment #5)
> Do we just want to make the enabled api return a true/false for sure? Do we use
> it anywhere assuming undefined?/null

No, it seems to be used as a boolean throughout. +1 to making it true/false.

Comment 7

[Mike Connor [:mconnor]]

We should do that.  If we have somewhere distinguishing between true/false/null, I will look at blame and laugh.  Or cry.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 447809 [details] [diff] [review]
Part 2: Tests for SyncEngine.sync(), incl some additions to harness

>diff -r b08ce2c09761 source/modules/constants.js.in

>-// Record size limit is currently 10K, so 100 is a bit over 1MB
>+// Record size limit is currently 28K, so 100 records is a bit over 2.7MB

it's 256k, actually, though the old cluster capped to 28k.  Filed bug 569295 to cover being smart about total upload size.

>diff -r b08ce2c09761 source/modules/engines.js

>-    // Figure out how many total items to fetch this sync; do less on mobile
>+    // Figure out how many total items to fetch this sync; do less on mobile.
>+    // 50 is hardcoded here because of URL length restrictions.
>+    // (GUIDs are 10 chars)

GUIDs are actually up to 64 chars under the 1.0 API.  Right now we can have long things like {9a1154a9-85f6-4e49-aa9f-9c339ab12e45}33 for bookmarks.

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (updated): Tests for EngineMangerSvc, Engine and SyncEngine sans sync()

Engine.enabled now always returns a boolean.

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (updated): Tests for SyncEngine.sync(), incl some additions to harness

Fixed nits in comments.

Comment 11

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/ff0b28eef311
Tests for EngineMangerSvc, Engine and SyncEngine sans sync(). 

http://hg.mozilla.org/labs/weave/rev/965642249768
Tests for SyncEngine.sync(), incl some additions to harness.

Comment 12

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/37afbc3db445 (new url from hg magic)
Tests for EngineMangerSvc, Engine and SyncEngine sans sync(). 

http://hg.mozilla.org/labs/weave/rev/fc08fa427202 (new url from hg magic)
Tests for SyncEngine.sync(), incl some additions to harness.