Break FileIO and URLLoader tests down into individual cases.

Break FileIO and URLLoader tests down into individual cases.
This will allow us to keep more of our tests running and identify the cases
where they fail, so we can fix them.
Add a comment to chrome_tests.gypi to keep mock header file together with
test_case.html.
BUG=Pepper Code Yellow
TEST=ui_tests, --gtest_filter=PPAPITest.*, OutOfProcessPPAPITest.*, PPAPINaClTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113354

[bbudge, 2011-12-02 18:46:00]

http://codereview.chromium.org/8770023/diff/12/ppapi/tests/test_url_loader.cc
File ppapi/tests/test_url_loader.cc (right):

http://codereview.chromium.org/8770023/diff/12/ppapi/tests/test_url_loader.cc...
ppapi/tests/test_url_loader.cc:109: RUN_TEST_FORCEASYNC_AND_NOT(BasicFilePOST,
filter);
The changes in this file are for:
1) Moving comments and test disabling to ppapi_uitest.cc, and
2) Breaking up HTTP request validation into untrusted and trusted cases.

[dmichael (off chromium), 2011-12-02 19:05:26]

http://codereview.chromium.org/8770023/diff/12/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/12/chrome/test/ui/ppapi_uitest.cc...
chrome/test/ui/ppapi_uitest.cc:441: #if !defined(OS_MAC) && !defined(OS_LINUX)
These should just be marked FAILS_ where they fail (unless they hang or
otherwise cause problems when you run them). If they really shouldn't be run,
then the DISABLED_ is preferred, I think. There's a pattern of using MAYBE_ in
front of tests that are run differently on the various platforms that you should
probably use.

http://codereview.chromium.org/8770023/diff/12/chrome/test/ui/ppapi_uitest.cc...
chrome/test/ui/ppapi_uitest.cc:453: //
TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(FileIO_NotAllowMixedReadWrite)
Ditto, FAILS_ if possible, DISABLED_ otherwise.

[bbudge, 2011-12-02 19:36:07]

http://codereview.chromium.org/8770023/diff/12/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/12/chrome/test/ui/ppapi_uitest.cc...
chrome/test/ui/ppapi_uitest.cc:441: #if !defined(OS_MAC) && !defined(OS_LINUX)
On 2011/12/02 19:05:26, dmichael wrote:
> These should just be marked FAILS_ where they fail (unless they hang or
> otherwise cause problems when you run them). If they really shouldn't be run,
> then the DISABLED_ is preferred, I think. There's a pattern of using MAYBE_ in
> front of tests that are run differently on the various platforms that you
should
> probably use.

Done. Thanks for the pro-tip.

http://codereview.chromium.org/8770023/diff/12/chrome/test/ui/ppapi_uitest.cc...
chrome/test/ui/ppapi_uitest.cc:453: //
TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(FileIO_NotAllowMixedReadWrite)
On 2011/12/02 19:05:26, dmichael wrote:
> Ditto, FAILS_ if possible, DISABLED_ otherwise.

Done.

[dmichael (off chromium), 2011-12-02 20:28:33]

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:327:
TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(URLLoader_DISABLED_BasicFileRangePOST)
The DISABLED/FAILS/MAYBE needs to go at the front of the whole test name, e.g.:
DISABLED_URLLoader_BasicFilePOST

http://codereview.chromium.org/8770023/diff/1007/ppapi/tests/test_url_loader.cc
File ppapi/tests/test_url_loader.cc (right):

http://codereview.chromium.org/8770023/diff/1007/ppapi/tests/test_url_loader....
ppapi/tests/test_url_loader.cc:589: // int32_t rv = OpenTrusted(request);
Would it make sense to uncomment this but just mark it FAILS in ppapi_uitest.cc?

[bbudge, 2011-12-02 21:32:10]

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:327:
TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(URLLoader_DISABLED_BasicFileRangePOST)
On 2011/12/02 20:28:33, dmichael wrote:
> The DISABLED/FAILS/MAYBE needs to go at the front of the whole test name,
e.g.:
> DISABLED_URLLoader_BasicFilePOST

That doesn't seem to work. The framework complains it can't find the test. I
think it's because the class name is appended at the front, so it's not like the
TEST_F ones.

