Closed Bug 865902 Opened 7 years ago Closed 6 years ago

Port old gfx unit tests

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 6 obsolete files)

Attached patch patch (obsolete) — Splinter Review
I looked through the old unit tests we stopped running and Region & ColorNames where good candidates (useful & easy to port).
Attachment #742078 - Flags: review?(milan)
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_).
Attachment #742078 - Flags: review?(milan) → review+
Blocks: 870801
Summary: Port Region & ColorNames tests → Port old gfx unit tests
(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.
Attached patch Port all the tests (obsolete) — Splinter Review
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.
Assignee: nobody → bgirard
Attachment #742078 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #777585 - Flags: review?(milan)
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)...
Attachment #777585 - Flags: review?(milan) → review+
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.
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=3bc6c96bcee5
Attachment #783889 - Attachment is obsolete: true
Attachment #784176 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c6fb59b3fb9d
Attachment #784176 - Attachment is obsolete: true
Attachment #784413 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=e2eacd015ad3
Attachment #784413 - Attachment is obsolete: true
Attachment #784548 - Flags: review+
Attached patch patchSplinter Review
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
Attachment #784548 - Attachment is obsolete: true
Attachment #784657 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/58aed54902a5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Duplicate of this bug: 870801
You need to log in before you can comment on or make changes to this bug.