Add Pepper API tests for chrome.socket.

Add Pepper API tests for chrome.socket.

This CL also refactors the gyp definitions for building NaCl testing modules.

BUG=226303
TEST=None

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

[yzshen1, 2013-04-10 21:37:25]

Hi,

Would you please take a look?

Antony, please look at files:
- chrome/test/data/extensions/api_tests/*
- chrome/browser/extensions/api/socket/socket_apitest.cc
These PPAPI tests are put here temporarily. I am planning to create testing
framework for all extension APIs in Pepper. But I don't think I have time to do
that in this milestone. On the other hand, it is good to have some tests for the
API that I have implemented. Therefore, I put them here for now with a TODO.
What do you think?


Nick and Brad, please look at those changes to .gyp[i] files.
I moved nacl_browser_test.gypi to ppapi_nacl_test_common.gyp. Besides, I changed
it to use 'direct_dependent_settings' instead of 'target_defaults'. (And I had
to make some more changes related to variable expasion to make it work.)

I think 'direct_dependent_settings' is nicer because we have finer-grained
control over what targets are affected by it. But I realized that we still need
to include untrusted.gypi which uses 'target_defaults', so the benefit may be
questionable. Please let me know if you want me to change it back.

Thanks!

[bradn, 2013-04-12 00:04:25]

Hmmn, clever. Uh, yeah shame about untrusted.gypi.
Let me see what Nick's asthetic reaction is.
It has the advantage that targets which don't need the magic, don't get it.
Might almost be interesting to revisit untrusted.gypi to see if it can be done
in that way, but I have a feeling probably not, since it has a lot of very
round-a-bout stuff.

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/nacl/nacl...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/nacl/nacl...
chrome/test/data/nacl/nacl_test_data.gyp:6: 'includes': [
Missing a space on these three lines.

[Nick Bray (chromium), 2013-04-12 16:57:45]

Sorry about the lag, I have been working on a time-sensitive issue.

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/socket/ppapi/controller.js (right):

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/socket/ppapi/controller.js:17:
chrome.test.runTests([ testAll ]);
nit: [ testAll ] => [testAll]

https://codereview.chromium.org/13811036/diff/7001/ppapi/ppapi_nacl_test_comm...
File ppapi/ppapi_nacl_test_common.gyp (right):

https://codereview.chromium.org/13811036/diff/7001/ppapi/ppapi_nacl_test_comm...
ppapi/ppapi_nacl_test_common.gyp:47: 'out_newlib32':
'>(nacl_newlib_out_dir)/>(nexe_target)_newlib_x86_32.nexe',
Could you default this to nacl_test_data?

https://codereview.chromium.org/13811036/diff/7001/ppapi/ppapi_nacl_test_comm...
ppapi/ppapi_nacl_test_common.gyp:106:
'>(current_depth)/native_client_sdk/src/tools/create_nmf.py',
Why not use DEPTH?

Also, can you force a diff with an svn move?  I want to see what changed.

I am also not entirely convinced that ppapi/ is the correct directory for this. 
nexe != ppapi.

[yzshen1, 2013-04-12 19:15:05]

Thanks, Brad and Nick.

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/socket/ppapi/controller.js (right):

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/socket/ppapi/controller.js:17:
chrome.test.runTests([ testAll ]);
On 2013/04/12 16:57:45, Nick Bray (chromium) wrote:
> nit: [ testAll ] => [testAll]

Done.

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/nacl/nacl...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/13811036/diff/7001/chrome/test/data/nacl/nacl...
chrome/test/data/nacl/nacl_test_data.gyp:6: 'includes': [
On 2013/04/12 00:04:25, bradn wrote:
> Missing a space on these three lines.

Done.

https://codereview.chromium.org/13811036/diff/7001/ppapi/ppapi_nacl_test_comm...
File ppapi/ppapi_nacl_test_common.gyp (right):

https://codereview.chromium.org/13811036/diff/7001/ppapi/ppapi_nacl_test_comm...
ppapi/ppapi_nacl_test_common.gyp:47: 'out_newlib32':
'>(nacl_newlib_out_dir)/>(nexe_target)_newlib_x86_32.nexe',
On 2013/04/12 16:57:45, Nick Bray (chromium) wrote:
> Could you default this to nacl_test_data?

Do you mean default nexe_destination_dir?
One problem is that I cannot define it in the 'variables' section starting at
line 25, because the same scope needs to use this variable (line 27-29). :/

https://codereview.chromium.org/13811036/diff/7001/ppapi/ppapi_nacl_test_comm...
ppapi/ppapi_nacl_test_common.gyp:106:
'>(current_depth)/native_client_sdk/src/tools/create_nmf.py',
On 2013/04/12 16:57:45, Nick Bray (chromium) wrote:
> Why not use DEPTH?
I am not sure whether there is a better way, but both <(DEPTH) and >(DEPTH)
resolve to '../' which is relative to this file. I have to have this new
variable to specify depth relative to its direct dependents. I would be happy to
know if there is a better way.

> 
> Also, can you force a diff with an svn move?  I want to see what changed.
I changed the indent so git doesn't think it is a 'move' anymore. :/
I tried to adjust the similarity threshold, but failed, even if I changed it to
1%.

I uploaded patchset #6 so you could see the actual changes. (The indent of line
19-187 is not adjusted for this patch.)

> 
> I am also not entirely convinced that ppapi/ is the correct directory for
this. 
> nexe != ppapi.
Later I would like to create a new target extension_ppapi_nacl_test to test all
apps v2 APIs in Pepper. And I am planning to put its gyp definition in ppapi/.
That is why I decided to put ppapi_nacl_test_common in ppapi/.

Would you please suggest where I should place this file? Maybe somewhere inside
native_client/? (I am not familiar with how it is organized.)
Thanks!

[asargent_no_longer_on_chrome, 2013-04-12 21:37:52]

Brad may already be familiar with the assumptions the chrome infrastructure
folks are making about the location/immutability of test data files, but I know
maruel and some others are working on a project called "test isolates" as
detailed here: http://dev.chromium.org/developers/testing/isolated-testing

It might be worth asking maruel whether the structure you're setting up here to
build the nexe and put it in the app's directory is compatible with what they're
working on.

https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/api/socket/socket_apitest.cc (right):

https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/socket/socket_apitest.cc:88:
"chrome/test/data/extensions/socket/ppapi/newlib");
I think you're missing 'api_test' in the path here. 

BTW, you can simplify this slightly - the test_data_dir_ member variable that
you inherit from ExtensionApiTest  via SocketApiTest is already pointing at
src/chrome/test/data/extensions/api_test/, so you should be able to just do:

app_dir_ = test_data_dir_.AppendASCII("socket/ppapi/newlib");

[yzshen1, 2013-04-12 22:16:55]

> Brad may already be familiar with the assumptions the chrome infrastructure
> folks are making about the location/immutability of test data files, but I
know
> maruel and some others are working on a project called "test isolates" as
> detailed here: http://dev.chromium.org/developers/testing/isolated-testing

> It might be worth asking maruel whether the structure you're setting up here
to
> build the nexe and put it in the app's directory is compatible with what
they're
> working on.

With a quick glance, I think the isolated-testing project solves the problem of
reading test data from the *source* at runtime. In this case, all test data is
written to our output directory, under out/{Debug,Release}/test_data.

+maruel as reviewer.
Hi, Marc-Antoine.
If this test doesn't read data from our source tree during runtime, do I need to
take any action in order to comply with isolated-testing? Thanks!

https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/api/socket/socket_apitest.cc (right):

https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/socket/socket_apitest.cc:88:
"chrome/test/data/extensions/socket/ppapi/newlib");
On 2013/04/12 21:37:52, Antony Sargent wrote:
> I think you're missing 'api_test' in the path here. 
Thanks! I updated the output path that the test data is written to and the path
here.

> 
> BTW, you can simplify this slightly - the test_data_dir_ member variable that
> you inherit from ExtensionApiTest  via SocketApiTest is already pointing at
> src/chrome/test/data/extensions/api_test/, so you should be able to just do:
> 
> app_dir_ = test_data_dir_.AppendASCII("socket/ppapi/newlib");

Here app_dir_ is a location inside our output directory. It is not part of
src/chrome/test/data/extensions/api_test/.
>

[yzshen1, 2013-04-12 22:49:18]

On 2013/04/12 22:16:55, yzshen1 wrote:
> > Brad may already be familiar with the assumptions the chrome infrastructure
> > folks are making about the location/immutability of test data files, but I
> know
> > maruel and some others are working on a project called "test isolates" as
> > detailed here: http://dev.chromium.org/developers/testing/isolated-testing
> 
> > It might be worth asking maruel whether the structure you're setting up here
> to
> > build the nexe and put it in the app's directory is compatible with what
> they're
> > working on.
> 
> With a quick glance, I think the isolated-testing project solves the problem
of
> reading test data from the *source* at runtime. In this case, all test data is
> written to our output directory, under out/{Debug,Release}/test_data.
> 
> +maruel as reviewer.
> Hi, Marc-Antoine.
> If this test doesn't read data from our source tree during runtime, do I need
to
> take any action in order to comply with isolated-testing? Thanks!

Hmm. I looked at browser_tests.isolated and realized that testing data in the
output directory should also be tracked. (Right?)

I will take a closer look at the docs and find out what I should change.
Thanks!

> 
>
https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
> File chrome/browser/extensions/api/socket/socket_apitest.cc (right):
> 
>
https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
> chrome/browser/extensions/api/socket/socket_apitest.cc:88:
> "chrome/test/data/extensions/socket/ppapi/newlib");
> On 2013/04/12 21:37:52, Antony Sargent wrote:
> > I think you're missing 'api_test' in the path here. 
> Thanks! I updated the output path that the test data is written to and the
path
> here.
> 
> > 
> > BTW, you can simplify this slightly - the test_data_dir_ member variable
that
> > you inherit from ExtensionApiTest  via SocketApiTest is already pointing at
> > src/chrome/test/data/extensions/api_test/, so you should be able to just do:
> > 
> > app_dir_ = test_data_dir_.AppendASCII("socket/ppapi/newlib");
> 
> Here app_dir_ is a location inside our output directory. It is not part of
> src/chrome/test/data/extensions/api_test/.
> >

[M-A Ruel, 2013-04-13 00:16:11]

Thanks for summoning me,
cc'ed Vadim and Chris FYI as an example review.

On 2013/04/12 22:49:18, yzshen1 wrote:
> On 2013/04/12 22:16:55, yzshen1 wrote:
> > > Brad may already be familiar with the assumptions the chrome
infrastructure
> > > folks are making about the location/immutability of test data files, but I
> > know
> > > maruel and some others are working on a project called "test isolates" as
> > > detailed here: http://dev.chromium.org/developers/testing/isolated-testing

Test data file must be immutable. We'll eventually enforce this.


> > > It might be worth asking maruel whether the structure you're setting up
here
> > to
> > > build the nexe and put it in the app's directory is compatible with what
> > they're
> > > working on.

As long as "app's directory" is a temporary directory, it's fine. Please do not
try to write inside the checkout, you'll have to rewrite this code sooner or
later.
So if I understand correctly and 'nexe_destination_dir':
'test_data/chrome/test/data/extensions/api_test/socket/ppapi' means that
something will be written there, then it's generally considered bad tree
hygiene. Put generated stuff under PRODUCT_DIR or the other subdirectories.


> > With a quick glance, I think the isolated-testing project solves the problem
> of
> > reading test data from the *source* at runtime. In this case, all test data
is
> > written to our output directory, under out/{Debug,Release}/test_data.

To be more explicit, it's about all runtime dependencies. Isolated testing
doesn't make any difference if each runtime dependencies is checked in or built.
They are treated the same way.


> > +maruel as reviewer.
> > Hi, Marc-Antoine.
> > If this test doesn't read data from our source tree during runtime, do I
need
> to
> > take any action in order to comply with isolated-testing? Thanks!
> 
> Hmm. I looked at browser_tests.isolated and realized that testing data in the
> output directory should also be tracked. (Right?)

Exact, everything under chrome/test/data/ is already mapped when the test is run
so if you put files there, you don't have to do anything specific. We did that
to simplify the live of developers. If you add a new target, like a new .nexe in
PRODUCT_DIR that is needed for a test case to succeed, then it needs to be added
to the corresponding .isolate file.

https://codereview.chromium.org/13811036/diff/36001/ppapi/ppapi_nacl_test_com...
File ppapi/ppapi_nacl_test_common.gyp (right):

https://codereview.chromium.org/13811036/diff/36001/ppapi/ppapi_nacl_test_com...
ppapi/ppapi_nacl_test_common.gyp:188: },
Weird alignment

[yzshen1, 2013-04-13 00:53:14]

> Test data file must be immutable. We'll eventually enforce this.
The test data includes a NaCl module which is built from source code.
And all the test data files are built/copied into the output directory. They
won't change during runtime.

I think that meets the requirement, right?


> As long as "app's directory" is a temporary directory, it's fine. Please do
not
> try to write inside the checkout, you'll have to rewrite this code sooner or
> later.
> So if I understand correctly and 'nexe_destination_dir':
> 'test_data/chrome/test/data/extensions/api_test/socket/ppapi' means that
> something will be written there, then it's generally considered bad tree
> hygiene. Put generated stuff under PRODUCT_DIR or the other subdirectories.

I didn't write inside the checkout, the compile result (along with some other
files) is written to
out/{Debug,Release}/test_data/chrome/test/data/extensions/api_test/socket/ppapi

Have I misunderstood anything?

> Exact, everything under chrome/test/data/ is already mapped when the test is
run
> so if you put files there, you don't have to do anything specific. We did that
> to simplify the live of developers. If you add a new target, like a new .nexe
in
> PRODUCT_DIR that is needed for a test case to succeed, then it needs to be
added
> to the corresponding .isolate file.

That makes sense. I will make the change (after my machine is setup after the
office move).

> Weird alignment
I did this to make git understand that I 'moved' the file and it is easier for
reviewers to see the diff. Otherwise, even if I set similarity threshold to 1%,
git still treated it as a new file.

I will correct the indent before commit.

Thanks!

[asargent_no_longer_on_chrome, 2013-04-15 17:54:05]

lgtm

https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
File chrome/browser/extensions/api/socket/socket_apitest.cc (right):

https://codereview.chromium.org/13811036/diff/29001/chrome/browser/extensions...
chrome/browser/extensions/api/socket/socket_apitest.cc:88:
"chrome/test/data/extensions/socket/ppapi/newlib");

> Here app_dir_ is a location inside our output directory. It is not part of
> src/chrome/test/data/extensions/api_test/.

Oops, sorry, I missed that.

[yzshen1, 2013-04-17 09:17:46]

> > Exact, everything under chrome/test/data/ is already mapped when the test is
> run
> > so if you put files there, you don't have to do anything specific. We did
that
> > to simplify the live of developers. If you add a new target, like a new
.nexe
> in
> > PRODUCT_DIR that is needed for a test case to succeed, then it needs to be
> added
> > to the corresponding .isolate file.
> 
> That makes sense. I will make the change (after my machine is setup after the
> office move).

Hi, Marc-Antoine.

'<(PRODUCT_DIR)/test_data/chrome/' is already set in browser_tests.isolated, so
I think I don't need to do anything special.
All testing data will be in this directory.

Please let me know if there is still anything that I should change.

Thanks!

[yzshen1, 2013-04-22 05:56:14]

Friendly ping, Nick, Brad and Marc-Antoine.
Thanks!

On 2013/04/17 09:17:46, yzshen1 wrote:
> > > Exact, everything under chrome/test/data/ is already mapped when the test
is
> > run
> > > so if you put files there, you don't have to do anything specific. We did
> that
> > > to simplify the live of developers. If you add a new target, like a new
> .nexe
> > in
> > > PRODUCT_DIR that is needed for a test case to succeed, then it needs to be
> > added
> > > to the corresponding .isolate file.
> > 
> > That makes sense. I will make the change (after my machine is setup after
the
> > office move).
> 
> Hi, Marc-Antoine.
> 
> '<(PRODUCT_DIR)/test_data/chrome/' is already set in browser_tests.isolated,
so
> I think I don't need to do anything special.
> All testing data will be in this directory.
> 
> Please let me know if there is still anything that I should change.
> 
> Thanks!

[M-A Ruel, 2013-04-22 12:34:43]

On 2013/04/13 00:53:14, yzshen1 wrote:
> I didn't write inside the checkout, the compile result (along with some other
> files) is written to
>
out/{Debug,Release}/test_data/chrome/test/data/extensions/api_test/socket/ppapi
> 
> Have I misunderstood anything?

Ok, I thought you were initially referring to src/test_data/ or something like
that.

lgtm

[Nick Bray (chromium), 2013-04-22 21:58:53]

LGTM for what I own, although it might be wise to have Brad sign off on the GYP
magic.

[bradn, 2013-04-23 17:59:33]

LGTM

[commit-bot: I haz the power, 2013-04-23 18:10:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/13811036/67002

[commit-bot: I haz the power, 2013-04-23 22:19:37]

Change committed as 195915