Enhance the test coverage

[jrycw]

This PR tries to enhance our test coverage.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.61%. Comparing base (7eae1f2) to head (bc868dd).
Report is 37 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #339      +/-   ##
==========================================
+ Coverage   83.51%   85.61%   +2.09%     
==========================================
  Files          41       41              
  Lines        4308     4317       +9     
==========================================
+ Hits         3598     3696      +98     
+ Misses        710      621      -89     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[machow]

Thanks for working on test coverage! This is asking for changes beyond tests, so feel free to punt to me if you want.

I left a comment discussing whether pulling the font data outside of the _get_font_stack() function can...

• cut out some of the recoding of font data in the tests
• up test coverage by removing the big if/elif font name block

Thanks again for taking a look at this. It was really helpful to see _get_font_stack() surfaced, and to think of coverage here!

[machow]

This test seems like it has to recode the data in _get_font_stack(), which is a bit cumbersome. WDYT of...

• pulling the data in _get_font_stack out of the function. For example, _helpers.py could have a top-level variable called FONT_STACKS that is a dictionary.
• since add emoji does a single action---extend the font list---how about a single test case, checking that add emoji did that in one situation?

This will have the effect that a couple of lines in _get_font_stack() can replace all the if/elif font name checks.

Here's an example of what FONT_STACKS my look like in _helpers.py:

FONT_STACKS = {
    "system-ui": ["system-ui", "sans-serif"],
    "transitional": ["Charter", ...],
    ...
}

EMOJI_FONTS =  ["Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"]

This way, the test of add emoji can just check that the flag does something like...

# this could be wrong, but hopefully the gist of it makes sense :o 
# CASE 1:
assert _get_font_stack("system-ui", add_emoji=False) == FONT_STACK["system-ui"]

[jrycw]

@machow , thanks for your review. I'm not sure if the latest commit meets your expectations. I'm happy to hear your feedback if it doesn't.

[machow]

It looks like font_stack_names might not be used here

[jrycw]

Good spot 😅.

[machow]

This looks great, thanks a ton! One tiny improvement could be adding a trailing comma to the shorter lists of fonts. E.g.

• old: ["system-ui", "sans-serif"]
• new: ["system-ui", "sans-serif",]

This way, the list entries will be put onto newlines. But I'm happy to merge as is!

[jrycw]

Thanks for sharing this tip with me. Actually, there are times when I get confused about why the formatter doesn't perform as I expected, but now I understand the reason.

[machow]

Thanks again for this---this kind of stuff is so helpful!