[Impeller] Write a text-decoration test at the `dart:ui` layer #48101

matanlurey · 2023-11-16T02:06:26Z

It would be nice to turn this into a test-fixture, and create a better local environment.

Here is how to run it locally:

# The test expects you to be PWD in the `src` directory or it crashes.
cd $ENGINE_SRC

out/host_debug_unopt_arm64/flutter_tester \
  --force-multithreading \
  --disable-observatory \
  --use-test-fonts \
  --icu-data-file-path=out/host_debug_unopt_arm64/icudtl.dat \
  --flutter-assets-dir=out/host_debug_unopt_arm64/gen/flutter/lib/ui/assets \
  --disable-asset-fonts \
  --enable-impeller \
  out/host_debug_unopt_arm64/gen/canvas_test.dart.dill

# "See" the output goldens.
open out/host_debug_unopt_arm64/gen/skia_gold_canvas_test.dart

flutter-dashboard · 2023-11-16T02:45:25Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #48101 at sha 8076dda

jonahwilliams · 2023-11-16T02:46:45Z

It looks like the text isn't showing up in the goldens ...

jonahwilliams · 2023-11-16T02:47:04Z

testing/dart/canvas_test.dart

    expect(canvas.getSaveCount(), equals(6));
  });
+
+  test('TextDecoration renders non-solid lines', () async {


This doesn't seem that bad, you made it seem like this would be pretty hard

Easy to write, hard to run :)

testing/dart/canvas_test.dart

matanlurey · 2023-11-16T02:48:23Z

It looks like the text isn't showing up in the goldens ...

Yeah I'm guessing this is a PWD issue similar to what I was seeing locally - test assets are expected to be in a specific place. Let's look at this together tomorrow, unless @dnfield notices I did something wrong.

testing/dart/canvas_test.dart

matanlurey · 2023-11-16T20:48:48Z

New code up, should work both locally and on CI :)

testing/dart/BUILD.gn

testing/dart/canvas_test.dart

dnfield

LGTM with nits

matanlurey · 2023-11-16T20:54:20Z

@jonahwilliams I'm going to mark as auto-submit, but I'm happy to take additional comments post-submit.

jonahwilliams · 2023-11-16T21:00:48Z

LGTM!

flutter-dashboard · 2023-11-16T21:40:35Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48101 at sha 4cea312

matanlurey · 2023-11-16T21:42:43Z

The fonts are still showing up blank on Skia-gold, let me take a look again.

jonahwilliams · 2023-11-16T21:44:11Z

testing/dart/canvas_test.dart

    // However, Flutter opts-in to a Skia feature to render tabs as a single space.
    // See: https://github.com/flutter/flutter/issues/79153
-    final File file = File(path.join('flutter', 'testing', 'resources', 'RobotoSlab-VariableFont_wght.ttf'));
+    final File file = File(path.join(_flutterBuildPath, 'flutter', 'third_party', 'txt', 'assets', 'Roboto-Regular.ttf'));


I would expect this to be in a folder called fixtures.

I think you want "./gen/flutter/impeller/fixtures/assets/Roboto-Regular.ttf"

But my deps is:

deps = [ "//flutter/third_party/txt:txt_fixtures" ]

What does that have to do with the impeller folder?

$ flutter % ls ../out/host_debug_unopt_arm64/gen/flutter/third_party/txt/assets Roboto-Regular.ttf

Hmm....

I'm not sure if this is the right target specifically, but any test fixtures should be built to a target in the out directory that you can look up

Also if the file didn't exist, we could get a FileNotFoundError AFAICT

ahh yeah. Hmmm

I mean, the decorations are showing up right 😆

