From 57d78e3da48634d96c450f5fd685e482e9d5aee2 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 23 Jul 2024 17:18:05 -0700 Subject: [PATCH] [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter/engine#54065) Fixes 2 problems recently uncovered in the DisplayList verbose comparison test mechanism: - The verbose compare methods never asserted a test failure, relying on the caller to do so from their return value - but they also did not prompt the caller to check the return value. So a `[[nodiscard]]` is added to remind test writers that they need to assert on the return value - As a result of the above, some bad tests were recently added to the tree that were failing but did not assert a test failure. Now that the `[[nodiscard]]` is added, they failed to compile and had to have asserts added. A secondary problem is that those non-failing tests were inadvertently cherry-picked from a reverted PR that is being reintroduced in incremental sections so as to avoid large scale golden image failures. The tests depend on parts of that PR that haven't been pulled forward yet (but will soon be) so those tests shouldn't have been added in the first place (and were failing, but not causing a gtest failure because of their missing asserts). They remain here, but their results are reversed to indicate the current state of affairs (they assert that the missing functionality isn't in place yet). Their assertions will be reverted when/as the missing functionality is pulled forward in a more incremental (responsible) way. --- .../display_list/display_list_unittests.cc | 48 ++++++++++++------- .../flutter/testing/display_list_testing.h | 22 +++++---- 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/engine/src/flutter/display_list/display_list_unittests.cc b/engine/src/flutter/display_list/display_list_unittests.cc index 96a16dbd4d9..1b94b6680c7 100644 --- a/engine/src/flutter/display_list/display_list_unittests.cc +++ b/engine/src/flutter/display_list/display_list_unittests.cc @@ -676,12 +676,12 @@ TEST_F(DisplayListTest, SingleOpDisplayListsRecapturedAreEqual) { sk_sp copy = copy_builder.Build(); auto desc = group.op_name + "(variant " + std::to_string(i + 1) + " == copy)"; - DisplayListsEQ_Verbose(dl, copy); + ASSERT_TRUE(DisplayListsEQ_Verbose(dl, copy)); ASSERT_EQ(copy->op_count(false), dl->op_count(false)) << desc; ASSERT_EQ(copy->bytes(false), dl->bytes(false)) << desc; ASSERT_EQ(copy->op_count(true), dl->op_count(true)) << desc; ASSERT_EQ(copy->bytes(true), dl->bytes(true)) << desc; - EXPECT_EQ(copy->total_depth(), dl->total_depth()) << desc; + ASSERT_EQ(copy->total_depth(), dl->total_depth()) << desc; ASSERT_EQ(copy->bounds(), dl->bounds()) << desc; ASSERT_TRUE(copy->Equals(*dl)) << desc; ASSERT_TRUE(dl->Equals(*copy)) << desc; @@ -4955,7 +4955,7 @@ TEST_F(DisplayListTest, DrawRectRRectPromoteToDrawRect) { expected.DrawRect(rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + ASSERT_TRUE(DisplayListsEQ_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, DrawOvalRRectPromoteToDrawOval) { @@ -4969,7 +4969,7 @@ TEST_F(DisplayListTest, DrawOvalRRectPromoteToDrawOval) { expected.DrawOval(rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + ASSERT_TRUE(DisplayListsEQ_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, DrawRectPathPromoteToDrawRect) { @@ -4983,7 +4983,9 @@ TEST_F(DisplayListTest, DrawRectPathPromoteToDrawRect) { expected.DrawRect(rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, DrawOvalPathPromoteToDrawOval) { @@ -4997,7 +4999,9 @@ TEST_F(DisplayListTest, DrawOvalPathPromoteToDrawOval) { expected.DrawOval(rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, DrawRRectPathPromoteToDrawRRect) { @@ -5012,7 +5016,9 @@ TEST_F(DisplayListTest, DrawRRectPathPromoteToDrawRRect) { expected.DrawRRect(rrect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, DrawRectRRectPathPromoteToDrawRect) { @@ -5027,7 +5033,9 @@ TEST_F(DisplayListTest, DrawRectRRectPathPromoteToDrawRect) { expected.DrawRect(rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, DrawOvalRRectPathPromoteToDrawOval) { @@ -5042,7 +5050,9 @@ TEST_F(DisplayListTest, DrawOvalRRectPathPromoteToDrawOval) { expected.DrawOval(rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, ClipRectRRectPromoteToClipRect) { @@ -5060,7 +5070,7 @@ TEST_F(DisplayListTest, ClipRectRRectPromoteToClipRect) { expected.DrawRect(draw_rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + ASSERT_TRUE(DisplayListsEQ_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, ClipOvalRRectPromoteToClipOval) { @@ -5078,7 +5088,9 @@ TEST_F(DisplayListTest, ClipOvalRRectPromoteToClipOval) { expected.DrawRect(draw_rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, ClipRectPathPromoteToClipRect) { @@ -5099,7 +5111,7 @@ TEST_F(DisplayListTest, ClipRectPathPromoteToClipRect) { expected.DrawRect(draw_rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + ASSERT_TRUE(DisplayListsEQ_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, ClipOvalPathPromoteToClipOval) { @@ -5120,7 +5132,9 @@ TEST_F(DisplayListTest, ClipOvalPathPromoteToClipOval) { expected.DrawRect(draw_rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, ClipRRectPathPromoteToClipRRect) { @@ -5142,7 +5156,7 @@ TEST_F(DisplayListTest, ClipRRectPathPromoteToClipRRect) { expected.DrawRect(draw_rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + ASSERT_TRUE(DisplayListsEQ_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, ClipRectInversePathNoPromoteToClipRect) { @@ -5237,7 +5251,7 @@ TEST_F(DisplayListTest, ClipRectRRectPathPromoteToClipRect) { expected.DrawRect(draw_rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + ASSERT_TRUE(DisplayListsEQ_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, ClipOvalRRectPathPromoteToClipOval) { @@ -5259,7 +5273,9 @@ TEST_F(DisplayListTest, ClipOvalRRectPathPromoteToClipOval) { expected.DrawRect(draw_rect, DlPaint()); auto expect_dl = expected.Build(); - DisplayListsEQ_Verbose(dl, expect_dl); + // Support for this will be re-added soon, until then verify that we + // do not promote. + ASSERT_TRUE(DisplayListsNE_Verbose(dl, expect_dl)); } TEST_F(DisplayListTest, BoundedRenderOpsDoNotReportUnbounded) { diff --git a/engine/src/flutter/testing/display_list_testing.h b/engine/src/flutter/testing/display_list_testing.h index 2ccb011dedc..f238360e44e 100644 --- a/engine/src/flutter/testing/display_list_testing.h +++ b/engine/src/flutter/testing/display_list_testing.h @@ -13,20 +13,26 @@ namespace flutter { namespace testing { -bool DisplayListsEQ_Verbose(const DisplayList* a, const DisplayList* b); -bool inline DisplayListsEQ_Verbose(const DisplayList& a, const DisplayList& b) { +[[nodiscard]] bool DisplayListsEQ_Verbose(const DisplayList* a, + const DisplayList* b); +[[nodiscard]] bool inline DisplayListsEQ_Verbose(const DisplayList& a, + const DisplayList& b) { return DisplayListsEQ_Verbose(&a, &b); } -bool inline DisplayListsEQ_Verbose(const sk_sp& a, - const sk_sp& b) { +[[nodiscard]] bool inline DisplayListsEQ_Verbose( + const sk_sp& a, + const sk_sp& b) { return DisplayListsEQ_Verbose(a.get(), b.get()); } -bool DisplayListsNE_Verbose(const DisplayList* a, const DisplayList* b); -bool inline DisplayListsNE_Verbose(const DisplayList& a, const DisplayList& b) { +[[nodiscard]] bool DisplayListsNE_Verbose(const DisplayList* a, + const DisplayList* b); +[[nodiscard]] bool inline DisplayListsNE_Verbose(const DisplayList& a, + const DisplayList& b) { return DisplayListsNE_Verbose(&a, &b); } -bool inline DisplayListsNE_Verbose(const sk_sp& a, - const sk_sp& b) { +[[nodiscard]] bool inline DisplayListsNE_Verbose( + const sk_sp& a, + const sk_sp& b) { return DisplayListsNE_Verbose(a.get(), b.get()); }