Shaping check (again) by simoncozens · Pull Request #3223 · fonttools/fontbakery

simoncozens · 2021-03-29T13:40:30Z

Description

OK, now we have a configuration file, I think this is a nice clean PR and good to go. The functionality that it adds is very powerful and non-obvious without example JSON files, so I think it may need extra documentation somewhere. Should I add something to the user manual, do we think?

To Do

update CHANGELOG.md
wait for all checks to pass
request a review

felipesanches

thanks, @simoncozens!

It looks good, but there are a few important things I would like to see before merging this. As discussed last Friday, I won't anymore merge PRs with new checks lacking rationale / request URL / good code-tests.

Your code-tests are good, but lack keywords, which makes them error-prone.
And the checks currently lack rationale and request metadata fields.

felipesanches · 2021-03-30T16:04:14Z

+# Do shaping results match expectations?
+
+
+@check(id="com.google.fonts/check/shaping/regression")


please add rationale using this as a template:

@check( id = 'com.google.fonts/check/shaping/regression', rationale = """ Lalalalalalalalala lalala lalala lala la lalala alala. Lelele lelelele lelelele lelelel eleleele. """, misc_metadata = { 'request': 'https://github.com/googlefonts/fontbakery/pull/3223' } )

felipesanches · 2021-03-30T16:06:24Z

+        msg = f"{shaping_text} produced '{forbidden}'"
+        report_items.append(create_report_item(vharfbuzz, msg, text=shaping_text, buf1=buf))
+
+    yield FAIL, (msg + ".\n" + "\n".join(report_items))


please give it a keyword by using the Message class

felipesanches · 2021-03-30T16:06:34Z

+# Are there any collisions?
+
+
+@check(id="com.google.fonts/check/shaping/collides")


please add rationale & misc_metadata['request'] = Pull request URL

felipesanches · 2021-03-30T16:07:05Z

+    yield FAIL, (msg + ".\n" + "\n".join(report_items))
+
+
+# Are there any collisions?


cleanup this loose comment. Maybe this fits as part of the description/rationale

felipesanches · 2021-03-30T16:09:44Z

+    return params
+
+
+# This is a very generic "do something with shaping" test runner.


check runner! In FontBakery lingo we use the word "test" to refer to automated code-tests while "check" means "font checking routines"

felipesanches · 2021-03-30T16:10:42Z

+
+# This is a very generic "do something with shaping" test runner.
+# It'll be given concrete meaning later.
+def run_a_set_of_tests(


run_a_set_of_checks (or even better run_a_set_of_shaping_checks?)

Hm. I'm not sure I agree. I understand that within fontbakery "tests" are what we do to the fontbakery codebase and "checks" are what we run. But this profile defines three checks (com.google.fonts/check/shaping/regression, etc.) and each of the checks runs a test suite. It's not true that the three checks run more checks. We're selling this as test-driven-development, which is a well-established terminology, so I think I want to keep the "test" wording here.

ok. I see. I think it is a bit confusing. But I see your point. I think I'll accept this for now and think better, maybe reevaluate later.

Note: for some reason I have reviewed this PR from bottom to top, so you will see another message from me below that was written before I saw your comment here.

at least add shaping to the function name. So... for now, run_a_set_of_shaping_tests and we may later better discuss terminology

felipesanches · 2021-03-30T16:12:10Z

+        try:
+            shaping_input_doc = json.loads(shaping_file.read_text())
+        except Exception as e:
+            yield FAIL, (f"{shaping_file}: Invalid JSON: {e}.")


all non-PASS log-messages should have keywords so that we can unambiguously exercise them on code-tests. Please use the Message class for that.

felipesanches · 2021-03-30T16:13:15Z

+    yield FAIL, (header + "\n" + "\n".join(report_items))
+
+
+# Are there any naughty glyphs?


cleanup this. Move into description/rationale

felipesanches · 2021-03-30T16:18:39Z

+
+        ttFont = TTFont(TEST_FILE("slabo/Slabo13px.ttf"))
+        assert_results_contain(
+            check(wrap_args(config, ttFont)), FAIL, None, "Slabo: A!=664,V!=691"


here's an exemple of a place where we should be refering to a specific FAIL message by its keyword. By not having a keyword and using None we risk falsely detecting this as a successful code-test run while the actual FAIL message emitted by the check being tested may come from some other code-path.

Please always use keywords!

simoncozens · 2021-03-30T17:19:28Z

Thank you, this is super helpful! I’m going to (in a separate PR) clean up all the other places where bare strings are used instead of messages because I’m sure other people will cut-and-paste checks just like I did.

felipesanches · 2021-03-30T18:31:31Z

I’m going to (in a separate PR) clean up all the other places where bare strings are used instead of messages

Thanks! I'll be immensely grateful for that!
I've been gradually trying to make this kind of change from time to time on portions of the code (mostly when merging new checks), but have never (yet) applied that policy update to the entire backlog of pre-existing checks.

felipesanches · 2021-03-30T18:35:22Z

I’m going to (in a separate PR) clean up all the other places where bare strings are used instead of messages

when doing so, please follow this code-style, so that I don't have to do it afterwards:

    if "<html>" in description or "</html>" in description:
        yield FAIL,\
              Message("html-tag",
                      f"{descfile} should not have an <html> tag,"
                      f" since it should only be a snippet that will"
                      f" later be included in the Google Fonts"
                      f" font family specimen webpage.")

The previous version worked nicely when generating standalone HTML reports, but needed tweaking to play well within the context of a bigger fontbakery report.

felipesanches · 2021-03-31T13:22:08Z

+        the fontbakery configuration file. For more information about write
+        test files and how to configure fontbakery to read the test suites,
+        see https://simoncozens.github.io/tdd-for-otl/
+    """,


Each paragraph in a rationale should be a single line. Do not perform manual line breaking. Fontbakery itself will do the text-layout math to better display the text block on each od the specialized reporters.

So here it should be:

@check( id = 'com.google.fonts/check/shaping/collides', rationale = """ Fonts with complex layout rules can benefit from regression checks to ensure that the rules are behaving as designed. This checks runs a test suite and reports instances where the glyphs collide in unexpected ways. Test suites should be written by the font engineer and referenced in the fontbakery configuration file. For more information about writing test files and how to configure fontbakery to read the test suites, see https://simoncozens.github.io/tdd-for-otl/ """,

I'm a bit annoyed by the word "test" and the expression "test suite" used here though... I understand it, but I'd rather use some other expression to avoid confusion keeping in mind the arbitrary definition of "tests" versus "checks" adopted in fontbakery jargon.

the same same single-line per rationale paragraph style should be adopted in all checks included in this PR

felipesanches · 2021-03-31T13:28:02Z

+
+# This is a very generic "do something with shaping" test runner.
+# It'll be given concrete meaning later.
+def run_a_set_of_tests(


ok. I see. I think it is a bit confusing. But I see your point. I think I'll accept this for now and think better, maybe reevaluate later.

Note: for some reason I have reviewed this PR from bottom to top, so you will see another message from me below that was written before I saw your comment here.

felipesanches · 2021-03-31T13:32:04Z

+    """,
+    misc_metadata={"request": "https://github.com/googlefonts/fontbakery/pull/3223"},
+)
+@check(id="com.google.fonts/check/shaping/forbidden")


Is there a second @check here? Or is it something weird about the GitHub PR review UI? I'm a bit confused. It looks like you forgot to remove the original @checkline when adding the rationale in this line.

felipesanches · 2021-03-31T13:33:54Z

+
+# This is a very generic "do something with shaping" test runner.
+# It'll be given concrete meaning later.
+def run_a_set_of_tests(


at least add shaping to the function name. So... for now, run_a_set_of_shaping_tests and we may later better discuss terminology

chrissimpkins · 2021-03-31T14:09:19Z

        'beziers',
-        'cmarkgfm'
+        'cmarkgfm',
+        'vharfbuzz',


Thoughts about making the new dependencies extras_requires packages so that they are optional? Maybe under a title like [shaping]? How well does vharfbuzz and anything else new that might be compiled here play with cross-platform installations of fontbakery?

@chrissimpkins, I've seen your comments a few seconds after merging the PR, sorry.

If we do this, then we must ensure that the checks auto-skip (instead of resulting in an ERROR) if those modules cannot be imported successfully. But, if possible, I'd like to keep this in the default installation, otherwise I fear that a huge number of people will simply never run these checks.

My plan was to cut a new fontbakery release after merging this PR. But your comment makes me want to better evaluate the cross-platform support of these new dependencies before committing to a new release.

felipesanches · 2021-03-31T14:11:51Z

thanks, @simoncozens

chrissimpkins · 2021-03-31T14:13:35Z

The functionality that it adds is very powerful and non-obvious without example JSON files, so I think it may need extra documentation somewhere.

Definitely agree Simon. I'd be interested in helping with this effort. In addition to the documentation, what is your thought about adding a simple set of templates that a user can grab and use to begin building out their shaping test suite?

simoncozens · 2021-03-31T14:22:21Z

Yes, I was discussing this with Ben today. I think we found that because of differing glyph name conventions in different fonts (and different approaches to shaping), it's not really possible to provide a generic set of templates that users can just pick up and run with. However, it would be possible to give of set of inputs that they might want to use to provide coverage of major shaping requirements of script, and also to provide a little python script that they can run when they are happy with a particular rendering, which loads the test suite so far and then rewrites the expectations for that rendering. i.e., we give them:

{
	"tests": [
		{
			"input": "ြကွှ",
		},
		{
			"input": "ကြှ",
		},
		{
			"input": "ကြ့",
		},
...
}

and if they like the output of ကြှ, they run fontbakery-fix-shaping-expectations ကြှ and it rewrites the JSON to

{
	"tests": [
		{
			"input": "ြကွှ",
		},
		{
			"input": "ကြှ",
			"expectation": "medialRa-myanmar.w2=0+216|ka-myanmar=0+1040|medialHa-myanmar.short=0@-486,0+0"
		},
		{
			"input": "ကြ့",
		},
...
}

simoncozens · 2021-03-31T14:24:16Z

Also note that I have a blog post at https://simoncozens.github.io/tdd-for-otl/ which is referenced in the rationale. But yes, we should provide some useful shaping files. I'll work on another PR.

chrissimpkins · 2021-03-31T14:25:07Z

it would be possible to give of set of inputs that they might want to use to provide coverage of major shaping requirements of script

This would be incredibly valuable.

My suggestion was much simpler though. Rather than providing any input text runs, do we want to provide the formatted JSON template that allows a user to grab the config file and begin entering text runs on their own? This seems like the initial hurdle to use.

chrissimpkins · 2021-03-31T16:09:07Z

Also note that I have a blog post at https://simoncozens.github.io/tdd-for-otl/ which is referenced in the rationale. But yes, we should provide some useful shaping files. I'll work on another PR.

This will be a helpful article for the community. It would be good to see mention of Nikolaus's contribution to the shaping test idea and implementation.

simoncozens · 2021-04-01T10:15:00Z

I've added a reference to Nikolaus' version.

simoncozens added 13 commits March 27, 2021 07:10

Shaping profile, WIP

d33e439

Various improvements

91a80d6

Slight visual improvement

7450430

Write everything to the reporter, not a separate file

9a096cf

Flake8 issues

2c89a49

Make shaping test read its tests files from config

c91c31b

Add shaping check to universal profile

0b2d672

Add requirements for shaping check

4e72b08

Add tests for shaping check

8c11011

Add CHANGELOG entry

2243a51

All for a little equals sign

d271fb1

Duplicate requirements into the *other* requirements list. :-/

a251a01

Collidoscope had a missing requirement, fix it

1f9b484

felipesanches requested changes Mar 30, 2021

View reviewed changes

simoncozens added 5 commits March 31, 2021 10:14

Unfortunately the GFM version eats my <style> tags. :-/

d81e6bb

Improve presentation

6c6cf7a

The previous version worked nicely when generating standalone HTML reports, but needed tweaking to play well within the context of a bigger fontbakery report.

Don’t die horribly if we don’t have the glyphs expected

049f502

Fix rationales and messages

d2727d5

Follow test naming convention

9f991cc

felipesanches requested changes Mar 31, 2021

View reviewed changes

Rename "tests"->"shaping tests" and fix rationale linebreaks

0d85ec8

chrissimpkins reviewed Mar 31, 2021

View reviewed changes

felipesanches merged commit 4d680af into fonttools:main Mar 31, 2021

		# Do shaping results match expectations?


		@check(id="com.google.fonts/check/shaping/regression")

		# Are there any collisions?


		@check(id="com.google.fonts/check/shaping/collides")

		yield FAIL, (msg + ".\n" + "\n".join(report_items))


		# Are there any collisions?

		return params


		# This is a very generic "do something with shaping" test runner.

		yield FAIL, (header + "\n" + "\n".join(report_items))


		# Are there any naughty glyphs?

Conversation

simoncozens commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

To Do

Uh oh!

felipesanches left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

felipesanches Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simoncozens commented Mar 30, 2021

Uh oh!

felipesanches commented Mar 30, 2021

Uh oh!

felipesanches commented Mar 30, 2021

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!

chrissimpkins Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

felipesanches commented Mar 31, 2021

Uh oh!

chrissimpkins commented Mar 31, 2021

Uh oh!

simoncozens commented Mar 31, 2021

Uh oh!

simoncozens commented Mar 31, 2021

Uh oh!

chrissimpkins commented Mar 31, 2021

Uh oh!

chrissimpkins commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simoncozens commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

simoncozens commented Mar 29, 2021 •

edited

Loading

felipesanches Mar 30, 2021 •

edited

Loading

chrissimpkins Mar 31, 2021 •

edited

Loading

chrissimpkins commented Mar 31, 2021 •

edited

Loading