Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FontVariation unit test #99687

Closed

Conversation

JustinBraben
Copy link

Added unit test cases for FontVariant

The tests load a dynamic font, sets the base font of the FontVariant, sets some properties for the variant, and checks that the original FontFile values are not equal to the changed FontVariant. Let me know if I should add/edit anything else.

Link back to #43440

@JustinBraben JustinBraben requested a review from a team as a code owner November 25, 2024 18:42
@AThousandShips AThousandShips changed the title Font variation unit test Add FontVariation unit test Nov 25, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Nov 25, 2024
@AThousandShips AThousandShips requested a review from a team November 25, 2024 18:46
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotoSans_Regular.woff2 is not a variable font, so it's not suitable for testing most of the FontVariation features.

Tests should check something that can be accidentally broken, the main purpose of testing is to catch potential regressions:

  • Check results of the complex operations against known good values (for variable font, it can be glyph metrics for different configs).
  • Check if input values are sanitized correctly (try using invalid or unusual values).
  • Add an example for a complex set of operations (with comments, so it also acts as low-level documentation).

None of the tests in this PR do anything useful, all are either duplicates of existing tests (loading font and checking it is loaded correctly), or essentially are equivalent to:

int a = 1;
CHECK(a == a);

CHECK(font_file->load_dynamic_font("thirdparty/fonts/NotoSans_Regular.woff2") == OK);

font_variation->set_base_font(font_file);
font_variation->get_base_font()->set_name(font_file->get_font_name());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this supposed to do?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_base_font sets the base font, but not the name of the base font. So this is just setting the name and aforementioned

int a = 1;
CHECK(a == a);

@JustinBraben
Copy link
Author

NotoSans_Regular.woff2 is not a variable font

Ok, does godot have any variable font files in its repo? Or would I need to add one? From what I can see there are no variable fonts in the repo.

And for the rest sure I can try to make a more complex example, check glyphs, input values being sanitized etc

@bruvzg
Copy link
Member

bruvzg commented Nov 26, 2024

Ok, does godot have any variable font files in its repo?

No, I do not think any of the included fonts is variable, but custom resources for test can be added to /tests/data (ideally font should be subsetted to decrease size as much as possible).

@KoBeWi KoBeWi removed this from the 4.x milestone Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants