add parametrized tests to cpputest#666
add parametrized tests to cpputest#666the-real-orca wants to merge 16 commits intocpputest:masterfrom
Conversation
The basic idea is: - have so called mix-in tests that can be inserted to other test groups - use a second, non-executed test registry (reuse TestRegistry) -> mix-in registry - add tests and test groups to this mix-in registry (derived from Utest and UTestShell) -> mix-in tests, mix-in groups - have a normal test that injects the appropriate test from the second registry in the scope of the current test group -> apply mix-in group to test group - this injection test is self-looped until all tests from the mix-in group are executed
The basic idea is: - have so called mix-in tests that can be inserted to other test groups - use a second, non-executed test registry (reuse TestRegistry) -> mix-in registry - add tests and test groups to this mix-in registry (derived from Utest and UTestShell) -> mix-in tests, mix-in groups - have a normal test that injects the appropriate test from the second registry in the scope of the current test group -> apply mix-in group to test group - this injection test is self-looped until all tests from the mix-in group are executed
Conflicts: src/CppUTest/Utest.cpp
Conflicts: tests/MixInTest.cpp tests/MixInTest.h
|
Looks interesting. Could you perhaps explain the use case? Which problem do parametrized mix-ins solve? |
|
About some of the CI errors - |
|
Interesting! I looked at the code and it does need some work and would like to have a bit of discussion on some parts, but this looks definitively useful. I do think we ought to be able to make this change smaller. I'm not fully understanding a couple of things. One thing I already mentioned in the mailing list, which is... why does it need a separate TestRegistry? Oh, the doc part will probably need to go in the doc repository on the website, right? If so, perhaps remove it from here. Also, a small additional change. Could you put the example tests in one file? And could you try to make the tests a bit more descriptive. I think it'll need more tests still also, but let me first understand it better! :) Again, thanks for the contribution! |
|
thanks for your feedback. I know that is’s a first proposal and that there may be some changes necessary. I tried to follow the cpputest philosophy and style as much as possible and reuse / mimic existing classes / macros. First to the motivation / problem to be solved: A (simplified) real world example (actually my main motivation). With parametrized tests, all the interface tests can be grouped in a mixing group (similar to a test group) and added to one or more actual test groups. So when adding a new test to the mixin group, the new test is automatically applied to all test groups importing this mixin group. Mixin test registry: Example tests: My first intention was to check if you like the basic concept and iteratively adapt it so that it does fit to the cpputest implementation. |
|
@the-real-orca Thank you for taking the trouble to explain. I'll definitely take a look at the tests now that I have a clearer picture. |
|
@the-real-orca to fix the Travis build: MIXIN_PARAMS(DemoMixInGroup) // MIXIN_GROUP name
{
SUT* obj;
char const* expectedName;
};@basvodde I think it's thanks to the new Cmake features that this was included in the Travis build at all -- the new files haven't been added to Automake yet. |
|
Also: class SUT
{
public:
virtual const char* className() = 0;
virtual ~SUT() {} // needs virtual destructor
}; |
tests/MixInApplyTest.cpp
Outdated
There was a problem hiding this comment.
Also, for Travis CI to pass, the final semicolon needs to go.
|
@basvodde why is it that the new tests don't run when I build locally? I added them to my CodeBlocks project in order to test-run them, but they don't get built by automake. |
|
Successful Travis build see https://travis-ci.org/arstrube/cpputest/builds/65948028 |
|
@the-real-orca related to the TestRegistry. Why not in the MIXIN_APPLY(ImplBTestGroup, DemoMixInGroup, ImplB_test) macro create one of the MixinTest objects and then store it within the test? Why would you need to have a global storage of them? |
|
@arstrube: thanks for taking the time to fix the code |
|
I’ve added documentation to the wiki, maybe this helps to clarify things a bit. @basvodde: The Mixin Tests are not in the scope where MIXIN_APPLY(ImplBTestGroup, DemoMixInGroup, ImplB_test) is used. MIXIN_APPLY() will typically be used in TestMyImplementationXXX.cpp, whereas the MIXIN_TESTs are defined in a different Cpp file. So they don’t know from each other until they are linked together and being executed. The only thing MIXIN_APPLY() knows about is the mixin group name and the according MIXIN_PARAMS() definition has to be visible for MIXIN_APPLY(). Without a global storage, all Mixin Test implementation code would need to be included to the TestMyImplementationXXX.cpp file. But then I would still need some kind of local storage at file scope. Another alternative would be some kind of preprocessor loops, but I’m not aware of a useful construction. |
|
@the-real-orca here is what needs to be done for Automake so the tests will actually be compiled and run: a371c41. |
|
About the discussion regarding one test file vs three - perhaps we could distinguish between
The former could be terse and ideally even live in one file (if possible). The latter should be a little more verbose than currently, and have individual tests against the implementations as well as the mix-in test. Also, the name of the mixin should then be a bit more descriptive than |
…ubsequent scripts have no problems with parsing the name)
|
Thanks for your suggestions. I’ve split the tests into tests testing that the mixin is working -> tests/MixInTest.cpp and a show-case example -> examples/AllTests/MixIn… (I’ve updated the makefile, hope it compiles under Linux) You are absolutely right, having a colon and space name can confuse subsequent scripts. I’ve changed it to underscores, so there is a clear separation between prefix and test name, but still composes to a valid class name. |
|
Hi everyone, |
|
@jakubmiernik I'm not happy with the code. The size of the PR makes it hard to work with, which is why it keeps getting postpones. Now I'm planning for a release and only adding items related to stability. Thus after the release, I hope to look at it. Quick scan on the code, it needs a lot of work still. |
No description provided.