Closed Bug 792546 Opened 9 years ago Closed 9 years ago

Move xpcshell test helper code to testing only JS modules


(Firefox :: Sync, defect)

Not set





(Reporter: gps, Assigned: gps)



(Whiteboard: [qa-])


(5 files)

Sync has a lot of code in head.js and other support files. As I am refactoring things, I'm finding that head.js and related are doing things for every test that I don't necessarily want. It also makes it hard to track down failures in head.js because the xpcshell test runner doesn't give nice errors when there's a failure before tests are actually executed.

Anyway, I've wanted to refactor head.js into testing-only JS modules for a while. I'm just going to throw a few patches together to do it.

First patch is pretty trivial. Please note that I'm installing tests in services/sync rather than services-sync. We decided that we want to use services/sync going forward. This is as good of a place to start as any.
Attachment #662667 - Flags: review?(rnewman)
Attachment #662667 - Flags: review?(rnewman) → review+
Assignee: nobody → gps
Attachment #662763 - Flags: review?(rnewman)
Comment on attachment 662763 [details] [diff] [review]
Part 2: Move fake services to module, v1

Review of attachment 662763 [details] [diff] [review]:

Also, ugh, that's some shoddy code. I'm sure I wrote some of it, too.
Attachment #662763 - Flags: review?(rnewman) → review+
This is actually part of services/common, but meh.
Attachment #663510 - Flags: review?(rnewman)
Attachment #663510 - Flags: review?(rnewman) → review+
I moved the low-hanging fruit.
Attachment #663524 - Flags: review?(rnewman)
I think this about does it for low-hanging fruit. Cleaning up other parts of head.js and the other always-included .js files will take a bit of refactoring, including to the xpcshell runner itself and to the extensions tests.
Attachment #663532 - Flags: review?(rnewman)
Comment on attachment 663524 [details] [diff] [review]
Part 4: Move some misc functions into a utils module, v1

Review of attachment 663524 [details] [diff] [review]:

::: services/sync/modules-testing/utils.js
@@ +10,5 @@
> +  "setBasicCredentials",
> +  "SyncTestingInfrastructure",
> +  "waitForZeroTimer",

Can we put these in a sane order? That is,



@@ +26,5 @@
> +
> +function waitForZeroTimer(callback) {
> +  // First wait >100ms (nsITimers can take up to that much time to fire, so
> +  // we can account for the timer in delayedAutoconnect) and then two event
> +  // loop ticks (to account for the Utils.nextTick() in autoConnect).

Might as well lift this to a JavaDoc.

@@ +39,5 @@
> +  }
> +  CommonUtils.namedTimer(wait, 150, {}, "timer");
> +}
> +
> +


@@ +44,5 @@
> +function setBasicCredentials(username, password, syncKey) {
> +  let ns = {};
> +  Cu.import("resource://services-sync/service.js", ns);
> +
> +  let auth = ns.Service.identity;

This should be on the list of singleton usages to remove. Should be fine for now, because Service still is, but…
Attachment #663524 - Flags: review?(rnewman) → review+
Comment on attachment 663532 [details] [diff] [review]
Part 5: Prune module imports, v1

Review of attachment 663532 [details] [diff] [review]:

Tests pass. Roll on.
Attachment #663532 - Flags: review?(rnewman) → review+
Blocks: 718066
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.