Add failure case for manifest file testing.

Add failure case for manifest file testing.

Currently we only have testing for success case of the
manifest file. This CL adds a test case for testing when
a file resource is not found, and an entry is not found in
the manifest.

BUG=370895
TEST=Ran browser_tests --gtest_filter=*IrtManifestFile and trybots.

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

[hidehiko, 2014-05-28 15:59:41]

Could you take a look?

Thanks,
- hidehiko

[Mark Seaborn, 2014-05-28 16:30:34]

https://codereview.chromium.org/303683005/diff/1/chrome/test/data/nacl/manife...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_fail_test.cc (right):

https://codereview.chromium.org/303683005/diff/1/chrome/test/data/nacl/manife...
chrome/test/data/nacl/manifest_file/irt_manifest_file_fail_test.cc:1: /*
Could you possibly merge this into the existing test file,
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc?

Adding a new file and Gyp logic is a lot of overhead just for a tiny test of a
single function call.  While this isn't so bad as a one-off, I'm concerned about
what happens when people want to add more tests for further corner cases of
open_resource() (e.g. usage on threads, usage before and after initialising
PPAPI).  Ideally there should just be one place to add the test cases to.

https://codereview.chromium.org/303683005/diff/1/chrome/test/data/nacl/manife...
chrome/test/data/nacl/manifest_file/irt_manifest_file_fail_test.cc:39: error =
nacl_irt_resource_open.open_resource("non-exist-file", &desc);
Nit: "non-existent-file" perhaps?

[hidehiko, 2014-05-29 06:58:39]

Could you take another look?

https://codereview.chromium.org/303683005/diff/1/chrome/test/data/nacl/manife...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_fail_test.cc (right):

https://codereview.chromium.org/303683005/diff/1/chrome/test/data/nacl/manife...
chrome/test/data/nacl/manifest_file/irt_manifest_file_fail_test.cc:1: /*
On 2014/05/28 16:30:34, Mark Seaborn wrote:
> Could you possibly merge this into the existing test file,
> chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc?
> 
> Adding a new file and Gyp logic is a lot of overhead just for a tiny test of a
> single function call.  While this isn't so bad as a one-off, I'm concerned
about
> what happens when people want to add more tests for further corner cases of
> open_resource() (e.g. usage on threads, usage before and after initialising
> PPAPI).  Ideally there should just be one place to add the test cases to.

Ok, I rewrote most of code. How about this?

https://codereview.chromium.org/303683005/diff/1/chrome/test/data/nacl/manife...
chrome/test/data/nacl/manifest_file/irt_manifest_file_fail_test.cc:39: error =
nacl_irt_resource_open.open_resource("non-exist-file", &desc);
On 2014/05/28 16:30:34, Mark Seaborn wrote:
> Nit: "non-existent-file" perhaps?

Done, in irt_manifest_file_test.cc.

[Mark Seaborn, 2014-05-29 16:55:48]

LGTM with a tweak

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (right):

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:102: snprintf(buf,
sizeof(buf), "open_resource() result: %d", error);
Rather than always formatting the numeric errno message, how about checking it
here?  Something like:

if (errno != EINVAL) {
  // Format failure message: snprintf etc...
  return buf;
}
return "passed";

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:120: snprintf(buf,
sizeof(buf), "open_resource() result: %d", error);
Same here

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.html (right):

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.html:45: // 2 is an
error code for ENOENT.
Hard-coding a numeric errno value here isn't very clean.  It would be better if
you could just check for a result like "passed" here.  See other comment.

[hidehiko, 2014-05-30 04:49:58]

Thank you for review. Submitting.

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (right):

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:102: snprintf(buf,
sizeof(buf), "open_resource() result: %d", error);
On 2014/05/29 16:55:48, Mark Seaborn wrote:
> Rather than always formatting the numeric errno message, how about checking it
> here?  Something like:
> 
> if (errno != EINVAL) {
>   // Format failure message: snprintf etc...
>   return buf;
> }
> return "passed";

Done.

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:120: snprintf(buf,
sizeof(buf), "open_resource() result: %d", error);
On 2014/05/29 16:55:48, Mark Seaborn wrote:
> Same here

Done.

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.html (right):

https://codereview.chromium.org/303683005/diff/20001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.html:45: // 2 is an
error code for ENOENT.
On 2014/05/29 16:55:48, Mark Seaborn wrote:
> Hard-coding a numeric errno value here isn't very clean.  It would be better
if
> you could just check for a result like "passed" here.  See other comment.

Done.

[hidehiko, 2014-05-30 04:50:03]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-05-30 04:53:30]

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

[commit-bot: I haz the power, 2014-05-30 10:18:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-30 13:07:36]

Change committed as 273813