Add browser_test for extension app API with missing schema

Add browser_test for extension app API with missing schema

This is a simplified version of api_test/stubs. It touches all the parts
of each chrome.foo.bar.baz API path and ensures they are defined. This
catches cases where a developer adding an API to _api_features.json but
did not provide a schema for them.

BUG=369318
TEST=added browser_tests ExtensionApiTest.StubsApp
R=kalman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268949

[James Cook, 2014-05-02 22:00:16]

kalman, PTAL

I'm not very experienced with JS, so please let me know where I screwed up the
style.

[not at google - send to devlin, 2014-05-02 22:19:57]

fun reviewing old code :) and... sheesh.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/stubs_app/background.js (right):

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:11: return
typeof(feature.extension_types) == "undefined" ||
this first check isn't right, since if it's undefined you need to step down into
the dependencies, and then you're in a world you don't want to be. most APIs'
availability is defined by a dependency on a permission (often a manifest key).

so this test has rotted a bit. I'm surprised this even passes, since this line
means that basically every API will get added anyway, and why does that not
cause line 72 to fail?

in other words you may as well remove this isAvailabileToPlatformApps check
entirely?

more robust would be to expose a function which returns all APIs that an
extension has access to, from the test API. all of the dependency logic is
implemented. I think that would be a good way to do it?

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:12:
feature.extension_types.indexOf('all') != -1 ||
however, this test could be much stronger if the sample app actually requested
permissions. without requesting permissions it basically should have access to
chrome.app.runtime and chrome.app.window. that's not very interesting.

you should be able to get a decent approximation by looping through all
permissions and adding those to the "permissions" field of the manifest.

