Added getter tests for dynamic fonts

[matorin57]

Added getter tests for Font when loading a dynamic font file as listed in #43440
Used a free dynamic font file found at https://www.dafont.com for testing.

For the size based values I chose a small and large sized and used what was already outputted, to act as regression.

[Calinou]

Used a free dynamic font file found at dafont.com for testing.

What is the font licensed under? I'd suggest copying the smallest font from this repository's thirdparty/fonts/ folder, so that we are 100% sure the font is freely-licensed. (In other words, we should avoid "personal use only" licenses.)

[matorin57]

Used a free dynamic font file found at dafont.com for testing.

What is the font licensed under? I'd suggest copying the smallest font from this repository's thirdparty/fonts/ folder, so that we are 100% sure the font is freely-licensed. (In other words, we should avoid "personal use only" licenses.)

I switched the test font to a font from thirdparty/fonts/

[Calinou]

I switched the test font to a font from thirdparty/fonts/

Remember to remove test.ttf and update references in the test code. You will also need to update the expected return values in the test.

[matorin57]

Thanks for catching that. Appeared to be some git mistakes. Should be cleared up now :).

[matorin57]

Quick question. Are the docs correct in that for PRs we want to amend previous commits instead of pushing new ones? Just clarifying so I know the expected flow.

[bruvzg]

The same font is already included as the editor font, so there's no reason to do it again, just use thirdparty/fonts/NotoSansHebrew_Regular.woff2.

[bruvzg]

Or load it directly from the editor resources font->set_data_ptr(_font_NotoSansHebrew_Regular, _font_NotoSansHebrew_Regular_size);, see tests/servers/test_text_server.h and editor/editor_fonts.cpp → load_internal_font for examples.

[AThousandShips]

Quick question. Are the docs correct in that for PRs we want to amend previous commits instead of pushing new ones?

The docs are correct, you can make multiple commits if you need to track changes, but this is a small change so no reason, you will have to squash before it can be merged anyway if it gets approved

[matorin57]

Addressed all feedback

[matorin57]

A question on process, do I need to do anything else to finish this PR?

[Calinou]

A question on process, do I need to do anything else to finish this PR?

I don't think so, we only need someone to perform a final review and it should be good to merge.

[kosta777]

Hi, I'd like to extend Font tests by adding testing for FontFile as per #43440 but it might make sense to have this merged first so that I can extend it.

This PR has been open for a long time. Could we do something about it? Is there a protocol around pinging somebody/increasing visibility somehow when we are waiting a long time for a review?

Alternatively, I could write FontFile unit tests in a separate file, but considering that Font and FontFile (and FontVariation for that matter) declarations are bundled together, maybe the tests for them should also be in the same place?

[kosta777]

Should it really be [String] here? If this is the tag that's used to filter which tests to run when running test suites, it would imo make more sense that it's something like [Font]. And same for other places in the file where we have [String].

[Calinou]

This is either a copy-pasting mistake or an oversight, so it should be [Font] indeed.

[Calinou]

Tested locally (rebased on top of master 6c57928), it works as expected.

All instances of [String] should be replaced with [Font] before merging.