New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758063 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Long OOO (go/where-is-mgiuca)
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 3
Type: Feature



Sign in to add a comment

Make it easier to write RenderTextHarfBuzz tests (wrt. text runs)

Project Member Reported by mgiuca@chromium.org, Aug 23 2017

Issue description

Say you have a test for HarfBuzz run lists:

  const base::string16 mixed = WideToUTF16(
      L"\x05D0\x05D1"
      L"1234"
      L"\x05D2\x05D3"
      L"abc");
  render_text->SetText(mixed);
  render_text->SetDirectionalityMode(DIRECTIONALITY_FORCE_LTR);
  test_api()->EnsureLayout();
  internal::TextRunList* run_list = GetHarfBuzzRunList();

What if I told you we could take expectations like this:

  ASSERT_EQ(4U, run_list->size());
  EXPECT_TRUE(run_list->runs()[0]->is_rtl);
  EXPECT_FALSE(run_list->runs()[1]->is_rtl);
  EXPECT_TRUE(run_list->runs()[2]->is_rtl);
  EXPECT_FALSE(run_list->runs()[3]->is_rtl);
  EXPECT_EQ(2U, run_list->logical_to_visual(0));
  EXPECT_EQ(1U, run_list->logical_to_visual(1));
  EXPECT_EQ(0U, run_list->logical_to_visual(2));
  EXPECT_EQ(3U, run_list->logical_to_visual(3));

and rewrite them like this:

  EXPECT_EQ("[7<-6][2->5][1<-0][8->10]", RunListDebugString(*run_list));

By making a TextRunList into a human-readable string then comparing it in the test, we get:

- More compact tests: Instead of two lines of code for each run, one line for the whole runlist.
- More readable tests: You can actually see the author's intent.
- More readable failure messages: Shows the expected and actual runlist string, rather than individual numerical failures.
- Tests more: Some of these tests *only* test the order of runs (not their ranges). Other tests *only* test the ranges (not the order). The proposed style tests both in one.

The debug string shows the runs in visual order. Each run is enclosed in square brackets, and shows the begin and end inclusive logical character position, with an arrow indicating the direction of the run. Single-character runs just show the character position.

For example, the corresponding run-list for the string "abc+אבג" is expressed as "[0->2][3][6<-4]".

So, that's the plan.
 

Comment 2 by msw@chromium.org, Aug 23 2017

I like it a lot! Thanks for putting some good thought into this, reviewing now.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5bd2a3472d09ca58ce9de2a74ad6cc867115c5d3

commit 5bd2a3472d09ca58ce9de2a74ad6cc867115c5d3
Author: Matt Giuca <mgiuca@chromium.org>
Date: Thu Aug 24 07:07:12 2017

RenderTextHarfBuzzTest: Rewrite run-list tests by comparing strings.

Previously, run-list tests would directly inspect the run-list object,
resulting in long, error-prone and unreadable tests. This adds
functionality to convert a run-list into a human-readable string that
can be compared with a one-line expectation.

This allows tests to be shorter, more readable, check more things and
provide more readable failure messages.

Bug:  758063 
Change-Id: I8746cf8ade460da1ae5b286ea3150f35678aa668
Reviewed-on: https://chromium-review.googlesource.com/627529
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496970}
[modify] https://crrev.com/5bd2a3472d09ca58ce9de2a74ad6cc867115c5d3/ui/gfx/render_text_unittest.cc

Comment 4 by mgiuca@chromium.org, Aug 24 2017

Status: Fixed (was: Started)

Sign in to add a comment