Media Galleries: Run API tests in a temp directory with an injected common.js file. Avoid the need …

Media Galleries: Run API tests in a temp directory with an injected common.js file. Avoid the need for nested tests to share common JS code.

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

[Lei Zhang, 2013-09-24 23:06:41]

This applies on top of https://codereview.chromium.org/24242010/ and
https://codereview.chromium.org/24356005/

[tommycli, 2013-09-24 23:26:10]

What's currently there is a kludge. This might be still kind of a hack though.
I'm going to wait for a second opinion from vandebo.

https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:109:
base::AutoReset<base::FilePath> reset(&test_data_dir_, temp_dir.path());
This override and the above copying logic probably belongs in its own inner
class.

You can put a nice name on it like InjectCommonScriptIntoTest (there's probably
a better name for this) - just so it's obvious what's going on.

Also it would only operate on a subset of the below tests.

https://codereview.chromium.org/24420003/diff/1/chrome/test/data/extensions/a...
File
chrome/test/data/extensions/api_test/media_galleries/copy_to_access/manifest.json
(right):

https://codereview.chromium.org/24420003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/media_galleries/copy_to_access/manifest.json:7:
"scripts": ["common.js", "test.js"]
I'm kind of uncomfy with the manifest pointing out a file that doesn't exist
until the cpp executing it copies it into a temp dir. - but I do understand the
benefits of this scheme too.

At the least there should be a comment explaining as such.

https://codereview.chromium.org/24420003/diff/1/chrome/test/data/extensions/a...
File
chrome/test/data/extensions/api_test/media_galleries/no_access/manifest.json
(right):

https://codereview.chromium.org/24420003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/media_galleries/no_access/manifest.json:7:
"scripts": ["common.js", "test.js"]
ditto earlier comment.

[Lei Zhang, 2013-09-24 23:41:57]

https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
(right):

https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:109:
base::AutoReset<base::FilePath> reset(&test_data_dir_, temp_dir.path());
On 2013/09/24 23:26:10, tommycli wrote:
> This override and the above copying logic probably belongs in its own inner
> class.
> 
> You can put a nice name on it like InjectCommonScriptIntoTest (there's
probably
> a better name for this) - just so it's obvious what's going on.
> 
> Also it would only operate on a subset of the below tests.

I would like to just make common.js available to all MG API tests. One should be
able to just write MG tests and take advantage of it without thinking about
whether it has been injected or not.

[tommycli, 2013-09-27 18:32:21]

On 2013/09/24 23:41:57, Lei Zhang wrote:
>
https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
> File chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
> chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:109:
> base::AutoReset<base::FilePath> reset(&test_data_dir_, temp_dir.path());
> On 2013/09/24 23:26:10, tommycli wrote:
> > This override and the above copying logic probably belongs in its own inner
> > class.
> > 
> > You can put a nice name on it like InjectCommonScriptIntoTest (there's
> probably
> > a better name for this) - just so it's obvious what's going on.
> > 
> > Also it would only operate on a subset of the below tests.
> 
> I would like to just make common.js available to all MG API tests. One should
be
> able to just write MG tests and take advantage of it without thinking about
> whether it has been injected or not.

Oh I see. Well then a comment in the manifests would probably be sufficiently
explanatory.

[tommycli, 2013-09-27 18:33:15]

On 2013/09/27 18:32:21, tommycli wrote:
> On 2013/09/24 23:41:57, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
> > File
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/24420003/diff/1/chrome/browser/extensions/api...
> >
chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc:109:
> > base::AutoReset<base::FilePath> reset(&test_data_dir_, temp_dir.path());
> > On 2013/09/24 23:26:10, tommycli wrote:
> > > This override and the above copying logic probably belongs in its own
inner
> > > class.
> > > 
> > > You can put a nice name on it like InjectCommonScriptIntoTest (there's
> > probably
> > > a better name for this) - just so it's obvious what's going on.
> > > 
> > > Also it would only operate on a subset of the below tests.
> > 
> > I would like to just make common.js available to all MG API tests. One
should
> be
> > able to just write MG tests and take advantage of it without thinking about
> > whether it has been injected or not.
> 
> Oh I see. Well then a comment in the manifests would probably be sufficiently
> explanatory.

Maybe in the cc file too - whatever it takes to make it obvious to the reader
that common.js is meant to be injected as a common lib into all the MG tests.

[Lei Zhang, 2013-09-27 19:26:58]

I renamed common.js to common_injected.js. Is that more obvious? I don't want to
have to put the same comment into every manifest.

[tommycli, 2013-09-27 19:30:03]

On 2013/09/27 19:26:58, Lei Zhang wrote:
> I renamed common.js to common_injected.js. Is that more obvious? I don't want
to
> have to put the same comment into every manifest.

Yeah I saw the comment in the cc file too. I think it's sufficiently obvious
now.

Thanks. lgtm.

[commit-bot: I haz the power, 2013-09-27 22:16:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/24420003/16001

[commit-bot: I haz the power, 2013-09-28 01:55:28]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-28 04:19:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/24420003/16001

[commit-bot: I haz the power, 2013-09-28 08:11:15]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-30 03:02:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/24420003/16001

[commit-bot: I haz the power, 2013-09-30 03:59:07]

Change committed as 225917