Skip to content
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

My own implementation of value parameterization (with multiple parameters) #321

Open
nyanpasu64 opened this issue Dec 18, 2019 · 6 comments

Comments

@nyanpasu64
Copy link
Contributor

nyanpasu64 commented Dec 18, 2019

Description

Value parameterization appears to be on the roadmap for doctest 2.4.0. After much difficulty, I have managed to add my own implementation of value parameterization.

  • It works much like pytest.mark.parameterize (https://docs.pytest.org/en/latest/parametrize.html).
  • However each parameter name and set of values is a free function, which can be used in multiple test cases.
    • This is also possible in pytest via range_0_3 = pytest.mark.parameterize("range_0_3", [0, 1, 2]).
  • Unlike pytest, it does not support assigning multiple values at once (at the moment): @pytest.mark.parametrize("test_input,expected", [("3+5", 8), ("2+4", 6), ("6*9", 42)])
  • To do so, my code would need to be modified:
    • (difficult) Change PARAMETERIZE() to accept more than 1 parameter type/name (difficult).
    • (easy) Change OPTION() to accept more than 1 assignment. Just replace k, v with k_eq_v, and #k " = " #v with #k_eq_v.

Usage

https://gitlab.com/nyanpasu64/exotracker-cpp/blob/b16d63e9427da8f6b9966b407cae0c874c116a58/tests/test_utils/test_parameterize.cpp

I've posted a shortened "usage example" here:

PARAMETERIZE(range_0_3, int, x,
    OPTION(x, 0);
    OPTION(x, 1);
    OPTION(x, 2);
)
PARAMETERIZE(range_0_4, int, x,
    OPTION(x, 0);
    OPTION(x, 1);
    OPTION(x, 2);
    OPTION(x, 3);
)
TEST_CASE("Generate the product set of all subcases.") {
    int x, y;
    PICK(range_0_3(x, range_0_4(y)));
}

My implementation is here: https://gitlab.com/nyanpasu64/exotracker-cpp/tree/b16d63e9427da8f6b9966b407cae0c874c116a58/tests/test_utils.

My parameterization implementation is built off subcases, and if you use multiple parameters in one TEST_CASE, doctest's support for nested subcases. doctest distinguishes subcases via (name, filename, line number) tuples. If I construct a subcase tree, filename and line number are not unique. To ensure doctest distinguishes each subcase, for each point on the subcase tree, I pass in a unique name based on the stringified form of each OPTION between the root and the current node.

The PICK macro takes the "leaf" subcase name (AKA the current parameter values) and logs it via INFO(). The subcase name will show up in failed assertions until the object created by INFO() is destroyed (when its scope ends). My subcase name generation is a bit ugly and adds a trailing ", " after the final subcase; it's good enough for my own use case, but should be fixed before upstreaming to doctest.

The macros are very fiddly and I am not confident they are free of undefined behavior.

My initial implementation had a use-after-free in PARAMETERIZE where inner (stack variable) was captured by reference, and the resulting function returned a lambda with a stale reference. This UB produced no visible signs on GCC, and std::bad_function_call on MSVC. I fixed this (https://gitlab.com/nyanpasu64/exotracker-cpp/blob/cycle-skip/tests/test_utils/parameterize.h#L52-56) by capturing [&, inner] instead. I still don't know what happens when you capture T & parameter via [&]. Do you get a reference, not a reference to a reference?

I developed my implementation based on doctest 2.3.4. Since then, #282 seems to have changed how SUBCASE works. My implementation seems to works fine on 2.3.6, but it was not what I studied while building my implementation.

Extra information

  • doctest version: 2.3.4
  • Operating System: Windows 10 and Linux
  • Compiler+version: MSVC, GCC
@onqtam
Copy link
Member

onqtam commented Dec 18, 2019

Work on this feature is appreciated! Seems that there is more interest in this feature - it was first requested a few years ago: #38

Some quick thoughts:

  • I would hope for this to be more general - have you looked at how Catch2 have implemented generators? (I haven't yet...)
  • doctest has a really hard constraint - it's not allowed to use any headers (STL or from the C stdlib) in the interface part of the header - where the forward declarations and macros reside... So unfortunately everything should be implemented with vanilla C++... I'm not sure this is going to be possible - it might have to go into a new separate extensions header, where STL includes are allowed (that has been planned for some time now)
  • there is no reference to a reference - if you make a reference and initialize it from another reference - both refer to the same object. Have you ran your implementation through the undefined behavior sanitizer (UBSAN) or the address sanitizer (ASAN)?
  • SUBCASE in function not behaving as expected #282 deals with the possibility for the same exact subcase (factored out into a function) to be used in different subcases - it shouldn't matter to what you have implemented - all your subcases are with unique names, so I assume all is fine. So now doctest actually keeps track of the current test case/subcase stack to determine if a specific subcase has already been encountered - so it no longer uses just the name/file/line of the subcase, but also the current subcase stack.

I might be able to look into this into more depth in at least a couple of months - for now all I can do is provide some light feedback such as this.

@Trass3r
Copy link

Trass3r commented Jul 14, 2020

Haven't looked into generators yet either but, in the meantime, when looking at DOCTEST_VALUE_PARAMETERIZED_DATA in https://github.com/onqtam/doctest/blob/master/doc/markdown/parameterized-tests.md it occurs to me there's got to be a way to implement this with macro iteration and stringification to get an efficient solution without any std::vectors usable like DOCTEST_VALUE_PARAMETERIZED_DATA(int data, 1, 2, 3, 4);.

@onqtam
Copy link
Member

onqtam commented Jul 14, 2020

@Trass3r that code was also written back when the SUBCASE macro didn't make a copy of the subcase string name and therefore the _doctest_subcases static container was required for the lifetime of the subcase name strings - this can now be simplified because the SUBCASE macro now makes a copy of the const char* which we pass it. Maybe I should update that...

@Trass3r
Copy link

Trass3r commented Jul 14, 2020

Quick test using https://github.com/swansontec/map-macro/blob/master/map.h:

#define STRINGIZE(arg)  STRINGIZE1(arg)
#define STRINGIZE1(arg) STRINGIZE2(arg)
#define STRINGIZE2(arg) #arg

#define PARAM_TEST(type, name, ...) type name; \
    static constexpr const char* s[] = {MAP_LIST(STRINGIZE, __VA_ARGS__)}; \
    static constexpr type d[] = {__VA_ARGS__}; \
    int i = 0; \
    for (auto&& e : d) { \
        SUBCASE(s[i++]) { name= e; } \
    }
int foo() {
    PARAM_TEST(int, data, 1, 2, 3);
    return data;
}

@Trass3r
Copy link

Trass3r commented Jul 14, 2020

Playground with improved version: https://godbolt.org/z/1s6dbo
Does generate a lot of code though due to all the SubCase/String objects and the copying. Gets much worse if you get rid of the arrays by manually unrolling the loop with the MAP macro OR by using clang with its excessive loop unrolling.

Also I guess the message could be improved:

DEEPEST SUBCASE STACK REACHED (DIFFERENT FROM THE CURRENT ONE):

  "3"

I have no clue what the "CURRENT ONE" is and how it's different. Looks like implementation details to me.
Should be something simple like

TEST CASE:  non-literal type ("3")

@nyanpasu64
Copy link
Contributor Author

Just fixed the links in my original post. I haven't looked at @Trass3r's code yet. But for single-use parameterization (not many tests with the same parameters), sometimes I just write a for loop, then call CAPTURE(var) to record which iteration caused test failures. It's less magical than my original code.

that code was also written back when the SUBCASE macro didn't make a copy of the subcase string name and therefore the _doctest_subcases static container was required for the lifetime of the subcase name strings - this can now be simplified because the SUBCASE macro now makes a copy of the const char* which we pass it. Maybe I should update that...

I'll have to look into whether I can simplify my macros and stop interning strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants