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

Use bundled Roboto in all tests #16218

Merged
merged 3 commits into from
Jan 31, 2020
Merged

Use bundled Roboto in all tests #16218

merged 3 commits into from
Jan 31, 2020

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Jan 30, 2020 •
edited
Loading

This is so our screenshots do not change depending on whether tests run locally, Cirrus, or LUCI.

/cc @godofredoc

@yjbanov yjbanov requested a review from nturgut January 30, 2020 01:12
@auto-assign auto-assign bot requested a review from chinmaygarde January 30, 2020 01:12
@yjbanov yjbanov mentioned this pull request Jan 30, 2020
Merged
@chinmaygarde chinmaygarde removed their request for review January 30, 2020 02:32
@mdebbar
Copy link
Contributor

mdebbar commented Jan 30, 2020

What happens if I forget to call setUpStableTestFonts() in my tests? Should we make it easier to spot? Like maybe use an unreadable font by default or something.

@yjbanov
Copy link
Contributor Author

yjbanov commented Jan 30, 2020

What happens if I forget to call setUpStableTestFonts() in my tests? Should we make it easier to spot? Like maybe use an unreadable font by default or something.

We actually have that already:

engine/lib/web_ui/lib/src/engine/dom_renderer.dart

Line 345 in 48eadbb

// These are intentionally outrageous font parameters to make sure that the
. But perhaps the default style is not outrageous enough to spot :)

@mdebbar
Copy link
Contributor

mdebbar commented Jan 30, 2020

Not styles, I meant the font itself. Like if I use Roboto in my test but forget to call setUpStableTestFonts(). What happens?

mdebbar
mdebbar approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

Other than my previous question, this PR looks good!

@yjbanov
Copy link
Contributor Author

yjbanov commented Jan 30, 2020

Not styles, I meant the font itself. Like if I use Roboto in my test but forget to call setUpStableTestFonts(). What happens?

So in the engine our tests use at least three modes:

  • Use the default (i.e. nothing is configured) - typically tests that do not render any text end up in this mode because they do not care about font styles at all.
  • Use Ahem (i.e. all font families are routed to Ahem irrespective of the family specified in the test). Our text measurement tests use this, and maybe some tests that ended up in this mode accidentally. In this mode it is totally OK to specify Roboto and not call setUpStableTestFonts.
  • Use Roboto (after this PR) where visual legibility of text is important to the test. This is when specifying Roboto and calling setUpStableTestFonts is important, otherwise you get OS/environment sensitivity.

We could have a more structured API for running engine tests, just like the framework does with testWidgets, but currently we use plain package:test and tests are free to do whatever they like, and configure the runtime however they like. I'm happy to chat about what we can do here to make things more manageable.

@mdebbar
Copy link
Contributor

mdebbar commented Jan 30, 2020

Thanks for breaking down the use cases. This makes it easier for me to clarify where my concern is coming from.

So in the 3rd case (i.e. Use Roboto (after this PR) where visual legibility of text is important). It's super easy for me to forget to call setUpStableTestFonts() and I won't even notice it. Unless I'm misunderstanding something (which I likely am).

Happy to discuss this when I'm back in the office tomorrow or Monday. It's not a blocker for this PR, so feel free to merge!

@yjbanov
Copy link
Contributor Author

yjbanov commented Jan 30, 2020

@mdebbar Funny enough I did forget to call setUpStableTestFonts in one test (see bcada89). That caused an outright test failure though, so I didn't need to rely on visual inspection to catch it.

@yjbanov yjbanov merged commit 804dca6 into flutter:master Jan 31, 2020
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 31, 2020
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2020
@engine-flutter-autoroll
804dca6 Use bundled Roboto in all tests (flutter/engine#16218)
50acc1c
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 31, 2020
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2020
@engine-flutter-autoroll
804dca6 Use bundled Roboto in all tests (flutter/engine#16218)
01e704f
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jan 31, 2020
@engine-flutter-autoroll
Roll engine 74d07b5..804dca6 (13 commits) (#49873)
aab7bd7 
flutter/engine@74d07b5...804dca6

git log 74d07b5..804dca6 --first-parent --oneline
2020-01-31 yjbanov@google.com Use bundled Roboto in all tests (flutter/engine#16218)
2020-01-31 gw280@google.com Revert "Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} (#15118)" (flutter/engine#16277)
2020-01-31 skia-flutter-autoroll@skia.org Roll src/third_party/skia 36c0521d57de..6305b2f8342a (8 commits) (flutter/engine#16272)
2020-01-31 jason-simmons@users.noreply.github.com Revert "[web] Correct getPositionForOffset for multi-line paragraphs (#16206)" (flutter/engine#16268)
2020-01-30 stuartmorgan@google.com Fix Windows file checks of unicode paths (flutter/engine#16105)
2020-01-30 iska.kaushik@gmail.com [fuchsia] Fix the import for dart_api.h (flutter/engine#16269)
2020-01-30 iska.kaushik@gmail.com [fuchsia] SceneHostBindings are no longer thread locals (flutter/engine#16262)
2020-01-30 gw280@google.com Pass through invoker.resources in fuchsia_test_archive (flutter/engine#16265)
2020-01-30 matthew-carroll@users.noreply.github.com Notify PlatformViewsController within FlutterEngine when a hot restart occurs. (#48518) (flutter/engine#16230)
2020-01-30 skia-flutter-autoroll@skia.org Roll src/third_party/dart fc3af737c759..162d6c5634a0 (209 commits) (flutter/engine#16261)
2020-01-30 skia-flutter-autoroll@skia.org Roll src/third_party/skia d1be5d64f8a7..36c0521d57de (13 commits) (flutter/engine#16260)
2020-01-30 mouad.debbar@gmail.com [web] Correct getPositionForOffset for multi-line paragraphs (flutter/engine#16206)
2020-01-30 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from -mGIA... to 93K0d... (flutter/engine#16257)


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 cbracken@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@yjbanov @NoamDev
Use bundled Roboto in all tests (flutter#16218)
01a7086 
* Use bundled Roboto in all tests
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@NoamDev
Revert "Use bundled Roboto in all tests (flutter#16218)"
7df7401 
This reverts commit 01a7086.
@yjbanov yjbanov deleted the use-roboto-in-tests branch June 22, 2021 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nturgut nturgut nturgut approved these changes

@mdebbar mdebbar mdebbar approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yjbanov @mdebbar @nturgut @googlebot