-
Notifications
You must be signed in to change notification settings - Fork 514
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
add parametrized tests to cpputest #666
base: master
Are you sure you want to change the base?
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
}; |
class ImplA : public SUT | ||
{ | ||
public: | ||
const char* className() { return "ImplA"; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.