Yeah, though that's also true if the font doesn't load (hey, I don't make the rules)

I'll take another look in a bit and add some FML_LOG statements and see if there is something obvious going wrong. Probably (?) using a font in a test that doesn't exist should be an error or at the very least an angry log statement.

flutter-dashboard · 2023-11-20T20:59:19Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #48101 at sha 55b28db

matanlurey · 2023-11-20T21:01:12Z

The fonts are still showing up blank on Skia-gold, let me take a look again.

@jonahwilliams @dnfield Turns out this is because the default font color is white on white on Skia gold, and had nothing to do with fonts. I'm now outputting blue and it looks good, PTAL.

jonahwilliams · 2023-11-20T21:03:15Z

lol wonderful. Well now we know

jonahwilliams

LGTM

…yer (flutter/engine#48101)

…138759) flutter/engine@337ab58...7a31543 2023-11-20 [email protected] [Impeller] Write a text-decoration test at the `dart:ui` layer (flutter/engine#48101) 2023-11-20 [email protected] Roll Skia from 69213ba6f68a to 2bcc2e8682a6 (1 revision) (flutter/engine#48238) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

matanlurey added 2 commits November 15, 2023 18:03

Add a golden test for text-decoration that runs on SkiaGold+Impeller.

1732e7e

++

8076dda

matanlurey requested review from dnfield and jonahwilliams November 16, 2023 02:06

flutter-dashboard bot added the will affect goldens label Nov 16, 2023

jonahwilliams reviewed Nov 16, 2023

View reviewed changes

testing/dart/canvas_test.dart Outdated Show resolved Hide resolved

Use FLUTTER_BUILD_DIRECTORY.

d0e4978

matanlurey requested a review from jonahwilliams November 16, 2023 18:09

dnfield reviewed Nov 16, 2023

View reviewed changes

testing/dart/canvas_test.dart Show resolved Hide resolved

matanlurey added 2 commits November 16, 2023 10:26

Tweak.

116b9fd

Add txt_fixtures and use them.

1ab6e0b

matanlurey requested a review from dnfield November 16, 2023 20:48

dnfield reviewed Nov 16, 2023

View reviewed changes

testing/dart/BUILD.gn Outdated Show resolved Hide resolved

dnfield reviewed Nov 16, 2023

View reviewed changes

testing/dart/canvas_test.dart Outdated Show resolved Hide resolved

dnfield approved these changes Nov 16, 2023

View reviewed changes

++

4cea312

matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 16, 2023

jonahwilliams reviewed Nov 16, 2023

View reviewed changes

Tweak font color to see if it's just transparent by default?

55b28db

matanlurey requested review from dnfield and jonahwilliams November 20, 2023 21:01

jonahwilliams approved these changes Nov 20, 2023

View reviewed changes

auto-submit bot merged commit 7a31543 into flutter:main Nov 20, 2023

engine-flutter-autoroll mentioned this pull request Nov 20, 2023

Roll Flutter Engine from 337ab58e81f7 to 7a31543b4630 (2 revisions) flutter/flutter#138759

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 20, 2023

7a31543b4 [Impeller] Write a text-decoration test at the dart:ui la…

ddf386b

…yer (flutter/engine#48101)

[Impeller] Write a text-decoration test at the dart:ui layer #48101

[Impeller] Write a text-decoration test at the dart:ui layer #48101

Uh oh!

Conversation

matanlurey commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Nov 16, 2023

Uh oh!

jonahwilliams commented Nov 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matanlurey commented Nov 16, 2023

Uh oh!

Uh oh!

matanlurey commented Nov 16, 2023

Uh oh!

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

matanlurey commented Nov 16, 2023

Uh oh!

jonahwilliams commented Nov 16, 2023

Uh oh!

flutter-dashboard bot commented Nov 16, 2023

Uh oh!

matanlurey commented Nov 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Nov 20, 2023

Uh oh!

matanlurey commented Nov 20, 2023

Uh oh!

jonahwilliams commented Nov 20, 2023

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Impeller] Write a text-decoration test at the `dart:ui` layer #48101

[Impeller] Write a text-decoration test at the `dart:ui` layer #48101

matanlurey commented Nov 16, 2023 •

edited

Loading