LMMS
Add unit tests for Base64 encoding/decoding methods
This is a relatively small set of test cases to ease myself into my comprehensive unit testing initiative. I have yet to familiarize myself with all the expectations and practices of LMMS development, so I'm not expecting to have gotten this code right on the first try. Please be ruthless in nitpicking everything, because I want to learn best practices as quickly as I can, so that I can make my PRs as easy as possible to review in the future :slightly_smiling_face:!
Most uses of these functions revolve around encoding/decoding float arrays of samples, so that is what is primarily tested. The tests encode mock data, then decode it, and confirm that they are identical. Does not test the compatibility QString/QVariant decode method, which appears to be deprecated.
I hope this doesn't interfere with @Veratil's non-Qt base64 PR (#6432).
Got my pre-commit hook for clang-format set up, so no one should need to waste their time on style review now.
Got my pre-commit hook for clang-format set up, so no one should need to waste their time on style review now.
I'm still going to review style. 😁
Given #6432, what is your plan @Veratil? Since that PR I think will change the interface for these methods, should it wait for that to be merged? I also suppose we could go with this for the unit test rather than the one added there?
@TechnoPorg, I rather not have you come back and make a new PR to change this unit test after #6432 (if it gets merged), but its up to you.
In terms of the code for this, try to not allocate needlessly on the heap with new. Whenever you call new and pair it with a delete at the end of the scope (i.e., the braces of the function you are currently in), the lifetime of that object would be the same as if it was created on the stack and de-allocated at the end automatically (or with RAII using std::unique_ptr, though that also should not be needed here).
The problem is that the Base64 methods for some reason require a pointer to a pointer, which I don't think is necessary anyways? It should be something like std::string encode(const unsigned char* data, size_t size), which would directly help the making of this unit test since all you would need to do is a reinterpret_cast on the pointer to the float on the stack and use sizeof(float) for the size.
Ideally we drop the old methods and use the new and update accordingly, but depends on how everyone feels about it and if we want to merge it.
I'm still going to review style. 😁
My bad, sorry! I thought clang-format did it all, but now that I look at a few other PRs, I do indeed see style review.
I am happy to let this PR go in favour of #6432, which I see already includes unit tests. I don't want to create more work for @Veratil needlessly rebasing their PR on top of this one when it accomplishes the same thing. If that one is planned to be merged soon, I'll close mine.
And thanks for the code feedback @saker! I really appreciate it. Is everything otherwise good in terms of test structure?
Is everything otherwise good in terms of test structure?
In terms of the way the tests were structured, it looks fine to me.