Optimized some tests to reduce duration #2274
Merged
+27
−27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k4pran
approved these changes
Nov 7, 2021
| (1280, 720, ","), | ||
| (1920, 1080, "-"), | ||
| (2560, 1440, ";"), | ||
| # (3840, 2160, ","), |
There was a problem hiding this comment.
Are these just left commented out to allow for local testing of these resolutions?
There was a problem hiding this comment.
Nope, I each one of these tests takes ages and do we really need that many different resolutions to be tested?
There was a problem hiding this comment.
I don’t think so, think we can just remove the commented out ones ?
There was a problem hiding this comment.
Ups, I misunderstood your first comment. That's exactly why they're commented out and not deleted.
Sorry.
kolibril13
approved these changes
Nov 12, 2021
behackl
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
General Problem
Manim testing is very slow and should be optimized.
You can get the slowest 50 test with:
Please run this command and post its output here. You should also state which OS you're using since—as you'll see in a moment—there are huge differences between them.
This is the output on a macbook (from @behackl):
This is how it looks like (without the changes of this PR) on my machine—Fedora Linux:
To get the time required for the entire test suit, you can use the GNU time tool like this:
Make sure to add the
\as you have to use the GNU time binary and not the shell builtin. You might not need this if you're using a very exotic shell.You'll receive an output similar to this:
The value you're looking for is
xx:yy.zzelapsed. This is the total amount of wall time the user has to wait for the program to finish.Effects of this PR
This PR is a first attempt at that and is by no means exhaustive. It optimizes a few test while attempting to remove as little testing coverage as possible.
This is my new output:
With this, the tests take a total of 1min and 40sec instead of 2min 5sec without the changes—20% improvement.
This isn't too much, but should be detectable by and just the beginning. It shows how small changes can already improve testing times considerably.
Reviewer Checklist