http://codereview.chromium.org/8770023/diff/1007/ppapi/tests/test_url_loader.cc
File ppapi/tests/test_url_loader.cc (right):

http://codereview.chromium.org/8770023/diff/1007/ppapi/tests/test_url_loader....
ppapi/tests/test_url_loader.cc:589: // int32_t rv = OpenTrusted(request);
Yes. Done.
On 2011/12/02 20:28:33, dmichael wrote:
> Would it make sense to uncomment this but just mark it FAILS in
ppapi_uitest.cc?

[dmichael (off chromium), 2011-12-02 22:31:42]

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:327:
TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(URLLoader_DISABLED_BasicFileRangePOST)
On 2011/12/02 21:32:10, bbudge1 wrote:
> On 2011/12/02 20:28:33, dmichael wrote:
> > The DISABLED/FAILS/MAYBE needs to go at the front of the whole test name,
> e.g.:
> > DISABLED_URLLoader_BasicFilePOST
> 
> That doesn't seem to work. The framework complains it can't find the test. I
> think it's because the class name is appended at the front, so it's not like
the
> TEST_F ones.
Right...  other places where tests are disabled use the TEST_F style. The way
you have will fail later (or more likely will appear to succeed without actually
running the test, because of how the PPAPI tests work now). For FAILS and FLAKY,
the test should still be run.

I think you need to have DISABLED/FLAKY/FAILS at the beginning or gtest won't
pick it up.

A few ways to fix it:
1) Explicitly use TEST_F
2) Add new macros like FAILS_TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP, which appends
FAILS_ at the right place.
3) Make the macros strip off DISABLED_, FLAKY_, or FAILS_ before passing the
test case name to RunTest.

I think my favorite is 3).

[bbudge, 2011-12-02 22:36:26]

+sky for chrome/tests/ui/ppapi_uitest.cc

[sky, 2011-12-02 22:46:16]

LGTM

[bbudge, 2011-12-06 00:36:04]

Awaiting try jobs.

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/1007/chrome/test/ui/ppapi_uitest....
chrome/test/ui/ppapi_uitest.cc:327:
TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(URLLoader_DISABLED_BasicFileRangePOST)
On 2011/12/02 22:31:42, dmichael wrote:
> On 2011/12/02 21:32:10, bbudge1 wrote:
> > On 2011/12/02 20:28:33, dmichael wrote:
> > > The DISABLED/FAILS/MAYBE needs to go at the front of the whole test name,
> > e.g.:
> > > DISABLED_URLLoader_BasicFilePOST
> > 
> > That doesn't seem to work. The framework complains it can't find the test. I
> > think it's because the class name is appended at the front, so it's not like
> the
> > TEST_F ones.
> Right...  other places where tests are disabled use the TEST_F style. The way
> you have will fail later (or more likely will appear to succeed without
actually
> running the test, because of how the PPAPI tests work now). For FAILS and
FLAKY,
> the test should still be run.
> 
> I think you need to have DISABLED/FLAKY/FAILS at the beginning or gtest won't
> pick it up.
> 
> A few ways to fix it:
> 1) Explicitly use TEST_F
> 2) Add new macros like FAILS_TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP, which appends
> FAILS_ at the right place.
> 3) Make the macros strip off DISABLED_, FLAKY_, or FAILS_ before passing the
> test case name to RunTest.
> 
> I think my favorite is 3).

Done.

[dmichael (off chromium), 2011-12-06 17:34:55]

I think you need to disable one more test for Windows, but otherwise LGTM

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:127: else if (test_name.find("MAYBE_") == 0)
I don't think you need MAYBE_, it should be defined away before we reach this
point.

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:459: #define MAYBE_FileIO_ReadWriteSetLength
FileIO_ReadWriteSetLength
Check your Windows try...  this one looks like it's failing on Windows too :-(

