Make it possible to include another file and port print_preview unit test to unit_tests.

Make it possible to include another file and port print_preview unit test to unit_tests.

Print Preview unit_tests were good to tackle because it brought in a dependency on the need
to include another js file.

This also hilighted another need - that of being able to include non-generative js files in gyp
and have them copied to the test_data dir, while doing both generation and copy for gtest files.
I changed the "extension" of files which generate C++ with gtest-like syntax to .gtestjs and
added a copyjs rule to the chrome_tests.gypi.

FWIW, I believe this is mostly needed for unit_tests and only applied it there, although I would
be amenable to also making this change for the webui tests to make it clear that they are not
plain js files.

R=dpapad@chromium.org
BUG=101443, 102222
TEST=unit_tests --gtest_filter=PrintPreviewUtilsUnitTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109310

[Sheridan Rawlins, 2011-11-03 02:19:06]

Auxiliary files can be included with addLibraries (in the case where the test
file is defining the fixture) or GEN_INCLUDE([...]) for test files wishing to
share a common fixture.

[dpapad, 2011-11-03 16:14:34]

Another reviewer should be added for the gyp changes as I am not familiar with
how gyp works.

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs
(right):

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs:1:
// -*- mode: js2 -*-
Is this needed? If so what does it mean?

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs:32:
TEST_F('PrintPreviewUtilsUnitTest', 'testIsPositiveInteger', function() {
Nit: Shouldn't all the test names start with a capital letter? Also I think the
prefix 'test' can be dropped now since it is redundant. Same holds for
chrome/test/data/unit/framework_unittest.gtestjs.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:36: function incFileToPaths(includeFile) {
Prefer the full name includeFileToPaths over the abbreviated one incFileToPaths
as per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera....

Also document functions in this file.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:48: for(var i = 0; i < includes.length; ++i) {
Space after for.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:80: for(var i = 0; i < addLibraries.length; ++i) {
Space after for.

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/unit/framework...
File chrome/test/data/unit/framework_unittest.gtestjs (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/unit/framework...
chrome/test/data/unit/framework_unittest.gtestjs:27: function
FrameworkUnitTestFail() {}
I think that this could be compressed a bit as follows.

function FrameworkUnitTestFail() {
  this.__proto__ = FrameworkUnitTest.prototype;
  this.testShouldFail = true;
}

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api.js
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:113: * Add libraries before loading this test
file.
Would this be a bit more clear as "Libraries to add before loading this test
file."?

[arv (Not doing code reviews), 2011-11-03 19:04:12]

Can we name these files *.gtest.js instead?

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs
(right):

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs:16:
addLibraries: [
addLibraries is not the right name for this. addLibraries implies a method.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:48: for(var i = 0; i < includes.length; ++i) {
nit 

i++

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:51: eval(js);
I assume we don't want to eval this in the scope of GEN_INCLUDE?

('global', eval)(js);

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:52: genIncludes =
genIncludes.concat(incPaths.base);
push?

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:67: this[testFixture].prototype.addLibraries.map(
This looks strange. Why are we getting this from the prototype?

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/unit/framework...
File chrome/test/data/unit/framework_unittest.gtestjs (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/unit/framework...
chrome/test/data/unit/framework_unittest.gtestjs:27: function
FrameworkUnitTestFail() {}
On 2011/11/03 16:14:34, dpapad wrote:
> I think that this could be compressed a bit as follows.
> 
> function FrameworkUnitTestFail() {
>   this.__proto__ = FrameworkUnitTest.prototype;
>   this.testShouldFail = true;
> }

That is bad practice. The following would fail then:

var test = new FrameworkUnitTestFail;
assertTrue(test instanceof FrameworkUnitTestFail);

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api.js
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:14: (function(window) {
rename window to exports for consistence with code out there

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:116: addLibraries: [],
This should just be called libraries or something like that.

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:1323: })(('window' in this) ? window : this);
What does window in this do here? How is this better than just passing this at
all times?

[Sheridan Rawlins, 2011-11-05 16:51:25]

Brad, can you review the gyp changes, and address Arv's question:

arv: Can we name these files *.gtest.js instead?

-SCR

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs
(right):

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs:1:
// -*- mode: js2 -*-
It was an emacs thing; not needed/desired.  Removed.

Done.

On 2011/11/03 16:14:34, dpapad wrote:
> Is this needed? If so what does it mean?

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs:16:
addLibraries: [
How's |extraLibraries| ?

Done.

On 2011/11/03 19:04:12, arv wrote:
> addLibraries is not the right name for this. addLibraries implies a method.

http://codereview.chromium.org/8438063/diff/1/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_utils_unittest.gtestjs:32:
TEST_F('PrintPreviewUtilsUnitTest', 'testIsPositiveInteger', function() {
On 2011/11/03 16:14:34, dpapad wrote:
> Nit: Shouldn't all the test names start with a capital letter? Also I think
the
> prefix 'test' can be dropped now since it is redundant. Same holds for
> chrome/test/data/unit/framework_unittest.gtestjs.

Done.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:36: function incFileToPaths(includeFile) {
FWIW, this is javascript
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml
But I like the suggestion; maybe this should be added to the js style guide...

Done.

On 2011/11/03 16:14:34, dpapad wrote:
> Prefer the full name includeFileToPaths over the abbreviated one
incFileToPaths
> as per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera....
> 
> Also document functions in this file.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:48: for(var i = 0; i < includes.length; ++i) {
On 2011/11/03 16:14:34, dpapad wrote:
> Space after for.

Done.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:48: for(var i = 0; i < includes.length; ++i) {
On 2011/11/03 19:04:12, arv wrote:
> nit 
> 
> i++

Done.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:51: eval(js);
On 2011/11/03 19:04:12, arv wrote:
> I assume we don't want to eval this in the scope of GEN_INCLUDE?
> 
> ('global', eval)(js);

Done.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:52: genIncludes =
genIncludes.concat(incPaths.base);
Yeah, that's what I wanted, but the thing being pushed is an array.  It's a
little verbose to use the Array.prototype.push.apply in order to get the second
parameter to unroll its elements, but it works.

Done.

On 2011/11/03 19:04:12, arv wrote:
> push?

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:67: this[testFixture].prototype.addLibraries.map(
I wanted to give the best of both worlds:
1) GEN_INCLUDE - allows inclusion during the generation phase and for the entire
file.  This allows general support files or, as was described to me over gchat,
the ability for several test files testing different aspects of the same page to
share a fixture for the browsePreload, SetUp, etc.

2) Fixture declaration - allows test files to declare several fixtures as needed
for tests in the same js file to pull in different libraries.  I prefer this,
actually, but (a) there may be test libraries which will fail to load if their
support libraries aren't available, and (b) the case of sharing fixtures above.

If we had to reduce to only GEN_INCLUDE, I suppose it wouldn't be too terrible,
just some tests may include more than they need or those cases would have to
create multiple test-files.

I like having both, but I can see the simplicity of reducing to one.  Let me
know how strongly you feel about using only GEN_INCLUDE.

On 2011/11/03 19:04:12, arv wrote:
> This looks strange. Why are we getting this from the prototype?

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:80: for(var i = 0; i < addLibraries.length; ++i) {
On 2011/11/03 16:14:34, dpapad wrote:
> Space after for.

Done.

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/unit/framework...
File chrome/test/data/unit/framework_unittest.gtestjs (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/unit/framework...
chrome/test/data/unit/framework_unittest.gtestjs:27: function
FrameworkUnitTestFail() {}
Ok, so this is a nop.  <not> Done.  :)

On 2011/11/03 19:04:12, arv wrote:
> On 2011/11/03 16:14:34, dpapad wrote:
> > I think that this could be compressed a bit as follows.
> > 
> > function FrameworkUnitTestFail() {
> >   this.__proto__ = FrameworkUnitTest.prototype;
> >   this.testShouldFail = true;
> > }
> 
> That is bad practice. The following would fail then:
> 
> var test = new FrameworkUnitTestFail;
> assertTrue(test instanceof FrameworkUnitTestFail);

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api.js
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:14: (function(window) {
On 2011/11/03 19:04:12, arv wrote:
> rename window to exports for consistence with code out there

Done.

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:113: * Add libraries before loading this test
file.
On 2011/11/03 16:14:34, dpapad wrote:
> Would this be a bit more clear as "Libraries to add before loading this test
> file."?

Done.

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:116: addLibraries: [],
Changed to extraLibraries - does that work for you?

Done.

On 2011/11/03 19:04:12, arv wrote:
> This should just be called libraries or something like that.

http://codereview.chromium.org/8438063/diff/1/chrome/test/data/webui/test_api...
chrome/test/data/webui/test_api.js:1323: })(('window' in this) ? window : this);
I forget why I needed this at some point - it may have been when experimenting
with node.js...

I'll remove and see how things fly with trybots.

Done.

On 2011/11/03 19:04:12, arv wrote:
> What does window in this do here? How is this better than just passing this at
> all times?

[Sheridan Rawlins, 2011-11-05 22:28:02]

Added comments, shortened the push call, and asked arv for more clarification
about the use of prototypes to fetch test fixture members.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:36: function incFileToPaths(includeFile) {
Hmm... missed the document part...  tackling in this patch.

On 2011/11/03 16:14:34, dpapad wrote:
> Prefer the full name includeFileToPaths over the abbreviated one
incFileToPaths
> as per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera....
> 
> Also document functions in this file.

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:52: genIncludes =
genIncludes.concat(incPaths.base);
Sorry, I guess in this loop, it's a singular item.  I'll simplify.  Thanks.

On 2011/11/05 16:51:25, Sheridan Rawlins wrote:
> Yeah, that's what I wanted, but the thing being pushed is an array.  It's a
> little verbose to use the Array.prototype.push.apply in order to get the
second
> parameter to unroll its elements, but it works.
> 
> Done.
> 
> On 2011/11/03 19:04:12, arv wrote:
> > push?
>

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:67: this[testFixture].prototype.addLibraries.map(
Or was your concern with using the prototype?  If it would make more sense,  can
instantiate the test fixture and use its properties, but I thought just reading
the prototype was slightly lighter weight...

Please clarify your concern.  Thanks.

On 2011/11/03 19:04:12, arv wrote:
> This looks strange. Why are we getting this from the prototype?

[bradn, 2011-11-07 06:10:37]

http://codereview.chromium.org/8438063/diff/15001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8438063/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1990: '-c', 'import shutil,sys;
shutil.copyfile(sys.argv[1], sys.argv[2])',
Minor nit. It might be nice to put this in a separate script.
As its general I'd argue something in src/build maybe.

http://codereview.chromium.org/8438063/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1997: 'extension': 'gtestjs',
So gtest.js might work here (like 80% sure it'll work for msvs).
If it doesn't work currently for make, it can be fixed.
xcode might be an issue, I've enquired with folks and will reply when I know.
One issue might be how it resolves the fact that there'd be a match on both js
and gtest.js for some targets, as it might do something different per platform.
I'll reply again when I know more.

[Sheridan Rawlins, 2011-11-08 01:11:49]

Regarding suffixing in .gtest.js, a quick first pass reveals that gyp doesn't
support it currently.  I feel that is separate from and out of scope of this CL.

I believe all comments are addressed.

http://codereview.chromium.org/8438063/diff/15001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8438063/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1990: '-c', 'import shutil,sys;
shutil.copyfile(sys.argv[1], sys.argv[2])',
On 2011/11/07 06:10:38, bradn wrote:
> Minor nit. It might be nice to put this in a separate script.
> As its general I'd argue something in src/build maybe.

Done.

http://codereview.chromium.org/8438063/diff/15001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1997: 'extension': 'gtestjs',
This needs gyp support to accomplish and I feel is outside of the scope of this
CL.

http://code.google.com/p/gyp/issues/detail?id=234 and http://crbug.com/103311

On 2011/11/07 06:10:38, bradn wrote:
> So gtest.js might work here (like 80% sure it'll work for msvs).
> If it doesn't work currently for make, it can be fixed.
> xcode might be an issue, I've enquired with folks and will reply when I know.
> One issue might be how it resolves the fact that there'd be a match on both js
> and gtest.js for some targets, as it might do something different per
platform.
> I'll reply again when I know more.

[dpapad, 2011-11-08 01:17:53]

LGTM.

[arv (Not doing code reviews), 2011-11-09 03:02:05]

lgtm

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js
File chrome/test/base/js2gtest.js (right):

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:57: var browsePreload =
this[testFixture].prototype.browsePreload;
var proto = this[testFixture].prototype;

http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#ne...
chrome/test/base/js2gtest.js:67: this[testFixture].prototype.addLibraries.map(
I found it strange that you interact with the prototype and that the prototype
has mutable objects on it.

Since this is just a special case I'm now fine with it even though it is pretty
unorthodox.

[bradn, 2011-11-09 05:13:24]

Lgtm
On Nov 8, 2011 7:02 PM, <arv@chromium.org> wrote:

> lgtm
>
>
>
>
> http://codereview.chromium.**org/8438063/diff/1/chrome/**
>
test/base/js2gtest.js<http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js>
> File chrome/test/base/js2gtest.js (right):
>
> http://codereview.chromium.**org/8438063/diff/1/chrome/**
>
test/base/js2gtest.js#**newcode57<http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#newcode57>
> chrome/test/base/js2gtest.js:**57: var browsePreload =
> this[testFixture].prototype.**browsePreload;
> var proto = this[testFixture].prototype;
>
> http://codereview.chromium.**org/8438063/diff/1/chrome/**
>
test/base/js2gtest.js#**newcode67<http://codereview.chromium.org/8438063/diff/1/chrome/test/base/js2gtest.js#newcode67>
> chrome/test/base/js2gtest.js:**67:
> this[testFixture].prototype.**addLibraries.map(
> I found it strange that you interact with the prototype and that the
> prototype has mutable objects on it.
>
> Since this is just a special case I'm now fine with it even though it is
> pretty unorthodox.
>
>
http://codereview.chromium.**org/8438063/<http://codereview.chromium.org/8438...
>

[commit-bot: I haz the power, 2011-11-09 05:22:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scr@chromium.org/8438063/20001

[commit-bot: I haz the power, 2011-11-09 07:32:04]

Try job failure for 8438063-20001 (retry) (retry) (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, step "chrome_frame_net_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[M-A Ruel, 2011-11-23 03:06:37]

http://codereview.chromium.org/8438063/diff/20001/build/cp.py
File build/cp.py (right):

http://codereview.chromium.org/8438063/diff/20001/build/cp.py#newcode1
build/cp.py:1: #!/usr/bin/python
This file is not needed. Use the gyp action 'copies' instead.

[bradn, 2011-11-23 04:11:00]

http://codereview.chromium.org/8438063/diff/20001/build/cp.py
File build/cp.py (right):

http://codereview.chromium.org/8438063/diff/20001/build/cp.py#newcode1
build/cp.py:1: #!/usr/bin/python
On 2011/11/23 03:06:38, Marc-Antoine Ruel wrote:
> This file is not needed. Use the gyp action 'copies' instead.

Copies doesn't work for his use case, as he need something that applies to each
instance of a rule.