Rob Wu [:robwu] Assignee
	Description • 8 years ago

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.

	Kris Maglione [:kmag]
	Comment 3 • 8 years ago
mozreview-review

Comment on attachment 8783269 [details]
Bug 1296900 - Hide commands API if manifest key is missing

https://reviewboard.mozilla.org/r/73174/#review75204

	Kris Maglione [:kmag]
	Comment 4 • 8 years ago
mozreview-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`

	Rob Wu [:robwu] Assignee
	Comment 5 • 8 years ago
mozreview-review

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.

	Kris Maglione [:kmag]
	Comment 8 • 8 years ago
mozreview-review-reply

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.

	Rob Wu [:robwu] Assignee
	Comment 9 • 8 years ago
mozreview-review-reply

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.

	Rob Wu [:robwu] Assignee
	Comment 10 • 8 years ago
mozreview-review-reply

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.

	Kris Maglione [:kmag]
	Comment 15 • 8 years ago
mozreview-review

Comment on attachment 8783268 [details]
Bug 1296900 - Add test for availability of default WebExtension APIs

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

	Pulsebot
	Comment 16 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 17 • 8 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/f4ffc342eaac for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3837054&repo=autoland

	Pulsebot
	Comment 20 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 8 years ago

This caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3970465&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/f0185f22f35c

	Rob Wu [:robwu] Assignee
	Comment 22 • 8 years ago

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.

	Pulsebot
	Comment 25 • 8 years ago

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

	Iris Hsiao [:ihsiao]
	Comment 26 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3e89a12f219e
https://hg.mozilla.org/mozilla-central/rev/9c43276b9d7d

	Gerry Chang [:gchang]
	Comment 27 • 8 years ago

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.