[Impeller] Stand up golden test harness for Impeller DL unittests about flutter HOT 7 CLOSED

I mean we can also add helpers to the DlBuilder that look more like Impeller.

That's what the facade is, they just so happen to match the API of aiks::Canvas. When making a large refactor, the least amount of change to the test code will give you the highest confidence that you didn't break anything. Actually we could probably implement this without changing AiksTests at all as a first step and might be worth considering. The changes I proposed are pretty straightforward though. We don't want a huge PR that completely resurfaces all the tests that are supposed to be verifying that it's correct.

from flutter.

For the existing aiks tests this is going to be a bit tricky. We have a significant investment in testing against the aiks api.

We should try to create a testing shim that will wrap up DlCanvas. That way tests written like this:

TEST_P(AiksTest, CanRenderColoredRect) {
  Canvas canvas;
  Paint paint;
  paint.color = Color::Blue();
  canvas.DrawPath(PathBuilder{}
                      .AddRect(Rect::MakeXYWH(100.0, 100.0, 100.0, 100.0))
                      .TakePath(),
                  paint);
  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

would become something like

TEST_P(AiksTest, CanRenderColoredRect) {
  DisplayListBuilder builder;
  AiksFacade canvas(builder);
  Paint paint;
  paint.color = Color::Blue();
  canvas.DrawPath(PathBuilder{}
                      .AddRect(Rect::MakeXYWH(100.0, 100.0, 100.0, 100.0))
                      .TakePath(),
                  paint);
  ASSERT_TRUE(OpenPlaygroundHere(builder.build()));
}

Then we could little by little delete the facade. That will potentially require creating some new helper functions in the testing namespace or pushing new functionality to the flutter namespace.

For the existing dl_unittests.cc they would almost be exactly the same, since the facade is just an adapter around DisplayListBuilder. The missing gap is just to get from a DisplayList to be rendered in a Playground. Since Playground can already render impeller::Picture, we could use impeller::DlDispatcher to create a picture and conform to the existing Playground code.

from flutter.

What is the AiksFacade providing here? The canvas API is almost identical? Or is it more about the playground implementation itself?

from flutter.

AiksFacade just allows us to migrate all our existing tests over when we delete aiks::Canvas. That way we can separately delete it. It may require a lot of effort to remove, in which case we can spread out its removal as we have time instead of having a huge upfront cost to migrating to display lists. It's probably the only sane way to keep our existing tests.

from flutter.

I mean we can also add helpers to the DlBuilder that look more like Impeller. I think before we commit to the facade I'd try migrating a few tests to see how difficult it will be. Some of the tests that use entity pass internals will need to be enitrely rewritten or deleted and the facade wont help there

from flutter.

Here is a design doc that talks about implementing this (and a tool to help propagate tests from framework usage): https://docs.google.com/document/d/17LiGKlt57R5CZMxcWt9wj9Z7eDihhUaUEg3oeGQAKTc/edit

from flutter.

Done in flutter/engine#52690

from flutter.