Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Impeller] test if aiks golden test can be ported to DL. #52711

Closed
wants to merge 3 commits into from
Closed

[Impeller] test if aiks golden test can be ported to DL. #52711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbb13ec
[Impeller] test if aiks golden test can be ported to DL.
jonahwilliams May 10, 2024
34eb561
rename golden tests to aiks test for golden continuity.
jonahwilliams May 10, 2024
cb28c8a
rename in golden harvester.
jonahwilliams May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions display_list/dl_color.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ struct DlColor {
static constexpr DlColor kDarkGrey() {return DlColor(0xFF3F3F3F);};
static constexpr DlColor kMidGrey() {return DlColor(0xFF808080);};
static constexpr DlColor kLightGrey() {return DlColor(0xFFC0C0C0);};
static constexpr DlColor kAliceBlue() {return DlColor(0xFFF0F8FF);};
// clang-format on

constexpr bool isOpaque() const { return getAlpha() == 0xFF; }
Expand Down
25 changes: 0 additions & 25 deletions impeller/aiks/aiks_path_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,6 @@
namespace impeller {
namespace testing {

TEST_P(AiksTest, RotateColorFilteredPath) {
Canvas canvas;
canvas.Concat(Matrix::MakeTranslation({300, 300}));
canvas.Concat(Matrix::MakeRotationZ(Radians(kPiOver2)));
auto arrow_stem =
PathBuilder{}.MoveTo({120, 190}).LineTo({120, 50}).TakePath();
auto arrow_head = PathBuilder{}
.MoveTo({50, 120})
.LineTo({120, 190})
.LineTo({190, 120})
.TakePath();
auto paint = Paint{
.stroke_width = 15.0,
.stroke_cap = Cap::kRound,
.stroke_join = Join::kRound,
.style = Paint::Style::kStroke,
.color_filter =
ColorFilter::MakeBlend(BlendMode::kSourceIn, Color::AliceBlue()),
};

canvas.DrawPath(arrow_stem, paint);
canvas.DrawPath(arrow_head, paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderStrokes) {
Canvas canvas;
Paint paint;
Expand Down
16 changes: 0 additions & 16 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,6 @@ TEST_P(AiksTest, CanRenderColoredRect) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderImage) {
Canvas canvas;
Paint paint;
auto image = std::make_shared<Image>(CreateTextureForFixture("kalimba.jpg"));
paint.color = Color::Red();
canvas.DrawImage(image, Point::MakeXY(100.0, 100.0), paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanRenderInvertedImageWithColorFilter) {
Canvas canvas;
Paint paint;
Expand Down Expand Up @@ -921,13 +912,6 @@ TEST_P(AiksTest, TextRotated) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanDrawPaint) {
Canvas canvas;
canvas.Scale(Vector2(0.2, 0.2));
canvas.DrawPaint({.color = Color::MediumTurquoise()});
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanDrawPaintMultipleTimes) {
Canvas canvas;
canvas.Scale(Vector2(0.2, 0.2));
Expand Down
33 changes: 32 additions & 1 deletion impeller/display_list/dl_golden_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@

#include "impeller/display_list/dl_golden_unittests.h"

#include "display_list/dl_blend_mode.h"
#include "display_list/dl_color.h"
#include "display_list/dl_paint.h"
#include "display_list/effects/dl_color_filter.h"
#include "flutter/display_list/dl_builder.h"
#include "flutter/testing/testing.h"
#include "gtest/gtest.h"
#include "impeller/geometry/constants.h"

namespace flutter {
namespace testing {
Expand Down Expand Up @@ -48,5 +52,32 @@ TEST_P(DlGoldenTest, CanRenderImage) {
ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

TEST_P(DlGoldenTest, RotateColorFilteredPath) {
Copy link
Member

@gaaclarke gaaclarke May 10, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'd do is create another file called dl_aiks_golden_unittests.cc. It will match the same structure as DlGoldenTest but have the name AiksTests. Then you can move tests from aiks to there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like I can rename it like that. I'm going to have the golden harvester do a string replace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What problem did you run into? It should work, I've already moved tests that way when I split up aiks_unittests.cc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not the test names, its suite names. It needs to be AiksTest otherwise the goldens are invalidated. We can't have multiple AiksTests set up or we trip assertions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not the test names, its suite names. It needs to be AiksTest otherwise the goldens are invalidated. We can't have multiple AiksTests set up or we trip assertions.

We already have multiple AiksTests, checkout aiks_unittests.cc and aiks_blend_unittests.cc. On monday let's sync up and look at it together. You are probably just running into the the problem that this one is in the flutter namespace and the others are in the impeller namespace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you know what. That's probably not going to work because AiksTests doesn't have access to OpenPlaygroundHere(display_list). We would have to copy that over, which isn't so bad.

If we want to go down the route you are considering in the latest patch I think the renaming of the suite should happen in the same file as the tests. The setUp method could override the name.

Let's meet up later today to discuss the options. We have to consider too what do we want to do with newly written tests, not just ones we've migrated from aiks.

DisplayListBuilder builder;
builder.Transform(SkMatrix::Translate(300, 300));
builder.Transform(SkMatrix::RotateDeg(impeller::kPiOver2));

SkPath arrow_stem;
SkPath arrow_head;

arrow_stem.moveTo({120, 190}).lineTo({120, 50});
arrow_head.moveTo({50, 120}).lineTo({120, 190}).lineTo({190, 120});

auto filter =
DlBlendColorFilter::Make(DlColor::kAliceBlue(), DlBlendMode::kSrcIn);

DlPaint paint;
paint.setStrokeMiter(15.0);
paint.setStrokeCap(DlStrokeCap::kRound);
paint.setStrokeJoin(DlStrokeJoin::kRound);
paint.setDrawStyle(DlDrawStyle::kStroke);
paint.setColorFilter(filter);

builder.DrawPath(arrow_stem, paint);
builder.DrawPath(arrow_head, paint);

ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
}

} // namespace testing
} // namespace flutter
3 changes: 3 additions & 0 deletions impeller/golden_tests/golden_playground_test_mac.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ namespace {
std::string GetTestName() {
std::string suite_name =
::testing::UnitTest::GetInstance()->current_test_suite()->name();
if (suite_name == "DlGoldenTest") {
suite_name = "AiksTest";
}
std::string test_name =
::testing::UnitTest::GetInstance()->current_test_info()->name();
std::stringstream ss;
Expand Down
3 changes: 3 additions & 0 deletions impeller/golden_tests/golden_tests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ namespace {
std::string GetTestName() {
std::string suite_name =
::testing::UnitTest::GetInstance()->current_test_suite()->name();
if (suite_name == "DlGoldenTest") {
suite_name = "AiksTest";
}
std::string test_name =
::testing::UnitTest::GetInstance()->current_test_info()->name();
std::stringstream ss;
Expand Down
8 changes: 1 addition & 7 deletions testing/impeller_golden_tests_output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,10 +773,4 @@ impeller_Play_AiksTest_VerticesGeometryUVPositionDataWithTranslate_OpenGLES.png
impeller_Play_AiksTest_VerticesGeometryUVPositionDataWithTranslate_Vulkan.png
impeller_Play_AiksTest_VerticesGeometryUVPositionData_Metal.png
impeller_Play_AiksTest_VerticesGeometryUVPositionData_OpenGLES.png
impeller_Play_AiksTest_VerticesGeometryUVPositionData_Vulkan.png
impeller_Play_DlGoldenTest_CanDrawPaint_Metal.png
impeller_Play_DlGoldenTest_CanDrawPaint_OpenGLES.png
impeller_Play_DlGoldenTest_CanDrawPaint_Vulkan.png
impeller_Play_DlGoldenTest_CanRenderImage_Metal.png
impeller_Play_DlGoldenTest_CanRenderImage_OpenGLES.png
impeller_Play_DlGoldenTest_CanRenderImage_Vulkan.png
impeller_Play_AiksTest_VerticesGeometryUVPositionData_Vulkan.png