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

Better testing of the Disabled flag. #140

Closed
vsariola opened this issue Aug 14, 2024 · 5 comments
Closed

Better testing of the Disabled flag. #140

vsariola opened this issue Aug 14, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@vsariola
Copy link
Owner

In #139, a bug was discovered in the Disabled flag. The original sin was that the feature was added hastily without proper testing.

This issue is here just to remind to add proper unit tests for the unit Disabled flag, to see that there are no further lurking bugs in there.

The feature could also be reimplemented in a more future-proof way: just filter out all the disabled units before generating the bytecode so there is no way the disabled units could affect the compile results.

@vsariola vsariola added the enhancement New feature or request label Aug 14, 2024
@LeStahL
Copy link
Contributor

LeStahL commented Aug 14, 2024

https://github.com/vsariola/sointu/blob/master/vm/delaytable.go#L145-L147

this block seems to crash the tracker now if a track with disabled units is in the cache.

@vsariola
Copy link
Owner Author

I just don't seem to learn, do I? First I add a feature without unit tests, then I push out a supposed fix without actually testing it? Ok, let's do this properly then :D I'll write a unit test to reproduce the error and THEN rewrite the implementation.

Oh well, at least this is not a software ran by millions and I'm not responsible for this. Sorry.

@LeStahL
Copy link
Contributor

LeStahL commented Aug 14, 2024

Don't worry. :) shit happens. Also you're always really quick with the maintenance here, which is super cool imo :)

@vsariola
Copy link
Owner Author

Took me while to figure out how to make the crash, but the immediate fix to the crash & the unit tests should be there.

@vsariola
Copy link
Owner Author

vsariola commented Aug 14, 2024

I didn't do the refactoring to filter out the disabled units, because that would make a slightly unnecessary copy of the synth structure just for that purpose. This is something that happens every time you e.g. move a slider a notch so I'm a bit worried that maybe the amount of garbage this creates is not negligible... So I need to test this in real action if there's any effect on performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants