Bug 73997 - [Chromium] Exclude the four RenderTableCellDeathTest death tests for Android
Summary: [Chromium] Exclude the four RenderTableCellDeathTest death tests for Android
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peter Beverloo
URL:
Keywords:
Depends on:
Blocks: 66689
  Show dependency treegraph
 
Reported: 2011-12-07 06:05 PST by Peter Beverloo
Modified: 2011-12-08 12:16 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2011-12-07 06:08 PST, Peter Beverloo
no flags Details | Formatted Diff | Diff
Updated patch (1.71 KB, patch)
2011-12-08 07:50 PST, Peter Beverloo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Beverloo 2011-12-07 06:05:06 PST
[Chromium] Exclude the four RenderTableCellDeathTest death tests for Android
Comment 1 Peter Beverloo 2011-12-07 06:08:14 PST
Created attachment 118201 [details]
Patch
Comment 2 Peter Beverloo 2011-12-07 06:11:14 PST
Julien, could you have a look? Alternative would be to exclude the file from the build, but having some (in this case, 4 out of 8) tests is still better than none.
Comment 3 Julien Chaffraix 2011-12-07 08:10:03 PST
Comment on attachment 118201 [details]
Patch

Those tests are more FYI than anything however disabling tests like that would need more explanations. Why is ASSERT_DEATH not available? Not implemented? Not possible to support on the Android platform? Do we have something tracking re-adding them? (skipping / disabling tests is easy but then we forget about them and that's what I fear here)
Comment 4 Peter Beverloo 2011-12-07 10:06:52 PST
Google Test has not been updated to recognize and support Android yet, and this is a requirement in order to enable the ASSERT_DEATH (or EXPECT_DEATH) macros as these are guarded per platform.

http://code.google.com/p/googletest/source/browse/trunk/include/gtest/internal/gtest-port.h#545

Would it work if I file a bug here to track this, and refer to that in the comment?
Comment 5 Julien Chaffraix 2011-12-07 10:13:56 PST
(In reply to comment #4)
> Google Test has not been updated to recognize and support Android yet, and this is a requirement in order to enable the ASSERT_DEATH (or EXPECT_DEATH) macros as these are guarded per platform.
> 
> http://code.google.com/p/googletest/source/browse/trunk/include/gtest/internal/gtest-port.h#545
> 
> Would it work if I file a bug here to track this, and refer to that in the comment?

Totally, it's just to make sure that we don't forget about the tests. Would be nice to refer to a Google Test bug too so that we can easily determine when to toggle the test on.
Comment 6 Peter Beverloo 2011-12-08 07:50:45 PST
Created attachment 118390 [details]
Updated patch

(In reply to comment #5)
> Totally, it's just to make sure that we don't forget about the tests. Would be nice to refer to a Google Test bug too so that we can easily determine when to toggle the test on.

I filed a WebKit Bug 74089. Upstream CL is http://codereview.appspot.com/5464048, but it may take quite a while before the updated GTest becomes available in Source/WebKit/chromium/, while this currently is a compile error.
Comment 7 Julien Chaffraix 2011-12-08 10:33:45 PST
Comment on attachment 118390 [details]
Updated patch

Thanks for filing the different bugs!
Comment 8 WebKit Review Bot 2011-12-08 12:16:45 PST
Comment on attachment 118390 [details]
Updated patch

Clearing flags on attachment: 118390

Committed r102362: <http://trac.webkit.org/changeset/102362>
Comment 9 WebKit Review Bot 2011-12-08 12:16:50 PST
All reviewed patches have been landed.  Closing bug.