Skip to content

Use local font in test suite #3

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

Closed
4 of 6 tasks
ehmatthes opened this issue Jul 15, 2023 · 6 comments
Closed
4 of 6 tasks

Use local font in test suite #3

ehmatthes opened this issue Jul 15, 2023 · 6 comments

Comments

@ehmatthes
Copy link
Owner

ehmatthes commented Jul 15, 2023

Tests are brittle due to cross-platform font issues. Running tests using an embedded open font would address this once and for all, and allow a low, reliable test pixel ratio.

  • Add a font file to the test resources.
  • Modify git-sim to support fonts with paths.
  • Verify that generated images are using the installed font.
  • Update reference files.
  • Verify that tests pass on all current contributors' systems.
  • Add a test for a named font, because most tests will use a path to a font.
@ehmatthes
Copy link
Owner Author

ehmatthes commented Jul 15, 2023

Working notes

  • Path to font is being converted from --font=/Users/eric/projects/git-sim/tests/e2e_tests/ProggyClean.ttf to PosixPath('/Users/eric/projects/Users/eric/projects/git-sim/.venv/bin/git-sim/tests/e2e_tests/ProggyClean.ttf'). Fix this. (Fixed; this was my issue in get_cmd_parts().

There is overlap between how git-sim uses font and how Manim uses font. If font is a path, we'd need to use that in a call to m.register_font(). But settings.font is used throughout git-sim in ways that are incompatible with a path to a font file.

The quick patchy demo fix for getting git-sim log to work with a path at the --font arg is to separate out the path and font variables:

    def construct(self):
        from pathlib import Path
        font_path = Path(settings.font)
        if font_path.exists():
            with m.register_font(font_path):
                if not settings.stdout and not settings.output_only_path and not settings.quiet:
                    print(
                        f"{settings.INFO_STRING} {type(self).__name__.lower()}{' --all' if self.all_subcommand else ''}{' -n ' + str(self.n) if self.n_subcommand else ''}"
                    )
                self.font = "ProggyCleanTT"
                self.show_intro()
                ...
  • Trace this back to sort out where we can call m.register_font() so it supports all git-sim commands.
    • Set context and deal with fonts in GitSimBaseCommand.__init__()!
  • Remove diagnostics.
  • Update all git-sim commands to use font_context.
  • Update reference files.
  • Questions:
    • Where should font_context live?
    • What font should be bundled? Consider appearance, and license.

@ehmatthes
Copy link
Owner Author

ehmatthes commented Jul 15, 2023

Working notes

  • This approach requires the fonttools package for end users as well.
  • Learn more about context managers. Is there a way to use register_font() without a context manager? Adding another level indentation to more complex commands such as construct() is not very nice.

@ehmatthes
Copy link
Owner Author

ehmatthes commented Jul 16, 2023

Rework on fresh branch

The fix_font_tests branch is a messy POC. Restart on a fresh branch.

  • Build support for paths in --font.
  • Modify tests to use path to local font file.
  • Test on Windows.
    • Had to install git-dummy first, then install git-sim. Otherwise git-dummy was not on path?
    • Highest pixel ratio was 0.00532. This is well below the current 0.015, and well below the older 0.0075.
  • Sign all commits.
  • Push and PR, with notes.

@ehmatthes
Copy link
Owner Author

ehmatthes commented Jul 16, 2023

PR Notes

  • What is the best place to put get_font_name()? Right now it's in main.py.
  • Placing this work in main.py because that's where settings are being processed.
  • Do we need to return _handle_animations() in commands.py?
  • Is the type for settings.font_context correct?
  • Using a font with obvious differences for development work.
  • Run pip install fonttools.
  • Can pixel ratio be set lower with this approach?
  • Mention script to rename test files.

@ehmatthes
Copy link
Owner Author

ehmatthes commented Jul 16, 2023

Other notes

  • Consider using opencv in compare_images()? It's already a dependency of the project.
  • Don't parametrize commands. Repetition is fine in test suites, and we can see the actual raw_cmd.
    • Do consider a helper function that would take raw_cmd and a part of the output filename to validate.

@ehmatthes
Copy link
Owner Author

ehmatthes commented Jul 17, 2023

Closing, as the main work here has been merged in git-sim 98. Also, open tasks have been moved to git-sim 99.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant