[WIP] Add testing helpers

[graphicore]

@felipesanches: I expect we'll have to iterate on this and possible further interfaces a bit. This is to get us started. Here's an example on how to use it:

> from fontbakery.fonts_profile import execute_check_once
> from fontbakery.profiles import gdef
> from fontTools.ttLib.ttFont import TTFont
> ttf = TTFont('/path/to/Family-Regular.ttf')
> execute_check_once(gdef,
    'com.google.fonts/check/gdef_spacing_marks',
    'not a file',
    {'ttFont': ttf})
[(<Status PASS>,
          'Font does not has spacing glyphs in the GDEF mark glyph class.')]

[felipesanches]

this new method as well as self.use_cache do not seem related to the new testing helpers

[graphicore]

Yes, I thought I'd need these when I started this, but since I throw away the check-runner instance after one time use, there's no actual need for a cache reset.

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[felipesanches]

@googlebot I consent.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[felipesanches]

1. I left a couple of code-tests failing in 21b6b94 (the cff profile ones) because I want to discuss it with @graphicore to figure out what went wrong.

fontbakery.checkrunner.SetupError: Profile uses names that are not declared in its namespace: is_cff, is_cff2.

2. There's also the apparent need to append a ":adobefonts" suffix to the checkid of one of the overidden checks. I dislike that. Would be cool to be able to simply pass the profile module and the check id to get_check instead.

3. Finally, there's a commented out attempt to use the get_check helper on the test for com.adobe.fonts/check/family/consistent_upm. It was not working because I was unsure how to setup fonts and ttFonts on get_check (see also commented out attempt there as well).

[graphicore]

I left a couple of code-tests failing in 21b6b94 (the cff profile ones) because I want to discuss it with @graphicore to figure out what went wrong.

fontbakery.checkrunner.SetupError: Profile uses names that are not declared in its namespace: is_cff, is_cff2.

The cff profile uses the conditions is_cff, and is_cff2 but it does not declare (define or import) them. It's the equivalent of a NameError.

This is the cure:

# in profiles/cff.py
profile_imports = [
    ('.shared_conditions', (is_cff', 'is_cff2'))
]

[graphicore]

There's also the apparent need to append a ":adobefonts" suffix to the checkid of one of the overidden checks. I dislike that. Would be cool to be able to simply pass the profile module and the check id to get_check instead.

If it takes the same input but produces different output, it should be a different check-id, because it is another function. It does not matter if the function was created by the override mechanism.

[graphicore]

Since this function is specific to FontsProfile from fonts_profile.py we should hint it in the name or put it somewhere else. e.g. could be called fonts_profile_get_check and still be an import as get_check.

[graphicore]

We could also make this behave roughly like pattern matching in Haskell, i.e. return a different implementation depending on the profile type (isinstance(profile, FontsProfile)) ....

[graphicore]

I'd treat this like a decorator: https://docs.python.org/3.8/library/functools.html#functools.update_wrapper

from functools import update_wrapper
return update_wrapper(_checker, execute_check_once)

[graphicore]

I agree, I'll look into this. Especially because ttFonts is a "derived_iterable" and that's not done with a cache override of conditions.

[graphicore]

Finally, there's a commented out attempt to use the get_check helper on the test for com.adobe.fonts/check/family/consistent_upm. It was not working because I was unsure how to setup fonts and ttFonts on get_check (see also commented out attempt there as well).

ttFonts is a "derived iterable", I'll look into it.

[felipesanches]

thanks ;-)

[graphicore]

@felipesanches why a merge commit? Couldn't you git rebase upstream/master + git push -f ?

[graphicore]

I was actually checking with you on a different topic:

I think the execute_check_once functions should be in the new codetesting.py module.

[felipesanches]

@felipesanches why a merge commit? Couldn't you git rebase upstream/master + git push -f ?

It was done automatically via github UI. I plan to squash and merge when we're good here

[graphicore]

@felipesanches seeing how you use this stuff, I think we may be better of implementing it differently.

[felipesanches]

Here are some thoughts:

When we simply run a check on a given sample font, the current implementation works well computing all the dependencies automatically. It also works well for the cases when we need to change values in tables of a TTF binary, because we naturally can have easy access to the fontTools TTFont object. One shortcoming though is that when we need to modify other things like a field in a METADATA.pb file (or contents of the DESCRIPTION.html file), then it is not trivial how to do it.

It seems that it would be useful if the execution of a check could return a "context" object which we would be able to use to get access to the computed dependencies and eventually modify them before feeding this modified context to a second check execution (for the other test-cases in a given code test)

[felipesanches]

I think such mechanism would be useful to improve code-tests like this one:

https://github.com/googlefonts/fontbakery/blob/master/tests/profiles/googlefonts_test.py#L1420-L1456

[graphicore]

@felipesanches I added a TestingContext class to codetesting and demonstrated it in googlefonts_test.test_check_metadata_nameid_full_name and adobefonts_test.test_check_family_consistent_upm_new_style

One thing it misses is that checks that would be skipped via conditions won't report that way, because the call to check is direct. However, I think the level of control, via the args argument is nice. See the diffs, e.g.:

def test_check_metadata_nameid_full_name():
    """ METADATA.pb font.fullname value matches fullname declared on the name table ? """

    from fontbakery.codetesting import FontsTestingContext
    check = FontsTestingContext(googlefonts_profile,
                      'com.google.fonts/check/metadata/nameid/full_name',
                      TEST_FILE("merriweather/Merriweather-Regular.ttf")
                      )

    assert_PASS(check(), 'with a good font...')

    # here we change the font.fullname on the METADATA.pb
    # to introduce a "mismatch" error condition:
    font_meta = check.args['font_metadata']
    good = font_meta.full_name
    font_meta.full_name = good + "bad-suffix"

    assert_results_contain(check(),
                           FAIL, 'mismatch',
                           'with mismatching fullname values...')

    # and restore the good value prior to the next test case:
    font_meta.full_name = good

    # And here we remove all FULL_FONT_NAME entries
    # in order to get a "lacks-entry" error condition:
    ttFont = check.args['ttFont']
    for i, name in enumerate(ttFont["name"].names):
        if name.nameID == NameID.FULL_FONT_NAME:
            del ttFont["name"].names[i]
    assert_results_contain(check(),
                           FAIL, 'lacks-entry',
                           'when a font lacks FULL_FONT_NAME entries in its name table...')

Maybe we can use the TestingContext instead of the execute_check_once functions in general.

I think however, checking if SKIP events happen via a checks conditions is valuable as well, so we could look into that specifically.

[felipesanches]

thanks, @graphicore! I'll review it and suggest/implement changes later this week. At a quick glance, it looks promising, but I also see a few things I may want to change ;-)

[graphicore]

ttFonts as values won't have any effect.it's defined as a derrived_iterable not as a value.

[graphicore]

Actually, font is neither a value, it's an item from the iterable of fonts and ttfont is not a value as well, it's a condition of font

[felipesanches]

@graphicore, I have been refactoring more code-tests to use the new style. I also changed a bit more the implementation of the TestingContext class. I was not able to remove the fonts, ttFonts values and still keep the tests passing. Could you take a look at it?

I'll be doing all of the port of the remaining code-tests today and hopefully I think I can have it ready for merging very soon.

[felipesanches]

I merged it now because other PRs are piling up and this one includes some linter fixes for those. I'll open another PR to resume work on this.