-
Notifications
You must be signed in to change notification settings - Fork 232
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
Automated unit tests? #21
Comments
I don't really have an idea of how this whold be organized either. But it sounds like a good idea. Creating a seperate function for drawing (printing) has been my plan since the beginning, but haven't gotten around to it. |
I am willing to work on this. I was writing a PR for #91 (see #115) and saw a lot of ways to modularize it. Currently a lot of the things are repeated in multiple places which makes the code fragile. If you forget to change something in all of the places, it becomes a mess. I think we should have a mental model of how we want to organise this before going ahead and doing changes. |
@hashhar great to see that you are taking an interest in this! The code is still a mess (and has gotten messier lately). BTW don't we already have have separate output modules for different types of output? Maybe my definition of "module" is off..? |
@karlstav No, I was saying that the recent changes you made to separate the output modules was a good step. We want more of that. Sorry for the misunderstanding. |
ahh yes. Agreed, a lot of other things should be separated out, like the config reading and validating, |
as discussed in pr168, the proof of the need of these tests proved itself immediately. @vinodadhikary can you give me some pointers on where to begin with these tests. I really don't have a clue where to begin. |
@karlstav, sincere apologies for delayed reply. I have forked your code and am currently trying to understand it. I will try to put together something around testing over the weekend that we can refer to in the future. |
draw n frames and quit, check if last frame is all zero or not all zero added some test configs and a script to run them all also added the tests to the github workflow
Now that we've got all sorts of pull requests going on, it might be good to at least think about unit-testing. We've had regressions and they're always hard to catch manually, but could have been prevented with automated tests that check for known past issues and correctness.
I don't know exactly how the tests would look, but @livibetter's manual test of piping generated sound into cava's inputs (#15) might make a good starting point.
In its present state, cava is tough to write tests for, as everything is in a single file that does many things. Separating the model ("how high should bars be") from its presentation ("how do we print this") would be a good first step.
Further thoughts, ideas?
The text was updated successfully, but these errors were encountered: