-
Notifications
You must be signed in to change notification settings - Fork 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
Init unit & integration tests #47
Comments
Maybe it's even better if someone different to me and therefore not directly involved in the creation of |
Omg I would love to get around to this, and it is slowly rising on my list of priorities. Right now I do quite manual "integration testing" for all the expected features... |
I would like to say this is not a priority for v0.1.0, but in all honesty if this is not implemented at announcement time, I believe the bug reports will pile up and we won't have a sustainable path forward for fixing the issues... I'm making this a v0.1.0. Feel free to debate me on this |
See this comment re: MacOS vs WSL pains
|
I absolutely agree. And it's very frustrating when changes introduce regressions. With a proper test harness we could be more certain that everything continues to work even if we re-structure some code or add new features. |
Has someone of you already started writing some tests? If not, I'd start with that on the coming weekend and the next week. |
So, with regards to testing: some modules like the cell range calculation could be easily unit tested. But how to test some more comprehensive behavior where we also want to check that Manim behaves how we like? For this,
|
Tests are tricky for me to design, having not done so for an extension before. In my mind, an integration test must:
I cannot imagine how to test the video in the interactive manim session 😂 |
I feel you, same for me, this is my first ever VSCode extension ;)
There will be absolutely no way to do this that wouldn't require to somehow instrument Manim itself. And that would be really cumbersome (e.g. letting instrument Manim to save pngs automatically in the background and then compare those with fixed pngs we have in our test pipeline...) So instead, we could check for other things, namely the feedback that Manim gives in the terminal, e.g. the progress output. But even that might not be the easiest to do. I will try out to create a Docker container with Manim installed and let's see if VSCode offers terminal reads in tests natively. But I mean, we already read the terminal for the extension itself, so that part should somehow work... |
I've set up the test tooling in #106 and am now in progress of installing Manim in #107 (also in GitHub Actions). And I've just found about this amazing repo: https://github.com/redhat-developer/vscode-extension-tester, I will try it out later since otherwise, testing UI elements could get really cumbersome very quickly (or even impossible without having access to the HTML DOM). |
I've done a lot of effort to get up and running a complete testing pipeline that runs through in GitHub Actions on macOS, Ubuntu and Windows and that you can also run through on your local machine and even debug the tests. See PRs #106, #107 and #109. See the developing guide on how to use it. Of course, we have to write more tests, but this issue of getting the test tooling up and running is complete. |
The code is now complex and probably also stable enough to definitely justify a test harness. Otherwise, with every line I change, I fear breaking something down the road, especially the
ManimShell
is not that easy to wrap your head around and there are many edge cases. And it's really annoying to have to replay all possible scenarios manually with every change.Luckily, VSCode comes with a Testing Extensions Guide for us. We should also use GitHub Actions to run these tests upon every push to a pull request. We should then define in the settings that these tests must run through successfully before merging is possible.
The text was updated successfully, but these errors were encountered: