Bug 94502 - [Chromium] CCMathUtilTest.smallestAngleBetweenVectors unit test failing
Summary: [Chromium] CCMathUtilTest.smallestAngleBetweenVectors unit test failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-20 10:43 PDT by Kenneth Russell
Modified: 2012-08-21 03:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2012-08-20 12:59 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch (2.58 KB, patch)
2012-08-20 13:16 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2012-08-20 10:43:20 PDT
See for example:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/21410/steps/webkit_unit_tests

Please fix ASAP in order to get the bots green.
Comment 1 Shawn Singh 2012-08-20 10:55:43 PDT
looks like this util function and the unit test were added with webkit rev 126018;   +skyostil

In the meantime, I'm trying to repro the failure locally if possible.
Comment 2 Kenneth Russell 2012-08-20 11:53:20 PDT
I'm going to disable this test for the moment until it's diagnosed.
Comment 3 Kenneth Russell 2012-08-20 11:55:34 PDT
Committed r126049: <http://trac.webkit.org/changeset/126049>
Comment 4 Shawn Singh 2012-08-20 12:33:19 PDT
(In reply to comment #3)
> Committed r126049: <http://trac.webkit.org/changeset/126049>

Just checking - was the additional TestExpectations stuff in this CL related to this unit test?
Comment 5 Shawn Singh 2012-08-20 12:59:36 PDT
Created attachment 159496 [details]
Patch

please note that the ".0" suffix seemed to be necessary here, other wise std::min could not be resolved correctly
Comment 6 Kenneth Russell 2012-08-20 13:08:42 PDT
Comment on attachment 159496 [details]
Patch

Per our offline discussion: are you sure that using higher precision for the intermediate results is really the right solution? No matter how many bits of precision are added, floating-point computations are inaccurate. Should the test be updated instead to allow for a small amount of inaccuracy?

BTW, the TestExpectations updates in the other commit were unrelated to this test.
Comment 7 Shawn Singh 2012-08-20 13:16:20 PDT
Created attachment 159503 [details]
Patch

Thanks, good point =)  new patch is even more trivial, and we should follow-up asking Sami whether it's OK that this edge case is tested with less precision.
Comment 8 Kenneth Russell 2012-08-20 13:18:35 PDT
Comment on attachment 159503 [details]
Patch

Looks good. r=me
Comment 9 WebKit Review Bot 2012-08-20 14:50:36 PDT
Comment on attachment 159503 [details]
Patch

Clearing flags on attachment: 159503

Committed r126075: <http://trac.webkit.org/changeset/126075>
Comment 10 WebKit Review Bot 2012-08-20 14:50:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Sami Kyöstilä 2012-08-21 03:12:11 PDT
Thanks for fixing this guys. The landed patch looks fine. I didn't see the failure on my local machine or any Android phones, so I was wrongly assuming that EXPECT_EQ automatically uses an epsilon value for floats.