Skip to content

feat(lib): propagate manim render exit code #545

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

Merged
merged 4 commits into from
Apr 29, 2025

Conversation

chrjabs
Copy link
Contributor

@chrjabs chrjabs commented Apr 25, 2025

Fixes Issue

Closes #544

Description

Propagate the exit code of manim render when invoking manim-slides render.

Check List

Check all the applicable boxes:

  • I understand that my contributions needs to pass the checks;
  • If I created new functions / methods, I documented them and add type hints;
  • If I modified already existing code, I updated the documentation accordingly;
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.10%. Comparing base (0c6cd67) to head (4b53f21).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #545   +/-   ##
=======================================
  Coverage   80.09%   80.10%           
=======================================
  Files          23       23           
  Lines        2025     2026    +1     
=======================================
+ Hits         1622     1623    +1     
  Misses        403      403           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeertmans jeertmans changed the title Propagate manim render exit code feat(lib): propagate manim render exit code Apr 25, 2025
@jeertmans jeertmans added the cli Related to the command line interface label Apr 25, 2025
@jeertmans
Copy link
Owner

Thanks for your contribution @chrjabs!

For this to be complete, could you:

  • include a short explanation of your addition in the CHANGELOG, see other lines as examples:
    (unreleased)=
    ## [Unreleased](https://github.com/jeertmans/manim-slides/compare/v5.5.1...HEAD)
    (unreleased-chore)=
    ### Chore
    - Moved `docs` and `tests` extras, as well as `dev-dependencies`,
    inside groups in `dependency-groups`. This could break existing code
    when using one of those extras, but as they were not part of the public API,
    we do not consider this to be a **breaking change**.
    [#542](https://github.com/jeertmans/manim-slides/pull/542)

    You can add something like this:
    (unreleased-added)=
    ### Added
    
    - `manim-slides render` now exists with the same return code as the one returned by `manim render` or `manimgl`.
      [@chrjabs](https://github.com/chrjabs) [#545](https://github.com/jeertmans/manim-slides/pull/545)
  • add a new test, e.g., you copy this tests
    @pytest.mark.parametrize(
    "renderer",
    [
    "--CE",
    pytest.param(
    "--GL",
    marks=pytest.mark.skipif(
    sys.version_info < (3, 10),
    reason="See https://github.com/3b1b/manim/issues/2263.",
    ),
    ),
    "--CE --renderer=opengl",
    ],
    ids=("CE", "GL", "CE(GL)"),
    )
    def test_render_basic_slide(
    renderer: str,
    slides_file: Path,
    presentation_config: PresentationConfig,
    manimgl_config: Path,
    ) -> None:
    runner = CliRunner()
    with runner.isolated_filesystem() as tmp_dir:
    shutil.copy(manimgl_config, tmp_dir)
    results = runner.invoke(
    render, [*renderer.split(" "), str(slides_file), "BasicSlide", "-ql"]
    )
    assert results.exit_code == 0, results
    and make sure that a non-working Manim construct results in a non-zero error code (e.g., you can use self.play("this fail")).

Let me know if that is unclear :-)

@chrjabs
Copy link
Contributor Author

chrjabs commented Apr 26, 2025

Thanks for the pointers. Feel free to squash when merging, or let me know what else is missing.
The new tests pass locally.

Test output
manim-slides [ render-exit-code][🐍 v3.13.3]
❯ uv run pytest tests/test_slide.py::test_render_failing_slide
=============================================================================================== test session starts ================================================================================================
platform linux -- Python 3.11.12, pytest-8.3.4, pluggy-1.5.0
PySide6 6.8.1 -- Qt runtime 6.8.1 -- Qt compiled 6.8.1
rootdir: /home/christoph/Git/manim-slides
configfile: pyproject.toml
plugins: missing-modules-0.2.1, cov-6.0.0, env-1.1.5, qt-4.4.0
collected 3 items

tests/test_slide.py ...                                                                                                                                                                                      [100%]

---------- coverage: platform linux, python 3.11.12-final-0 ----------
Coverage XML written to file coverage.xml


================================================================================================ 3 passed in 19.90s ================================================================================================

Copy link
Owner

@jeertmans jeertmans left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for your contribution @chrjabs!

@jeertmans jeertmans merged commit 04b0eb5 into jeertmans:main Apr 29, 2025
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] non-zero exit code when producing animation fails
2 participants