865902 - Port old gfx unit tests

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

I looked through the old unit tests we stopped running and Region & ColorNames where good candidates (useful & easy to port).

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 742078 [details] [diff] [review]
patch

Review of attachment 742078 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming gfx/tests/gtests is a new directory and these files moved over in another patch.

::: gfx/tests/TestColorNames.cpp
@@ +50,5 @@
>  
>      // Check that color lookup by name gets the right rgb value
> +    ASSERT_TRUE(NS_ColorNameToRGB(NS_ConvertASCIItoUTF16(tagName), &rgb)) <<
> +      "can't find '" << tagName.get() << "'";
> +    ASSERT_TRUE((rgb == kColors[index]));

Is it useful to give the index back as extra information?  Perhaps something like:
ASSERT_TRUE((rgb == kColors[index])) << " failed at index " << index << " out of " << ArrayLength(kColorNames);
Other places below with similar things.

::: gfx/tests/TestRegion.cpp
@@ +12,2 @@
>      nsRegion region(r);
> +    EXPECT_TRUE(region.GetLargestRectangle().IsEqualInterior(r));

In the context of keeping the functionality we had before, this makes sense, I was just wondering why it's a non-failing test (EXPECT_, rather than ASSERT_).

Comment 2

[Benoit Girard (:BenWa)]

(In reply to Milan Sreckovic [:milan] from comment #1)
> ::: gfx/tests/TestRegion.cpp
> @@ +12,2 @@
> >      nsRegion region(r);
> > +    EXPECT_TRUE(region.GetLargestRectangle().IsEqualInterior(r));
> 
> In the context of keeping the functionality we had before, this makes sense,
> I was just wondering why it's a non-failing test (EXPECT_, rather than
> ASSERT_).

No particular reason. I'm hoping these tests will rarely fail so it shouldn't matter much.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Port all the tests

I left out the CMS test since we have better tests in qcms that we're using at the moment. Also these tests are from when we still used lcms.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 777585 [details] [diff] [review]
Port all the tests

Review of attachment 777585 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, got lost in the queue.

::: gfx/tests/gtest/TestRect.cpp
@@ +27,5 @@
>  
>    // Make sure the rectangle was properly initialized
> +  EXPECT_TRUE(rect2.x == rect1.x && rect2.y == rect2.y &&
> +      rect2.width == rect2.width && rect2.height == rect2.height) <<
> +    "[2] Make sure the rectangle was properly initialized with copy constructor";

Should we just use EXPECT_TRUE(rect1 == rect2) here?  I guess you were just producing the matching code, fair enough, should probably just stay this way.

@@ +122,5 @@
>    RectType  rect2(rect1);
>  
>    // Test against a rect that's the same as rect1
> +  EXPECT_FALSE(!rect1.Intersects(rect2)) <<
> +    "[1] Test against a rect that's the same as rect1";

All of the tests below make sense as a "conversion to gunit framework", but the result does start looking clumsy with EXCEPT_FALSE(!something) instead of EXCEPT_TRUE(something)...

Comment 5

[Benoit Girard (:BenWa)]

Yes you're right. The test don't provide much value so I almost just removed them since they haven't been running for about 2 years. I think the test could be nicer but considering they haven't regressed I don't see us maintaining them so code cleanliness isn't important here.

Comment 6

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=b759ce60e32e

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch - rebased + fix msvc compile error

https://tbpl.mozilla.org/?tree=Try&rev=4f0db79acbff

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=3bc6c96bcee5

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=c6fb59b3fb9d

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=e2eacd015ad3

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch

Decided to retire the gfxFontSelectionTest.cpp because it's not a great test and has problems which I don't believe are worth the time to fix. I'd rather write a more useful test in another bug, if we want that.

https://tbpl.mozilla.org/?tree=Try&rev=70a8a26e0967

Comment 12

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58aed54902a5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58aed54902a5