or, as a shortcut for now (... and likely all time), just copying the app REPL
permissions/manifest.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:43: for (var
propName in module.properties) {
Object.getOwnPropertyNames(module.properties).forEach

is safer, doesn't step up proto chain.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:67: // Not the last
component. Allowed to be undefined because some paths are
this check confuses me.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:86: var apiPaths =
getApiPaths();
I would just inline this

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:98: if (success) {
use "failures.length == 0" here and you don't need |success|

[James Cook, 2014-05-02 23:01:39]

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/stubs_app/background.js (right):

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:67: // Not the last
component. Allowed to be undefined because some paths are
On 2014/05/02 22:19:57, kalman wrote:
> this check confuses me.

A quick question while I'm working on the rest of it...

This check confuses me as well. I copied it over from the other api_tests/stubs
test. It seems to be needed when testing the path of APIs like:

app.currentWindowInternal.focus

When iterating over the path it finds "currentWindowInternal" is undefined.  Is
that because I'm screwing up how I get the list of possible paths?  Do I need to
filter out internal APIs?

[not at google - send to devlin, 2014-05-02 23:03:17]

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/stubs_app/background.js (right):

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:67: // Not the last
component. Allowed to be undefined because some paths are
On 2014/05/02 23:01:39, James Cook wrote:
> On 2014/05/02 22:19:57, kalman wrote:
> > this check confuses me.
> 
> A quick question while I'm working on the rest of it...
> 
> This check confuses me as well. I copied it over from the other
api_tests/stubs
> test. It seems to be needed when testing the path of APIs like:
> 
> app.currentWindowInternal.focus
> 
> When iterating over the path it finds "currentWindowInternal" is undefined. 
Is
> that because I'm screwing up how I get the list of possible paths?  Do I need
to
> filter out internal APIs?

ah yeah you need to filter out internal APIs.

[James Cook, 2014-05-02 23:59:14]

kalman, please take another look.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/stubs_app/background.js (right):

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:11: return
typeof(feature.extension_types) == "undefined" ||
On 2014/05/02 22:19:57, kalman wrote:
> this first check isn't right, since if it's undefined you need to step down
into
> the dependencies, and then you're in a world you don't want to be. most APIs'
> availability is defined by a dependency on a permission (often a manifest
key).
> 
> so this test has rotted a bit. I'm surprised this even passes, since this line
> means that basically every API will get added anyway, and why does that not
> cause line 72 to fail?
> 
> in other words you may as well remove this isAvailabileToPlatformApps check
> entirely?
> 
> more robust would be to expose a function which returns all APIs that an
> extension has access to, from the test API. all of the dependency logic is
> implemented. I think that would be a good way to do it?

I took out this isAvailableToPlatformApps check and just iterate over all APIs.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:12:
feature.extension_types.indexOf('all') != -1 ||
On 2014/05/02 22:19:57, kalman wrote:
> however, this test could be much stronger if the sample app actually requested
> permissions. without requesting permissions it basically should have access to
> chrome.app.runtime and chrome.app.window. that's not very interesting.
> 
> you should be able to get a decent approximation by looping through all
> permissions and adding those to the "permissions" field of the manifest.
> 
> or, as a shortcut for now (... and likely all time), just copying the app REPL
> permissions/manifest.

Added permissions from app REPL and a pass through _permission_features.json
that picked up a few more. This test now checks a lot more APIs. :-)

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:43: for (var
propName in module.properties) {
On 2014/05/02 22:19:57, kalman wrote:
> Object.getOwnPropertyNames(module.properties).forEach
> 
> is safer, doesn't step up proto chain.

Done.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:67: // Not the last
component. Allowed to be undefined because some paths are
Filtering out internal APIs gets rid of the need for this insanity. Done.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:86: var apiPaths =
getApiPaths();
On 2014/05/02 22:19:57, kalman wrote:
> I would just inline this

Done.

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:98: if (success) {
On 2014/05/02 22:19:57, kalman wrote:
> use "failures.length == 0" here and you don't need |success|

Done.

[not at google - send to devlin, 2014-05-03 00:12:43]

lgtm

does this mean that in theory we need to go and fix the old Stubs test too?

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/stubs_app/background.js (right):

https://codereview.chromium.org/264923009/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/stubs_app/background.js:12:
feature.extension_types.indexOf('all') != -1 ||
On 2014/05/02 23:59:15, James Cook wrote:
> On 2014/05/02 22:19:57, kalman wrote:
> > however, this test could be much stronger if the sample app actually
requested
> > permissions. without requesting permissions it basically should have access
to
> > chrome.app.runtime and chrome.app.window. that's not very interesting.
> > 
> > you should be able to get a decent approximation by looping through all
> > permissions and adding those to the "permissions" field of the manifest.
> > 
> > or, as a shortcut for now (... and likely all time), just copying the app
REPL
> > permissions/manifest.
> 
> Added permissions from app REPL and a pass through _permission_features.json
> that picked up a few more. This test now checks a lot more APIs. :-)

Awesome :)

though won't pick up on people accidentally not adding schemas for new APIs.
hopefully they will test for it. it's the removing of APIs that has broken us
historically (... those 2 times it has happened).

https://codereview.chromium.org/264923009/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/stubs_app/background.js (right):

https://codereview.chromium.org/264923009/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/stubs_app/background.js:19: ["functions",
"events"].forEach(function(section) {
it would be cleaner to loop over [module.functions, module.events] here.

https://codereview.chromium.org/264923009/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/stubs_app/manifest.json (right):

https://codereview.chromium.org/264923009/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/stubs_app/manifest.json:42: "videoCapture"
whee

could you also add empty objects for "bluetooth", "sockets", and "commands" to
the manifest?

[James Cook, 2014-05-05 16:36:26]

https://codereview.chromium.org/264923009/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/stubs_app/background.js (right):

https://codereview.chromium.org/264923009/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/stubs_app/background.js:19: ["functions",
"events"].forEach(function(section) {
On 2014/05/03 00:12:43, kalman wrote:
> it would be cleaner to loop over [module.functions, module.events] here.

Done.

[James Cook, 2014-05-05 16:36:29]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-05-05 16:36:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/264923009/60001

[commit-bot: I haz the power, 2014-05-06 07:13:26]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 09:19:47]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 11:16:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-06 11:16:02]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[James Cook, 2014-05-06 16:23:43]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-05-06 16:23:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/264923009/80001

[commit-bot: I haz the power, 2014-05-07 02:44:41]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 08:47:06]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-07 11:14:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-07 11:14:09]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[James Cook, 2014-05-07 13:59:10]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-05-07 13:59:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jamescook@chromium.org/264923009/80001

[James Cook, 2014-05-07 21:17:41]

Committed patchset #5 manually as r268949 (presubmit successful).

[James Cook, 2014-05-09 03:30:37]

On 2014/05/07 21:17:41, James Cook wrote:
> Committed patchset #5 manually as r268949 (presubmit successful).

Patch reverted because it leaks a file "about" (from "about:blank") in the
working directory, which causes 70 other browser_tests failures.

[not at google - send to devlin, 2014-05-09 04:32:24]

On 2014/05/09 03:30:37, James Cook wrote:
> On 2014/05/07 21:17:41, James Cook wrote:
> > Committed patchset #5 manually as r268949 (presubmit successful).
> 
> Patch reverted because it leaks a file "about" (from "about:blank") in the
> working directory, which causes 70 other browser_tests failures.

How does it manage that?

[James Cook, 2014-05-09 04:46:56]

On 2014/05/09 04:32:24, kalman wrote:
> On 2014/05/09 03:30:37, James Cook wrote:
> > On 2014/05/07 21:17:41, James Cook wrote:
> > > Committed patchset #5 manually as r268949 (presubmit successful).
> > 
> > Patch reverted because it leaks a file "about" (from "about:blank") in the
> > working directory, which causes 70 other browser_tests failures.
> 
> How does it manage that?

See crbug.com/371030 for details. I caused a 15 hour outage by landing this
patch. :-(