TonicAudio / Tonic

Easy and efficient audio synthesis in C++
The Unlicense
522 stars 62 forks source link

proposal for cross-platform testsuite #259

Open andik opened 9 years ago

andik commented 9 years ago

Hey guys,

The following is the description of the concept of the cross-platform testsuite: please tell me what you think about it:

I did not use an existing framework because they either:

The files holding the testcases are cpp. I plan to use one file for each testsuite, but it is not system-imposed to do so.

The following is the basic class layout without any macro hideaway:

// part of test framework
enum Result { OK, FAILED};
class Testcase {};

template <class Testcase> 
class Testsuite {};

template <const char * name, class Testcase, int Line> 
class TestcaseRegistration {};

namespace TonicTests {
   class SpecialisedTestcase : public Testcase {
     // setup, teardown etc.
   };

   // We needs to define a variable which holds all testcases for the suite.
   Testcases* MyTestsuiteStorage = 0;   
   // The testsuite itself
   class MyTestsuite 
      : public Testsuite< SpecialisedTestcase, &MyTestsuiteStorage > 
   {
        class MyTestcase0;
        TestcaseRegistration<"simple test", MyTestcase0, 0> testreg0;
        class MyTestcase0 : public SpecialisedTestcase {
            int Result void(std::ostream& failstream) {
                int i = 1;
                const Result result = test_eq(1, i, "i", failstream);
                if (result != OK) return result;
                return OK;
            }
        };
   };

   int main(int argc, const char* argv[]) {
       MyTestsuite().run();
   }
}

this all could be hidden by using some macro magic:

    namespace TonicTests {
        class SpecialisedTestcase : public Testcase {
            // setup, teardown etc.
        };

        // MyTestsuite is necessary to be instantiated in main
        TESTSUITE(MyTestsuite, SpecialisedTestcase, "a simple Testsuite")

            TESTCASE("simple test")
                int i = 1;
                TEST(eq, 1, i, "i")
            END_TESTCASE()

        END_TESTSUITE()
    }

   int main(int argc, const char* argv[]) {
       MyTestsuite().run();
   }    

It all works extremly simple:

The magic here is the automatic Testcase registrations, so that we don't have to maintain a separate testcase list or generate code. the idea was taken from the catch framework and adapted to use classes instead of functions. I want to do this all on a class level, because this is much easier to specialise for example for generator tests, etc.

My Main problem with this solution is that the braces will get hidden. This may annoy pro users and syntax highlighting. But I did not find a better Solution. Also this saves us a lot of useless characters, and looks quite nice, so this is my proposal.

See https://github.com/andik/Tonic/tree/cmake-unit-tests Note: In the branch the principle class layout is done, but not the correct hideaway through macros, also I need to get rid of the #include stuff I do to get the testcases in. I want to make one cpp file for each suite. Also the class-naming there is not up to this concept. But yesterday night nearly instantly 10 of 16 generator test converted from the iPhone suite passed.

Maybe I'll publish this as a separate public domain Library and just include it into tonic so that this is available to other people also.

morganpackard commented 9 years ago

Sweet! You are on fire, @andik! Really looking forward to taking a look at this.

On Thu, Feb 5, 2015 at 12:26 PM, andik notifications@github.com wrote:

Hey guys,

The following is the description of the concept of the cross-platform testsuite: please tell me what you think about it:

I did not use an existing framework because they either:

  • depend completely on non-C++ macro "mountains" (tinytest)
  • have no automatic testcase registration (tinytest)
  • have complicated REQUIRE(x == y, ...) magic macros which cannot easily be specialised p.e. for buffer comparison. (catch, lest)
  • I plan to implement a special buffer comparison which writes both buffers to files so that one can use gnuplot to visualise the differences easily. BAM

The files holding the testcases are cpp. I plan to use one file for each testsuite, but it is not system-imposed to do so.

The following is the basic class layout without any macro hideaway:

// part of test frameworkenum Result { OK, FAILED};class Testcase {}; template class Testsuite {}; template <const char * name, class Testcase, int Line> class TestcaseRegistration {}; namespace TonicTests { class SpecialisedTestcase : public Testcase { // setup, teardown etc. };

// We needs to define a variable which holds all testcases for the suite. Testcases* MyTestsuiteStorage = 0; // The testsuite itself class MyTestsuite : public Testsuite< SpecialisedTestcase, &MyTestsuiteStorage > { class MyTestcase0; TestcaseRegistration<"simple test", MyTestcase0, 0> testreg0; class MyTestcase0 : public SpecialisedTestcase { int Result void(std::ostream& failstream) { int i = 1; const Result result = test_eq(1, i, "i", failstream); if (result != OK) return result; return OK; } }; };

