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

Minimal tests #28

Merged
merged 7 commits into from
May 20, 2020
Merged

Minimal tests #28

merged 7 commits into from
May 20, 2020

Conversation

huguesdevimeux
Copy link
Member

@huguesdevimeux huguesdevimeux commented May 20, 2020

closes #28

@huguesdevimeux huguesdevimeux linked an issue May 20, 2020 that may be closed by this pull request
Copy link
Contributor

@leotrs leotrs left a comment

Choose a reason for hiding this comment

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

As mentioned in #24, we are trying to make the LaTeX dependency be optional in the future.

Can you please change the TextMobject to be normal text, not latex? You could also add another short scene that uses LaTeX, and in the future this one would only be ran if LaTeX is present.

Can you also please remove the sample_scenes.py file from the top level?

@leotrs
Copy link
Contributor

leotrs commented May 20, 2020

Oh also please add "closes #28" to your PR description so github knows to close that issue automatically when this is merged.

@huguesdevimeux
Copy link
Member Author

@leotrs Yes, you're absolutely right about the LaTex thing. I am changing that.

And which file are you talking about? I don't see any sample_scenes.py.
If you were talking of example_scenes.py I would be in favor of keeping it.

@PgBiel
Copy link
Member

PgBiel commented May 20, 2020

We're still going to have to test LaTeX usage though

Also if you're using TextMobject then it's implied it will use LaTeX; you'll want to replace everywhere where it says "TextMobject" with "Text" in order to avoid LaTeX

@kilacoda-old
Copy link
Contributor

We could implement a check in manimlib.imports for that I think. Something like

if latex_installed():
    from manimlib.mobject.types.svg.tex_mobject import *

leotrs
leotrs previously approved these changes May 20, 2020
@leotrs leotrs dismissed their stale review May 20, 2020 12:36

TextMobject needs to be replaced by Text

@leotrs
Copy link
Contributor

leotrs commented May 20, 2020

@huguesdevimeux yes I did mean example_scenes.py, my bad.

My rationale is that it is highly unorthodox to have .py files at the top level. I'd much rather have a new folder samples/ that includes that file, or just use those scenes in example_scenes.py as tests (as you are kinda doing right now). I really don't think it's a good idea to leave example_scenes.py up top.

@PgBiel PgBiel added the enhancement Additions and improvements in general label May 20, 2020
@leotrs
Copy link
Contributor

leotrs commented May 20, 2020

Ack, sorry to be fussy, I meant to move the file to samples/samples_scenes.py and not to manimlib/samples/samples_scenes.py. Two reasons for this: the sample scenes are in principle not part of the core library, and in fact can be ran without knowledge of any of the other code in manimlib/. The other reason is that the sample_scenes.py file needs to import manimlib and that gets tricky when inside the manimlib folder itself. (In fact, we are trying to keep all imports local within the library itself as per #15.)

@huguesdevimeux
Copy link
Member Author

huguesdevimeux commented May 20, 2020

Thank you for being "fussy" like you said, it helps me to do less mistakes :) I will correct that.

Nevertheless I don't think its a good idea to mix up samples_scenes.py with testing. My idea is that in tests_example_scene.py, one would be able to do precise test about a precise feature without having to render a full and complex scene like there are/there will be in samples_scenes.py. At the moment the tests functions are not plentiful because we didn't start the feature development, but I'm convinced that it will be soon or later.

@leotrs
Copy link
Contributor

leotrs commented May 20, 2020

Fully agreed there, the point is to have something that could serve as a test, even if it's just a run-these-scenes-and-eyeball-them kind of a test. In the (hopefully near) future, that will most definitely change to something way more exhaustive.

So for now, we can use the scenes in samples/ as quick and dirty tests, until we have a true tests/ folder.

leotrs
leotrs previously approved these changes May 20, 2020
@leotrs
Copy link
Contributor

leotrs commented May 20, 2020

Great! We need one more review here. @PgBiel ?

tests/tests_example_scene.py Outdated Show resolved Hide resolved
@leotrs leotrs merged commit fb03cde into ManimCommunity:master May 20, 2020
@PgBiel PgBiel added the testing Anything related to testing the library label May 20, 2020
@eulertour eulertour mentioned this pull request May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general testing Anything related to testing the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add minimal tests
5 participants