[bbudge, 2011-12-06 17:55:38]

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:127: else if (test_name.find("MAYBE_") == 0)
That's what I thought initially. However without it I get "test not found"
failures and the code definitely gets hit in the debugger. It's probably a
subtlety in C macro expansion.
On 2011/12/06 17:34:55, dmichael wrote:
> I don't think you need MAYBE_, it should be defined away before we reach this
> point.

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:459: #define MAYBE_FileIO_ReadWriteSetLength
FileIO_ReadWriteSetLength
Yep. It's the PPAPINaClTest, so I want to investigate why this new test is
failing. It could be a proxy bug.
On 2011/12/06 17:34:55, dmichael wrote:
> Check your Windows try...  this one looks like it's failing on Windows too :-(

[dmichael (off chromium), 2011-12-06 18:08:18]

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:121: if (test_name.find("FAILS_") == 0)
optional suggestion: I waffled about mentioning this when I thought there were 3
prefixes you needed. If you really need 4, it might make sense to make this a
loop:
const char* const prefixes = { "FLAKY_", "DISABLED_", ...};
for (size_t i = 0; i < sizeof(prefixes)/sizeof(prefixes[0]); ++i)
  if (test_name.find(prefixes[i]) == 0)
    return test_name.substr(strlen(prefixes[i]));
return test_name;

I like that this way you don't hard-code the size of each prefix.

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:230: RunTest(StripPrefixes(#test_name)); \
I bet the MAYBE_ problem is happening because of the same issue described here:
http://stackoverflow.com/questions/1597007/creating-c-macro-with-and-line-tok...
You probably need yet another macro, like:
#define STRING_NAME(test_name) #test_name
and change these to:
RunTest(StripPrefixes(STRING_NAME(test_name))); \

Hope that helps...

[bbudge, 2011-12-06 22:48:15]

Included the NaCl proxy fix FYI. You're an OWNER for that too if you care to
take a look.

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:121: if (test_name.find("FAILS_") == 0)
Hey, thanks! I futzed around with ways to get rid of the magic numbers. This is
good.
On 2011/12/06 18:08:18, dmichael wrote:
> optional suggestion: I waffled about mentioning this when I thought there were
3
> prefixes you needed. If you really need 4, it might make sense to make this a
> loop:
> const char* const prefixes = { "FLAKY_", "DISABLED_", ...};
> for (size_t i = 0; i < sizeof(prefixes)/sizeof(prefixes[0]); ++i)
>   if (test_name.find(prefixes[i]) == 0)
>     return test_name.substr(strlen(prefixes[i]));
> return test_name;
> 
> I like that this way you don't hard-code the size of each prefix.

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:127: else if (test_name.find("MAYBE_") == 0)
On 2011/12/06 17:55:38, bbudge1 wrote:
> That's what I thought initially. However without it I get "test not found"
> failures and the code definitely gets hit in the debugger. It's probably a
> subtlety in C macro expansion.
> On 2011/12/06 17:34:55, dmichael wrote:
> > I don't think you need MAYBE_, it should be defined away before we reach
this
> > point.
> 

Done.

http://codereview.chromium.org/8770023/diff/21003/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:230: RunTest(StripPrefixes(#test_name)); \
On 2011/12/06 18:08:18, dmichael wrote:
> I bet the MAYBE_ problem is happening because of the same issue described
here:
>
http://stackoverflow.com/questions/1597007/creating-c-macro-with-and-line-tok...
> You probably need yet another macro, like:
> #define STRING_NAME(test_name) #test_name
> and change these to:
> RunTest(StripPrefixes(STRING_NAME(test_name))); \
> 
> Hope that helps...

Done.

http://codereview.chromium.org/8770023/diff/29011/ppapi/native_client/src/sha...
File ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_file_io.cc (right):

http://codereview.chromium.org/8770023/diff/29011/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_file_io.cc:162: return
PP_ERROR_FAILED;
Could have been PP_ERROR_BADARGUMENT but I would have had to change the PPAPI
code, so being consistent here.

http://codereview.chromium.org/8770023/diff/29011/ppapi/native_client/src/sha...
ppapi/native_client/src/shared/ppapi_proxy/plugin_ppb_file_io.cc:199: return
PP_ERROR_FAILED;
ditto

[dmichael (off chromium), 2011-12-06 22:55:32]

lgtm

[bbudge, 2011-12-07 00:44:46]

Remove conflicts with HEAD.