858278 - Create browser-chrome tests verifying that browser code can create a loader and use our APIs correctly.

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Dave Townsend [:mossop]]

As we want browser code and non-SDK add-ons to be able to use our APIs we're going to have to write tests that verify that they work. I don't think running the full test suite is necessary (though is an option!) but at least creating a loader and trying to use a few APIs would be a start. These should probably be in a more traditional Firefox test suite to ensure no contamination from our test harnesses, browser-chrome is probably a good bet.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Hmm this is similar to what I need in bug 915376, except in that bug we want to test the bootstrap.js as well.

In this bug though I think that we should run as many of the unit tests as we expect to see pass.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=3b2eb3fdcf01

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 858278-implement-sdk-tests.patch

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 8363312 [details] [diff] [review]
858278-implement-sdk-tests.patch

Review of attachment 8363312 [details] [diff] [review]:
-----------------------------------------------------------------

Glorious! A couple of comments and I'd like to see the unloading fixed before I r+ this.

::: addon-sdk/test/browser_sdk_loader_chrome.js
@@ +7,5 @@
> +
> +  let { generateUUID } = Cc['@mozilla.org/uuid-generator;1']
> +                         .getService(Ci.nsIUUIDGenerator);
> +  ok(isUUID(generateUUID()), "chrome.Cc and chrome.Ci works");
> +  

Nit: trailing whitespace

::: addon-sdk/test/browser_sdk_loader_chrome_in_sdk.js
@@ +6,5 @@
> +  // sdk/util/uuid uses Cc, Ci, components
> +  const { uuid } = require("sdk/util/uuid");
> +
> +  ok(isUUID(uuid()), "chrome.Cc and chrome.Ci works in SDK includes");
> +  

Nit: Trailing whitespace

::: addon-sdk/test/browser_sdk_loader_sdk_modules.js
@@ +9,5 @@
> +
> +  ok(has(testArray, 1), 'loader loading simple array utils');
> +  ok(has(testArray, 'hello'), 'loader loading simple array utils');
> +  ok(!has(testArray, 4), 'loader loading simple array utils');
> +  

Nit: trailing whitespace

::: addon-sdk/test/head.js
@@ +30,5 @@
> +    'sdk/': 'resource://gre/modules/commonjs/sdk/',
> +    'toolkit/': 'resource://gre/modules/commonjs/toolkit/',
> +    // Also need to specify `method/core` path
> +    // bug 961247
> +    'method/': 'resource://gre/modules/commonjs/method/'

Would it work instead to just map "./" to CURRENT_DIR and then "" to "resource://gre/modules/commonjs/" ?

@@ +42,5 @@
> +  // many SDK modules will fail
> +  // bug 961252
> +  let globalDefaults = {
> +    console: console
> +  };

Can you file a bug to have loader do this by default?

@@ +60,5 @@
> +    // We also need metadata dummy object
> +    // bug 961245
> +    metadata: {}
> +  });
> +}

You should register a cleanup function to unload any loaders when the test completes.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Dave Townsend (:Mossop) from comment #4)
> @@ +42,5 @@
> > +  // many SDK modules will fail
> > +  // bug 961252
> > +  let globalDefaults = {
> > +    console: console
> > +  };
> 
> Can you file a bug to have loader do this by default?

Uhh I see you did that already!

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8363312 [details] [diff] [review]
858278-implement-sdk-tests.patch

Greg, do we need a build config review for the trivial build config bits in here?

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 858278-implement-sdk-tests.patch

Regarding:
"Would it work instead to just map "./" to CURRENT_DIR and then "" to "resource://gre/modules/commonjs/""

The initial entry point was "main" so it resolved whatever URL to the "" path, changing to "./main" fixed this issue. Changes and nits added to new patch!

Try:
https://tbpl.mozilla.org/?tree=Try&rev=3f3d187da1b7

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 8363976 [details] [diff] [review]
858278-implement-sdk-tests.patch

Review of attachment 8363976 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/test/head.js
@@ +20,5 @@
> +Services.prefs.setBoolPref("devtools.debugger.log", true);
> +
> +registerCleanupFunction(() => {
> +  info("finish() was called, cleaning up...");
> +  unload(loader);

So this assumes that each test creates a single loader, what if they don't?

Instead you could just register a new cleanup function for every call to makeLoader to unload the created loader. Either that or just maintain an array of them.

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 858278-implement-sdk-tests.patch

Good call, Dave. Fixing that

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8365411 [details] [diff] [review]
858278-implement-sdk-tests.patch

Review of attachment 8365411 [details] [diff] [review]:
-----------------------------------------------------------------

::: addon-sdk/test/moz.build
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +BROWSER_CHROME_MANIFESTS += ['browser.ini']

Don't create moz.build files that only define test manifests. Instead, roll these entries into parent moz.build files.

e.g. addon-sdk/moz.build can contain:

BROWSER_CHROME_MANIFESTS += ['test/browser.ini']

The reason we do this is performance. Fewer files to read, fewer directories to traverse, etc.

We may one day make this build configuration an error because it is inefficient!

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 858278-implement-sdk-tests.patch

Removed the extra moz.build and rolled it into the parent!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Where is this landing?

Comment 13

[Dave Townsend [:mossop]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12)
> Where is this landing?

Anywhere really, fx-team or inbound would do

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/52b83e4b3f8b

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52b83e4b3f8b