code audit and create unit test plan for service.js

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 months ago

People

(Reporter: mconnor, Assigned: philikon)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

Reporter

Description

9 years ago
No description provided.
Flags: blocking-weave1.3+
Reporter

Updated

9 years ago
Target Milestone: --- → 1.3
Reporter

Updated

9 years ago
Whiteboard: [final]

Updated

9 years ago
Whiteboard: [final]

Comment 1

9 years ago
Interesting to note from the code coverage tool:
http://philikon.de/files/synccoverage/_Users_philipp_dev_mozilla_weave_weave_dist_stage_modules_service.js.html

_registerEngines doesn't actually register any engines because the test harness identifies as  xuth@mozilla.org XULRunner Unit Test Harness

This results in the tool (correctly) reporting a lot of engines/stores/tracker as being not covered. We could just add in an extra case "xuth@mozilla.org": for the firefox id case to get a bunch of coverage.
I'll take this bug.

The URL in comment 1 no longer works after having made some changes to the coverage reporter. Here's an updated link: http://philikon.de/files/weavesync-coverage/service.js.html#l380 though I won't promise to host these files forever ;). I just filed Bug 569228 with a patch containing the coverage tool so anybody should be able to create a coverage report now.
Assignee: mconnor → philipp
Weave.Service.onStartup has some of Firefox-only code for autoconnect after startup. It feels out of place there. To me it would make more sense to move this to an observer for the "weave:service:ready" notification that is sent off just before the piece of code in question. The observer would only be registered when on Firefox. That way more/other behaviour could also be added forother apps (Fennec, SeaMonkey, ...) without hacking it into onStartup.

Thoughts?
Reporter

Comment 4

9 years ago
It's not quite Firefox-only, but it's likely true that we could require apps to control connection timing via whatever means we declare.  Need to think about this some more.
Err right, just the delay calculation is Firefox-only.

Thinking about this some more, it really doesn't quite feel right to trigger auto-connect immediately here. After all, Weave.Service.onStartup is executed on *import* of service.js. So right now there's no way to import it without having auto-connect triggered (unless you modify the preference setting, of course).
Get rid of unused '_syncInProgress' attribute, unnecessary getters and setters for 'keyGenEnabled'.
Attachment #449279 - Flags: review?(mconnor)
Tests for module startup as well as various dynamic attributes of Weave.Service.
Attachment #449280 - Flags: review?(mconnor)
Reporter

Updated

9 years ago
Attachment #449279 - Flags: review?(mconnor) → review+
Reporter

Updated

9 years ago
Attachment #449280 - Flags: review?(mconnor) → review+
Reporter

Updated

9 years ago
Target Milestone: 1.3 → 2.0
Blocks: 569740

Comment 8

9 years ago
http://hg.mozilla.org/services/fx-sync/rev/b779e8187b3f
Part 1: Get rid of unused '_syncInProgress' attribute, unnecessary getters and setters. 

http://hg.mozilla.org/services/fx-sync/rev/1b3ffbc890a2
Part 2: Tests for module startup, Weave.Service attributes.
Reporter

Updated

9 years ago
Flags: blocking-weave1.3+
Some more tests, many more to come. :)
Attachment #450762 - Flags: review?(mconnor)
Reporter

Updated

9 years ago
Attachment #450762 - Flags: review?(mconnor) → review+
Updated part 3 after bug 570636 was landed.
Attachment #450762 - Attachment is obsolete: true
Part 4: Get rid of superfluous attribute, introduce constants for password/passphrase realms, add/improve tests for login(), logout(), persistLogin().
Attachment #451825 - Flags: review?(mconnor)
Part 5: Some more tests, this time for checkUsername, createAccount, changePassword. Many more still to go...
Attachment #451826 - Flags: review?(mconnor)
Clean up some commented out code that I left in by accident.
Attachment #451826 - Attachment is obsolete: true
Attachment #451968 - Flags: review?(mconnor)
Attachment #451826 - Flags: review?(mconnor)
Reporter

Updated

9 years ago
Attachment #451825 - Flags: review?(mconnor) → review+
Reporter

Updated

9 years ago
Attachment #451968 - Flags: review?(mconnor) → review+

Comment 14

9 years ago
http://hg.mozilla.org/services/fx-sync/rev/11cd5c679054
Part 3: Tests for Weave.Service._{find|set|update}Cluster() 

http://hg.mozilla.org/services/fx-sync/rev/b2cab5c4a61f
Part 4: Get rid of superfluous attribute, introduce constants for password/passphrase realms, add/improve tests for login(), logout(), persistLogin(). 

http://hg.mozilla.org/services/fx-sync/rev/c589f9b8c65a
Part 5: Tests for checkUsername, createAccount, changePassword
Part 4 followup: Fix a reference error in Weave.Service._checkServerError and improve test coverage for verifyLogin() to exercise that code path.

r=mconnor via IRC

1.4.x: http://hg.mozilla.org/services/fx-sync/rev/91f7fe8eecc2
default: http://hg.mozilla.org/services/fx-sync/rev/f838ee5b039b
Reporter

Updated

8 years ago
Target Milestone: 2.0 → ---
Reporter

Comment 16

8 years ago
So I'm going to call this done, for now.  The impending async refactor will need to fill in any tests not written at that point.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.