int main(int argc, const char* argv[]) { MyTestsuite().run(); } }

this all could be hidden by using some macro magic:

namespace TonicTests {
    class SpecialisedTestcase : public Testcase {
        // setup, teardown etc.
    };

    // MyTestsuite is necessary to be instantiated in main
    TESTSUITE(MyTestsuite, SpecialisedTestcase, "a simple Testsuite")

        TESTCASE("simple test")
            int i = 1;
            TEST(eq, 1, i, "i")
        END_TESTCASE()

    END_TESTSUITE()
}

int main(int argc, const char* argv[]) { MyTestsuite().run(); }

It all works extremly simple:

  • TestcaseRegistration<> adds a instance of SpecialisedTestcase to MyTestsuiteStorage upon it's instantiation (which is ordered by declaration or TestcaseRegistration's in the class, maybe additional through LINE template parameter).
  • the TestcaseRegistration<> template is a subclass of Testsuite<>. Thus it can access it's static methods easily it uses Testsuite<>::addTestcase() for the job described abose.
  • the TESTCASE() macro also uses the Testsuite<> namespace: the Type ` Testsuite<>::LocalTestcase defines the Type which MyTestcase0 should inherit from.
  • MyTestsuite::run() iterates through the MyTestsuiteStorage list/map (has yet to be decided) and calls MyTestcase0::run() upon each testcase instance.
  • Testcase::test_eq() writes a message to failstream if the test fails and returns FAILED which in turn causes MyTestsuite::run() to count the test as failed and output a pretty formatted message (or write a log etc.).
  • The class names of testcases will get automatically generated based upon the LINE macro.
  • a TEST(...) macro expands to a simple function call which can be implemented in the SpecialisedTestcase easily. This way we can easily extend the testability. p.e. TEST(eq, ...) will be test_eq(...) but it also returns when test_eq() fails.

The magic here is the automatic Testcase registrations, so that we don't have to maintain a separate testcase list or generate code. the idea was taken from the catch framework and adapted to use classes instead of functions. I want to do this all on a class level, because this is much easier to specialise for example for generator tests, etc.

My Main problem with this solution is that the braces will get hidden. This may annoy pro users and syntax highlighting. But I did not find a better Solution. Also this saves us a lot of useless characters, and looks quite nice, so this is my proposal.

See https://github.com/andik/Tonic/tree/cmake-unit-tests Note: In the branch the principle class layout is done, but not the correct hideaway through macros, also I need to get rid of the #include stuff I do to get the testcases in. I want to make one cpp file for each suite. Also the class-naming there is not up to this concept. But yesterday night nearly instantly 10 of 16 generator test converted from the iPhone suite passed.

Maybe I'll publish this as a separate public domain Library and just include it into tonic so that this is available to other people also.

Reply to this email directly or view it on GitHub https://github.com/TonicAudio/Tonic/issues/259.

Morgan Packard cell: (720) 891-0122 twitter: @morganpackard

morganpackard commented 9 years ago

Generally, this looks great to me. Couple things:

  1. I'm not sure I'm convinced that the test files need to be named .test. I understand they're weird, and depend heavily on macros, but they are c++ code, and I'd like to be able to edit them with an IDE and have syntax highlighting, code completion, etc, without extra configuration and telling my IDE how to interpret them.
  2. Templates and macros make code harder to understand, and there's lots of both in here! I have no clue, for example, what's going on with this code: template<class TestcaseClass, Testcases** testcaseStorage>, and why one would want to use a pointer to a pointer as a template parameter, rather simply passing it in as a constructor argument or something All things being equal, I'd prefer not to use templates, but I understand they have their place (and I can't imagine Tonic itself with out them). So, if there's a choice between making the code simple, beginner-level, stupid, and fancy, demonstrating all of the power and complexity of templates, please keep it stupid and simple! But if the templates actually save code and simplify, please keep them.
  3. I got errors: screen shot 2015-02-08 at 8 54 25 pmThese errors go away when I comment out #include "generator.test". So something isn't working about the way these .test files are getting included. I get the same errors when I try building with make, instead of xcode. If we can come up with a more "old fashioned" way of defining and registering tests, I'd prefer that. By old-fashioned, I mean not using #include in the middle of class bodies. I know it can work, but to my eyes it's unusual, and a bit confusing. I also suspect that it has something to do with the errors I'm getting. As with #2, if there really is not elegant way to do it without using #include way down in the middle of the file, I vote to continue doing that, but otherwise, I'd vote for a less fancy, more standard approach.
  4. I think what you're doing with TestcaseRegistration<"simple test", MyTestcase0, 0> testreg0; is similar to what @ndonald2 did with the TONIC_REGISTER_SYNTH macro here. I wonder if the iOS synths might be a good alternative model of a way to set up these test cases. I haven't thought about the problem nearly as hard as you have, just wanted to direct your attention to the fact that we have a technique in use currently that allows us to create subclasses in a single .cpp file (no header, and the file isn't ever included anywhere) and automatically instantiate them without maintaining any master list (except by the use of the TONIC_REGISTER_SYNTH macro).

I think this is close to something we can run with, and I REALLY appreciate all your time and thought on it! If you can create something that compiles and runs reliably on all our target platforms (address issue #3), I'll be very happy. If you can come up with a less weird way of registering the test cases: probably not using #include mid-file, maybe with fewer macros, maybe adopting the technique used for the iOS synths so we never need to worry about including test cases, I'll be ecstatic. If you can find a way not to use things that my my head hurt like pointers-to-pointers-in-templates, I'll think I've died and gone to heaven. And if you incorporate a way to visualize differences in buffer output, well, my head will explode in amazement (in a good way!).

Thanks @andik!

andik commented 9 years ago

Hey morgan, you're basically a little too fast ^^ I pushed new commits I made over the last week. What you saw was my inital hackery, it's much cleaner now. Tests are working for me with mingw and xcode. But each testsuite is one executable which is good for makefiles and not so good for xcode... but it is working.

to #1 and #3: this has been adressed, really, this wasn't my best idea ^^

to #4: Thank you for that tip, I wasn't aware that this macro exists. I'm almost exactly doing what the TONIC_REGISTER_SYNTH macro does. But in fact each Testsuite is some kind of template/macro magic generated SynthFactory class. registered testcases must remain inside of this class. This makes it a little more complicated. #2 could be solved by moving the testsuite base template's content (AdapTest::Testsuite) into the TESTSUITE() macro. But I don't like code that has to end with \... but if this makes things easier for you I would do so.

If you mind, I created the basic headers as a new public domain library, so that it is refactored out of Tonic and you don't have to maintain it (https://github.com/andik/adaptest) ^^.

sadly visual buffer comparison isn't implemented yet. But thats not a problem, because I only can issue a pull request when the cmake stuff is merged, I do not want to fiddle the differences by hand. So I would prefer to finish this first, one after another. I still have a lot of stuff in my mind (how about a clock scheduler for the metro like pd has).

morganpackard commented 9 years ago

@andik Got everything to compile (using make), and it works! I'm not happy with how the generated xcode project looks, or performs (doesn't compile immediately, weird double-listed files, weird directory structure), but I see no reason not to just say the tests must be run using cmake, and that xcode isn't supported for tests.

I can imagine ways to make this nicer (the biggest thing being some way to run all the tests with one click), but I think what you've set up here will totally work for us. I'm excited to start writing some more tests and incorporating this in to my cross-platform work flow! Do you think this is ready to fold in to the main repository, or are there still significant changes you're considering?

andik commented 9 years ago

Hey morgan,

I have written some Stuff to print out buffer differences as CSV and generating a gnuplot script for it. I need to test and verify it a little, but I think during that week I will get this done.

I also thought a little about the cmake. I could PR only the test files without the cmake (using a new branch). This way we could keep that thread open and discuss it further...

By the way cmake provides a one-shot all test runner using make test. the test cmake file could be rewritten that all testsuite go into a single binary, this would also be a one-click runner. But the drawback would be if one testsuite crashes (memory access errror or so) the others would also.

morganpackard commented 9 years ago

I wasn't able to run the tests without cmake, so I'm not sure it makes sense to add them in to the main repo without cmake. I'm in favor of only supporting cmake + make builds for tests, and not testing/maintaining xcode/visual studio builds for tests.

On Mon, Feb 16, 2015 at 2:08 AM, andik notifications@github.com wrote:

Hey morgan,

I have written some Stuff to print out buffer differences as CSV and generating a gnuplot script for it. I need to test and verify it a little, but I think during that week I will get this done.

I also thought a little about the cmake. I could PR only the test files without the cmake (using a new branch). This way we could keep that thread open and discuss it further...

By the way cmake provides a one-shot all test runner using make test. the test cmake file could be rewritten that all testsuite go into a single binary, this would also be a one-click runner. But the drawback would be if one testsuite crashes (memory access errror or so) the others would also.

Reply to this email directly or view it on GitHub https://github.com/TonicAudio/Tonic/issues/259#issuecomment-74467577.

Morgan Packard cell: (720) 891-0122 twitter: @morganpackard

morganpackard commented 9 years ago

Hey @andik. Couple things.

  1. Do you know of a way to run the tests on windows using cygwin/make, without visual studio? It looks like CMAKE_CXX_COMPILER_ID needs to be set. But I don't know how to set it.
  2. I tried to compile the tests using the generated visual studio project and got this error: Error 220 error C2899: typename cannot be used outside a template declaration C:\small_design_firm\NPC\Nobel field new\software\submodules\Tonic\Tests\adaptest.h 210 Any idea of a fix for that?

tonic_test_build_error

LB-- commented 9 years ago

I think it could be written as for(auto i = tests.begin(); i != tests.end() ++i) instead

andik commented 9 years ago

@morganpackard

  1. use mingw I did test it successfully. After Install: cmake -g "MinGW Makefiles" ... then mingw32-make. Did run the tests on that.
  2. vote against using auto as this would require C++11. I wrote adaptest to not require that. I'll look into this.
andik commented 9 years ago

ok, commited a new release. To watch plots you don't need gnuplot anymore. only the generated html files and the supplied dygraph.js (copied into the directory of the html file). @morganpackard what do you think about merging all testsuites into one binary? This way they can be run from within xcode more easily.. it doesn't seem possible to create xcode unittests through cmake edit this would render make test useless (as it threads binaries as testsuites)

morganpackard commented 9 years ago

Hey @andik. Tried to install mingw but didn't finish. So tried again to generate the visual studio project using cmake. I get further than I did last time, but get this error:

Error 348 error C3861: 'snprintf': identifier not found C:\small_design_firm\NPC\Nobel field new\software\submodules\Tonic\Tests\adaptest.h 106

Apparently, Visual Studio doesn't support this function yet.

morganpackard commented 9 years ago

Got the stuff to compile by changing the offending lines to { sprintf(buf, fmt, v1); }. No idea if this is safe or appropriate.

With that change, I can compile and run individual tests from the project. (see the screen shot to see what the visual studio project looks like.) Hooray!!!! However, the application exits before I can read the output and tell if the tests passed or failed! Do you think it might make sense to wait for keyboard input before exiting the test suite application?

generated_vs2013

andik commented 9 years ago

Yes I can remember having this problem when I ported other software from Unix to windows... You could use _snprintf also. In fact, neither the usage of sprintf nor snprintf is a good idea, I need to write a formatter Instead of using this. I just have not that much experience using C++ strings and I want to keep the code as small as possible...

For the other point we would need to check if the tests started their own console. This may be complicated. How would you think about using file based recording instead of console based?

morganpackard commented 9 years ago

I think it's important to be able to see the results in the console. Why not just wait for keyboard input at the end of the main function?

morganpackard commented 9 years ago

This is enough to keep the console open in the standalone demo:

cin.get();

andik commented 9 years ago

If you like, I'll put this into the Tonic tests... I will not put this into adaptest because I do not think that this is a good general purpose solution. We could also make this only for Visual Studio (using a cmake generated define in a cmake generated headerfile aka TonicTestConfig.h or so.). Or by using a command line parameter (--wait or so).

morganpackard commented 9 years ago

It'd be great if you'd be willing to put that in the tonic tests. Thanks!

andik commented 9 years ago

done. Aaargh, commited not pushed... will do that this evening

morganpackard commented 9 years ago

I'd like to merge this in to development. I'd prefer a leaner approach to the cmakelists files, one where they aren't sprinkled around the project. At this point, I'm not comfortable enough with cmake to present it as the dominant build system. I'd much rather have the cmake files in their own place, separate from the source, and not in the root folder, like in #261. However, I don't feel strongly enough about that to hold off on getting the tests in to Tonic. I think what you've done is really great, and works solidly enough on mac and windows. So, what do you think needs to be done before this branch is ready to be merged in to development?

Thanks so much for all the work you put in to making this happen, @andik! Very much appreciated!

andik commented 9 years ago

How about having a cmakefile only in the Tests directory. (And maybe one in the standalone example directory?) Both could grap Tonic sources without building a lib. I'll do this and the file a PR, okay? I'd really love to see how you used this.

morganpackard commented 9 years ago

This sounds great. I am really interested in switching over totally to cmake for builds, but until we do that this more minimal approach feels good to me.