Closed Bug 858278 Opened 7 years ago Closed 6 years ago

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

Categories

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

x86_64
Windows 7
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: mossop, Assigned: jsantell)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → jsantell
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.
Depends on: 961194
Attached patch 858278-implement-sdk-tests.patch (obsolete) — Splinter Review
Attachment #8363312 - Flags: review?(dtownsend+bugmail)
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.
Attachment #8363312 - Flags: review?(dtownsend+bugmail) → review-
(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 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?
Attachment #8363312 - Flags: review?(gps)
Attached patch 858278-implement-sdk-tests.patch (obsolete) — Splinter Review
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
Attachment #8363312 - Attachment is obsolete: true
Attachment #8363312 - Flags: review?(gps)
Attachment #8363976 - Flags: review?(gps)
Attachment #8363976 - Flags: review?(dtownsend+bugmail)
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.
Attachment #8363976 - Flags: review?(dtownsend+bugmail) → review-
Attached patch 858278-implement-sdk-tests.patch (obsolete) — Splinter Review
Good call, Dave. Fixing that
Attachment #8363976 - Attachment is obsolete: true
Attachment #8363976 - Flags: review?(gps)
Attachment #8365411 - Flags: review?(gps)
Attachment #8365411 - Flags: review?(dtownsend+bugmail)
Attachment #8365411 - Flags: review?(dtownsend+bugmail) → review+
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!
Attachment #8365411 - Flags: review?(gps) → review+
Removed the extra moz.build and rolled it into the parent!
Attachment #8365411 - Attachment is obsolete: true
Attachment #8366286 - Flags: review+
Where is this landing?
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12)
> Where is this landing?

Anywhere really, fx-team or inbound would do
https://hg.mozilla.org/mozilla-central/rev/52b83e4b3f8b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.