Port nacl_integration exit_status test to browser_tests.

Port nacl_integration exit_status test to browser_tests.

This CL removes a sanity check in nacl_browsertest_util.cc because it is no
longer guanteed a "test" will be a simple file name. Tests may now include a
query string.

BUG= http://code.google.com/p/nativeclient/issues/detail?id=2959

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

[Nick Bray (chromium), 2012-09-10 20:22:26]

mseaborn: primary reviewer.
brettw: OWNER for chrome/chrome_tests.gypi (trivial change)

[brettw, 2012-09-10 23:35:26]

owner lgtm rubberstamp

[Mark Seaborn, 2012-09-11 22:10:06]

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
File chrome/test/data/nacl/nacl_browser_test.gypi (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
chrome/test/data/nacl/nacl_browser_test.gypi:63: '<(DEPTH)/ppapi',
Surely the existing entry is enough to do (say) '#include "ppapi/cpp/var.h"'?

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
chrome/test/data/nacl/nacl_test_data.gyp:17:
'<(DEPTH)/ppapi/native_client/tools/browser_tester/browserdata/nacltest.js',
Nit: do you want a TODO to move this into chrome/test/data when all tests are
converted?

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:14:
RunNaClIntegrationTest(FILE_PATH_LITERAL(
Indenting by 4 isn't consistent with the block above

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:16: ));
Closing ) on a new line isn't the usual C++ style?

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.cc (left):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:140: FilePath real_path =
test_server_->document_root().Append(test_file);
Why does this go?  Comment in commit message.  I don't know this code well
enough...

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:99: // A message handler for
nacl_integration tests ported to be browser_tests.
Commenting this based on where the code came from doesn't seem quite right... 
It's OK to have this, but first explain what it's for and what it does now.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:126: LOG(INFO) << "|||| " << message;
Aren't LOG(INFO) omitted from the buildbot logs (at least on Windows), unlike
chrome_browser_tests' existing logging?

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:130: const std::string& type,
Indent by 1 more space

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.h:87: // Run a test that was written
using the nacl_integration test jig.
'nacl_integration' refers to the step that runs Scons.  Use a different term in
this comment?

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.h:88: void RunNaClIntegrationTest(const
FilePath::StringType& test_file);
Nit: test_file isn't just a filename, it contains a URL '?' fragment.

[Nick Bray (chromium), 2012-09-12 01:19:12]

PTAL

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
File chrome/test/data/nacl/nacl_browser_test.gypi (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
chrome/test/data/nacl/nacl_browser_test.gypi:63: '<(DEPTH)/ppapi',
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Surely the existing entry is enough to do (say) '#include "ppapi/cpp/var.h"'?

Removed.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/data/nacl/...
chrome/test/data/nacl/nacl_test_data.gyp:17:
'<(DEPTH)/ppapi/native_client/tools/browser_tester/browserdata/nacltest.js',
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Nit: do you want a TODO to move this into chrome/test/data when all tests are
> converted?

Done.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:14:
RunNaClIntegrationTest(FILE_PATH_LITERAL(
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Indenting by 4 isn't consistent with the block above

Done.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:16: ));
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Closing ) on a new line isn't the usual C++ style?

Done.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.cc (left):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:140: FilePath real_path =
test_server_->document_root().Append(test_file);
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Why does this go?  Comment in commit message.  I don't know this code well
> enough...

Because the new tests put a query string at the end.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:99: // A message handler for
nacl_integration tests ported to be browser_tests.
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Commenting this based on where the code came from doesn't seem quite right... 
> It's OK to have this, but first explain what it's for and what it does now.

Done.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:130: const std::string& type,
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Indent by 1 more space

Done.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.h:87: // Run a test that was written
using the nacl_integration test jig.
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> 'nacl_integration' refers to the step that runs Scons.  Use a different term
in
> this comment?

Done.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.h:88: void RunNaClIntegrationTest(const
FilePath::StringType& test_file);
On 2012/09/11 22:10:06, Mark Seaborn wrote:
> Nit: test_file isn't just a filename, it contains a URL '?' fragment.

Done.

[Mark Seaborn, 2012-09-12 18:09:45]

LGTM to the code, but it seems to be flaky on the bots:

[31810:31810:0911/190245:3898951441:INFO:nacl_browsertest_util.cc(130)] ||||
[SHUTDOWN] 2 passed, 0 failed, 0 errors
[31810:31810:0911/190245:3898979596:FATAL:histogram.cc(131)] Check failed: count
== redundant_count_ (1 vs. 2)
[31810:31810:0911/190245:3898979596:FATAL:histogram.cc(131)] Check failed: count
== redundant_count_ (1 vs. 2)

from
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/5609...
(a supposedly successful run, but NaClBrowserTestGLibc.ExitStatus was retried)

You probably don't want to commit this without checking that.

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest_util.cc (left):

https://chromiumcodereview.appspot.com/10918152/diff/1/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest_util.cc:140: FilePath real_path =
test_server_->document_root().Append(test_file);
On 2012/09/12 01:19:13, Nick Bray (chromium) wrote:
> On 2012/09/11 22:10:06, Mark Seaborn wrote:
> > Why does this go?  Comment in commit message.  I don't know this code well
> > enough...
> 
> Because the new tests put a query string at the end.

So please add a note in the commit message that you took out a sanity check.

Or you could move the check to RunLoadTest()?

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/data/n...
File chrome/test/data/nacl/nacl_browser_test.gypi (right):

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/data/n...
chrome/test/data/nacl/nacl_browser_test.gypi:147: }
There are no changes to this file except whitespace, so you can undo this file's
change.

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_util.cc:103: // URL requests.  This message
handler decodes and responds to these requests.
Be consistent about using 1 or 2 spaces between sentences

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_util.h:91: // these tests having a stronger
afinity with the Chrome repo. This method
'affinity'

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_util.h:92: // provides a compatability layer
to simplify turning nacl_integration tests
'compatibility'

[Nick Bray (chromium), 2012-09-12 20:50:59]

jar@ says the UMA assert is either a highly improbable race or memory
corruption.  I audited the code and didn't see any obvious ways this particular
test could cause corruption.  Waiting for try bots, and I'll commit if they're
clean.

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/data/n...
File chrome/test/data/nacl/nacl_browser_test.gypi (right):

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/data/n...
chrome/test/data/nacl/nacl_browser_test.gypi:147: }
On 2012/09/12 18:09:45, Mark Seaborn wrote:
> There are no changes to this file except whitespace, so you can undo this
file's
> change.

Done.

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
File chrome/test/nacl/nacl_browsertest_util.cc (right):

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_util.cc:103: // URL requests.  This message
handler decodes and responds to these requests.
On 2012/09/12 18:09:45, Mark Seaborn wrote:
> Be consistent about using 1 or 2 spaces between sentences

Done.

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
File chrome/test/nacl/nacl_browsertest_util.h (right):

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_util.h:91: // these tests having a stronger
afinity with the Chrome repo. This method
On 2012/09/12 18:09:45, Mark Seaborn wrote:
> 'affinity'

Done.

https://chromiumcodereview.appspot.com/10918152/diff/10001/chrome/test/nacl/n...
chrome/test/nacl/nacl_browsertest_util.h:92: // provides a compatability layer
to simplify turning nacl_integration tests
On 2012/09/12 18:09:45, Mark Seaborn wrote:
> 'compatibility'

Done.