Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150663007:
Disable PPAPI*NaCl* tests in the disable_nacl=1 builds (Closed)
|
Created:
6 years, 10 months ago by Alexander Potapenko Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMake PPAPI*NaCl* tests pass in the disable_nacl=1 builds
R=raymes@chromium.org, bbudge, raymes
BUG=327351
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252194
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Messages
Total messages: 18 (0 generated)
Please take a look. The PPAPI*NaCl*.* tests are failing under ThreadSanitizer (which builds with disable_nacl=1), although they shouldn't be ran at all.
+dmichael This seems a bit messy. Is there a nicer way we can do it? Could we override RunTest in PPAPINaClNewlibTest and friends and put the check in there (or something similar)?
On 2014/02/07 02:21:15, raymes wrote: > +dmichael > > This seems a bit messy. Is there a nicer way we can do it? Could we override > RunTest in PPAPINaClNewlibTest and friends and put the check in there (or > something similar)? If the tests silently report success, nobody notices when disable_nacl=1 accidentally slips into the config. I'm also not sure if it's easy to bail out from a gtest with a success error code.
On 2014/02/07 11:49:55, Alexander Potapenko wrote: > On 2014/02/07 02:21:15, raymes wrote: > > +dmichael > > > > This seems a bit messy. Is there a nicer way we can do it? Could we override > > RunTest in PPAPINaClNewlibTest and friends and put the check in there (or > > something similar)? > > If the tests silently report success, nobody notices when disable_nacl=1 > accidentally slips into the config. I don't think they'd notice that the tests are DISABLED either, though. I'm fine with either way, honestly. > I'm also not sure if it's easy to bail out from a gtest with a success error > code.
My preference is to do something in PPAPINaClNewlibTest and friends. The reason being that when we need to disable individual tests or groups of tests we start to get a macro nightmare. We could log something to the console to indicate that NaCl is disabled if we wanted to make it clearer. What do you think? On Sat, Feb 8, 2014 at 10:30 AM, <dmichael@chromium.org> wrote: > On 2014/02/07 11:49:55, Alexander Potapenko wrote: >> >> On 2014/02/07 02:21:15, raymes wrote: >> > +dmichael >> > >> > This seems a bit messy. Is there a nicer way we can do it? Could we >> > override >> > RunTest in PPAPINaClNewlibTest and friends and put the check in there >> > (or >> > something similar)? > > >> If the tests silently report success, nobody notices when disable_nacl=1 >> accidentally slips into the config. > > I don't think they'd notice that the tests are DISABLED either, though. > > I'm fine with either way, honestly. > > >> I'm also not sure if it's easy to bail out from a gtest with a success >> error >> code. > > > > > https://codereview.chromium.org/150663007/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok if I just add some EXIT_IF_NO_NACL macro at the beginning of each test? #if defined(DISABLE_NACL) #define EXIT_IF_NO_NACL return #else #define EXIT_IF_NO_NACL #endif
On 2014/02/10 10:57:31, Alexander Potapenko wrote: > Ok if I just add some EXIT_IF_NO_NACL macro at the beginning of each test? > > #if defined(DISABLE_NACL) > #define EXIT_IF_NO_NACL return > #else > #define EXIT_IF_NO_NACL > #endif RETURN_IF_NO_NACL is a more appropriate name.
On 2014/02/10 11:03:23, Alexander Potapenko wrote: > On 2014/02/10 10:57:31, Alexander Potapenko wrote: > > Ok if I just add some EXIT_IF_NO_NACL macro at the beginning of each test? > > > > #if defined(DISABLE_NACL) > > #define EXIT_IF_NO_NACL return > > #else > > #define EXIT_IF_NO_NACL > > #endif > > RETURN_IF_NO_NACL is a more appropriate name. That sounds reasonable. My fear, though, is that people will add new tests and forget to add the macro. Is it difficult to do the test in the subclass.
On 2014/02/17 01:57:20, raymes wrote: > On 2014/02/10 11:03:23, Alexander Potapenko wrote: > > On 2014/02/10 10:57:31, Alexander Potapenko wrote: > > > Ok if I just add some EXIT_IF_NO_NACL macro at the beginning of each test? > > > > > > #if defined(DISABLE_NACL) > > > #define EXIT_IF_NO_NACL return > > > #else > > > #define EXIT_IF_NO_NACL > > > #endif > > > > RETURN_IF_NO_NACL is a more appropriate name. > > That sounds reasonable. My fear, though, is that people will add new tests and > forget to add the macro. Is it difficult to do the test in the subclass. (btw sorry for the slow reply! this slipped off my radar)
On 2014/02/17 01:57:20, raymes wrote: > On 2014/02/10 11:03:23, Alexander Potapenko wrote: > > On 2014/02/10 10:57:31, Alexander Potapenko wrote: > > > Ok if I just add some EXIT_IF_NO_NACL macro at the beginning of each test? > > > > > > #if defined(DISABLE_NACL) > > > #define EXIT_IF_NO_NACL return > > > #else > > > #define EXIT_IF_NO_NACL > > > #endif > > > > RETURN_IF_NO_NACL is a more appropriate name. > > That sounds reasonable. My fear, though, is that people will add new tests and > forget to add the macro. Is it difficult to do the test in the subclass. Re-uploaded the patch with RETURN_IF_NO_NACL, PTAL. There's already a check that makes the test requiring NaCl fail if it's built with disable_nacl=1 (that's the problem we're trying to solve). So once we've regular testing of disable_nacl=1 for browser_tests (I'm gonna add them to the ThreadSanitizer bot) we'll notice when a new test is added without this macro. It's unclear to me how to make the test return prematurely by changing the test fixture (IIRC gTest doesn't allow this, e.g. this is the reason for not using ASSERT_* macros in the functions called from the test cases), and chances still are that people will add new test fixtures and screw our checks.
On Tue, Feb 18, 2014 at 3:38 AM, <glider@chromium.org> wrote: > On 2014/02/17 01:57:20, raymes wrote: >> >> On 2014/02/10 11:03:23, Alexander Potapenko wrote: >> > On 2014/02/10 10:57:31, Alexander Potapenko wrote: >> > > Ok if I just add some EXIT_IF_NO_NACL macro at the beginning of each >> > > test? >> > > >> > > #if defined(DISABLE_NACL) >> > > #define EXIT_IF_NO_NACL return >> > > #else >> > > #define EXIT_IF_NO_NACL >> > > #endif >> > >> > RETURN_IF_NO_NACL is a more appropriate name. > > >> That sounds reasonable. My fear, though, is that people will add new tests >> and >> forget to add the macro. Is it difficult to do the test in the subclass. > > > Re-uploaded the patch with RETURN_IF_NO_NACL, PTAL. > Cool this looks better. > There's already a check that makes the test requiring NaCl fail if it's > built > with disable_nacl=1 (that's the problem we're trying to solve). > So once we've regular testing of disable_nacl=1 for browser_tests (I'm gonna > add > them to the ThreadSanitizer bot) we'll notice when a new test is added > without > this macro. > It's unclear to me how to make the test return prematurely by changing the > test > fixture (IIRC gTest doesn't allow this, e.g. this is the reason for not > using > ASSERT_* macros in the functions called from the test cases), Have a look in ppapi_test.cc in PPAPITestBase. I think if we make the RunTest* methods virtual and override them in PPAPINaClTest to return if NaCl is disabled it would do the exact same thing as what you have now but we wouldn't have to change each test. What do you think? I think the downside is maybe it's a bit less clear what's going on? If you have a preference toward what you have currently then it's ok with me. > and chances > still > are that people will add new test fixtures and screw our checks. > > https://codereview.chromium.org/150663007/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Have a look in ppapi_test.cc in PPAPITestBase. I think if we make the > RunTest* methods virtual and override them in PPAPINaClTest to return > if NaCl is disabled it would do the exact same thing as what you have > now but we wouldn't have to change each test. What do you think? I > think the downside is maybe it's a bit less clear what's going on? If > you have a preference toward what you have currently then it's ok with > me. Yeah, this is much cleaner (see patch set 3) Right now it prints the "This test always passes under disable_nacl=1." warning twice per test (once in SetUpCommandLine, once in RunTest*) If you prefer, I can omit logging in SetUpCommandLine().
On 2014/02/18 09:15:03, Alexander Potapenko wrote: > > Have a look in ppapi_test.cc in PPAPITestBase. I think if we make the > > RunTest* methods virtual and override them in PPAPINaClTest to return > > if NaCl is disabled it would do the exact same thing as what you have > > now but we wouldn't have to change each test. What do you think? I > > think the downside is maybe it's a bit less clear what's going on? If > > you have a preference toward what you have currently then it's ok with > > me. > > Yeah, this is much cleaner (see patch set 3) > Right now it prints the "This test always passes under disable_nacl=1." warning > twice per test (once in SetUpCommandLine, once in RunTest*) > If you prefer, I can omit logging in SetUpCommandLine(). Thanks this looks much better! I don't mind too much.
https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:349: #if defined(DISABLE_NACL) Consider putting this right at the top of the file above the anonymous namespace. https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:359: RETURN_IF_NO_NACL(); Should this just go right at the top of the function if the test won't run anyway? https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:399: const std::string& test_case) { indentation is slightly off - 2 spaces too many I think https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.h (right): https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.h:52: void RunTest(const std::string& test_case); I think RunTest should be overridden as well.
Done, PTAL https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:349: #if defined(DISABLE_NACL) On 2014/02/18 23:20:08, raymes wrote: > Consider putting this right at the top of the file above the anonymous > namespace. Done. https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:359: RETURN_IF_NO_NACL(); On 2014/02/18 23:20:08, raymes wrote: > Should this just go right at the top of the function if the test won't run > anyway? Done. https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:399: const std::string& test_case) { On 2014/02/18 23:20:08, raymes wrote: > indentation is slightly off - 2 spaces too many I think Done. https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.h (right): https://codereview.chromium.org/150663007/diff/190001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.h:52: void RunTest(const std::string& test_case); On 2014/02/18 23:20:08, raymes wrote: > I think RunTest should be overridden as well. Done.
lgtm! https://codereview.chromium.org/150663007/diff/340001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.h (right): https://codereview.chromium.org/150663007/diff/340001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.h:132: // PPAPITestBase: nit: // PPAPITestBase overrides.
On 2014/02/19 23:47:08, raymes wrote: > lgtm! > > https://codereview.chromium.org/150663007/diff/340001/chrome/test/ppapi/ppapi... > File chrome/test/ppapi/ppapi_test.h (right): > > https://codereview.chromium.org/150663007/diff/340001/chrome/test/ppapi/ppapi... > chrome/test/ppapi/ppapi_test.h:132: // PPAPITestBase: > nit: // PPAPITestBase overrides. Done.
Message was sent while issue was closed.
Committed patchset #5 manually as r252194 (presubmit successful). |
