Zone Validation Test

[derselbst]

This is my implementation of a unit test to verify the preset and instrument zone validation behaviour.

Addresses #813.

[mawe42]

The tests look good, although they are very much tied to the current implementation. Which is to be expected, they are unit tests, after all.

I've taken your four unit test cases and created small test soundfonts for each case and added them to the defsfont-integration-test branch: 1673153

I've created the broken soundfonts quickly with a hex editor. It's actually fairly easy to do, I think. I don't know... might be less work (and more stable with regard to refactoring changes in the sf loader) in the long run?

[mawe42]

And I've added three more that test the same error conditions for instrument zones in 8cc99c1.

Like I said... I'm not sure if they would be a better approach than the unit tests. I added them quickly so we can compare both approaches. Editing the files by hand was fairly easy once I added some debug code that gave me the file positions of the bits I was interested in. And they will probably be more stable than the unit tests... but the unit test are easier to read, understand and modified than a binary file with defects.

What do you think?

[derselbst]

I agree with your points, but I can't decide either. I intentionally messed up the code and both approaches reliably fail. Spotting the root cause is a bit easier to do with the unit test here, provided that it has a test case that covers the broken piece of code. And as I currently don't see any big code changes incoming, I would still say, let's implement both approaches. Not generally for all parts of the sfloader, but at least for testing this particular zone validation logic. I'll continue working on this and try to make it a little nicer.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

78.1% Coverage
0.0% Duplication

[derselbst]

@mawe42 Sorry for flooding your mailbox with all those single pushes. But at least now it should be good enough. Coverage is also nice for both preset and instrument cases.

[mawe42]

Sorry for flooding your mailbox with all those single pushes.

No probs, that's what mail filters / delete buttons are for :-)

But at least now it should be good enough. Coverage is also nice for both preset and instrument cases.

I'm a bit divided on the "extern" for the two previously static functions. Feels a little dirty to make those changes to the code only to be able to unit test them. But it's not a big problem, just a code smell in my opinion.

And in this PR you add the Gen_* enums back into fluid_sffile.h... is that just temporary? Or is there another reason not to use the GEN_* enums?

And one more general thought: the tests are fairly complex and very close to the current implementation. That poses the question for me: what is the actual purpose of these tests? They probably won't be much use for non-trivial refactorings of the sffile code, as they are so closely coupled to the current implementation. And they (partly) test code that has been in production for decades. So I guess I'm still wondering if they are worth the maintenance effort. But on the other hand... as you say, there probably won't be much code churn in the tested code. So maintaining these tests will probably not cause additional work.

[derselbst]

And in this PR you add the Gen_* enums back into fluid_sffile.h... is that just temporary? Or is there another reason not to use the GEN_* enums?

Don't worry. That's just temporarily.

what is the actual purpose of these tests?

For now, the (only?) purpose is to test and verify the current behaviour, to make sure your #823 doesn't introduce a regression or unwanted behaviour change ;) Time will show, if this test survives or not.

[mawe42]

Ok, then I have no further comments: LGTM :-)