Closed Bug 1296900 Opened 6 years ago Closed 6 years ago

Add tests to verify that not too many APIs are visible by default

Categories

(WebExtensions :: Untriaged, defect)

51 Branch
defect
Not set
normal

Tracking

(firefox51 wontfix, firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: robwu, Assigned: robwu)

Details

(Whiteboard: [test] triaged)

Attachments

(2 files)

We have tests that tests whether an API exists, or whether a specific API does not exist in some contexts, but not one that checks whether APIs that shouldn't be available are really not available.

For instance, when I accidentally add the getBackgroundPage method to content scripts, all tests still pass.

Need a test to make sure that we don't unnecessarily publish APIs.
Whiteboard: [test] triaged
Comment on attachment 8783269 [details]
Bug 1296900 - Hide commands API if manifest key is missing

https://reviewboard.mozilla.org/r/73174/#review75204
Attachment #8783269 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8783268 [details]
Bug 1296900 - Add test for availability of default WebExtension APIs

https://reviewboard.mozilla.org/r/73172/#review75206

Aside from the issues below, I really don't like having this list of APIs centralized in one test file, especially when it also checks for application-specific properties that are defined in other parts of the source tree. If we're going to do this, I'd rather split it into a shared portion and an application-specific portion, and keep the latter under the `browser/` and `mobile/android/` directories.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_all_apis.html:19
(Diff revision 1)
> +"use strict";
> +
> +// Tests whether not too many APIs are visible by default.
> +
> +// Fennec's API is limited, skip some missing APIs - bug 1185785.
> +var isFennec = navigator.userAgent.includes("Android");

`AppConstants.platform == "Android"`

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_all_apis.html:42
(Diff revision 1)
> +function SKIP(path, ...skipReasons) {
> +  return {
> +    toString() { return path; },
> +    skipReasons,
> +  };
> +}

I'd rather we just add the properties that we expect in the places where we expect them than use annotations like this.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_all_apis.html:49
(Diff revision 1)
> +    toString() { return path; },
> +    skipReasons,
> +  };
> +}
> +
> +let expectedCommonApis = [

I'd rather this be hierarchical, and ideally also check the type of the properties. Something along the lines of:

    {
      extension: {
        getURL: "function",
        ...
      },
      tabs: {
        onUpdated: EVENT_HANDLER,
        ...
      },
      ...
    }

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_all_apis.html:184
(Diff revision 1)
> +  for (let path of actualApis) {
> +    if (!expectedApis.includes(path)) {
> +      unknown.push(path);
> +    }
> +  }
> +  is("", notfound.join(), `Should find all expected APIs (${description})`);

`isDeeply`
Attachment #8783268 - Flags: review?(kmaglione+bmo)
Comment on attachment 8783268 [details]
Bug 1296900 - Add test for availability of default WebExtension APIs

https://reviewboard.mozilla.org/r/73172/#review75238

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_all_apis.html:49
(Diff revision 1)
> +    toString() { return path; },
> +    skipReasons,
> +  };
> +}
> +
> +let expectedCommonApis = [

That is too verbose and unneeded. This test is not testing that the existing properties are correct (every API should be tested separately), but whether unexpected APIs appear.
Comment on attachment 8783268 [details]
Bug 1296900 - Add test for availability of default WebExtension APIs

https://reviewboard.mozilla.org/r/73172/#review75238

> That is too verbose and unneeded. This test is not testing that the existing properties are correct (every API should be tested separately), but whether unexpected APIs appear.

It's barely more verbose than the list with the full path spelled out for every property, and the structure has advantages for the traversal of the API. The type checking may not be strictly necessary, but it's a trivial addition, and it has the potential to catch important errors.
Comment on attachment 8783268 [details]
Bug 1296900 - Add test for availability of default WebExtension APIs

https://reviewboard.mozilla.org/r/73172/#review75206

The only way to be sure that there are no unexpected APIs is to explicitly list the expected APIs.
I've just split the test expectations in separate files.
Comment on attachment 8783268 [details]
Bug 1296900 - Add test for availability of default WebExtension APIs

https://reviewboard.mozilla.org/r/73172/#review75238

> It's barely more verbose than the list with the full path spelled out for every property, and the structure has advantages for the traversal of the API. The type checking may not be strictly necessary, but it's a trivial addition, and it has the potential to catch important errors.

By verbose I was referring to the type check: Any type checks should be covered by API tests already.

I spell out the full API because it is more easily findable with grep / Ctrl-F, so it is easier for a human to read.
Comment on attachment 8783268 [details]
Bug 1296900 - Add test for availability of default WebExtension APIs

https://reviewboard.mozilla.org/r/73172/#review77472
Attachment #8783268 - Flags: review?(kmaglione+bmo) → review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/06ee8b3640af
Add test for availability of default WebExtension APIs r=kmag
https://hg.mozilla.org/integration/autoland/rev/fa509c880990
Hide commands API if manifest key is missing r=kmag
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/998ed8336288
Add test for availability of default WebExtension APIs r=kmag
https://hg.mozilla.org/integration/autoland/rev/c37cf3cfd39c
Hide commands API if manifest key is missing r=kmag
Odd. The try did look green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b059319886&selectedJob=27980212

I'll try a try job without any filters.
Flags: needinfo?(rob)
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/3e89a12f219e
Add test for availability of default WebExtension APIs r=kmag
https://hg.mozilla.org/integration/autoland/rev/9c43276b9d7d
Hide commands API if manifest key is missing r=kmag
https://hg.mozilla.org/mozilla-central/rev/3e89a12f219e
https://hg.mozilla.org/mozilla-central/rev/9c43276b9d7d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.