-
Notifications
You must be signed in to change notification settings - Fork 124
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
Deduplicate test names in parameterised tests #656
base: master
Are you sure you want to change the base?
Deduplicate test names in parameterised tests #656
Conversation
Without this change, there are lots of warnings about tests that have already been registered. Also, counting the number of asserts does not seem to work properly, see boost-ext#581.
18822db
to
b4cb115
Compare
b4cb115
to
a40c780
Compare
@@ -1025,7 +1025,12 @@ struct eq_ : op { | |||
|
|||
if constexpr (type_traits::has_static_member_object_value_v<TLhs> and | |||
type_traits::has_static_member_object_value_v<TRhs>) { | |||
#if defined(_MSC_VER) && !defined(__clang__) | |||
// for some reason, accessing the static member via :: does not compile on MSVC |
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.
instead #if/#else why not simply switch to lhs.value @ rhs.value?
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.
That was my first attempt, but then the less-than operator does not compile on clang anymore. It interprets the <
in lhs.value < rhs.value
as opening brace for a template. For some reason, this code seems to be really hard to parse for compilers. We could probably use the #if/else#
only for the less-than operator and switch to lhs.value op rhs.value
for the other operators.
apply( | ||
[f, name](const auto&... args) { | ||
[f, name, unique_name, &counter](const auto&... args) { | ||
(detail::on<F>(events::test<F, Ts>{.type = "test", |
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.
The solution here makes sense, wonder whether maybe printing actual value if possible wouldn't be nicer?
"foo"_test = [](auto arg) {} | std::vector{1, 2, 3};
// foo/1
// foo/2
// foo/3
or
// foo(1)
// foo(2)
// foo(3)
which is simiar just with values intead of (1st parameter) etc.. which can be a backup if value is not printable.
any thoughts?
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.
For values which give a small string printing them is nicer than just enumerating the parameters, but these strings could potentially be very large. For example, when testing a linear algebra library, we could have something like
"foo"_test = [](auto arg) {} | create_test_matrices();
and then get
foo ([1 2 3 4
5 6 7 8
9 10 11 12])
already for a small matrix, and for large matrices (100x100, 1000x1000, ...) the output will be huge.
We could first stringify the values, then check their size (and maybe whether they contain newlines) and use the enumeration instead if they are too large. This would give the nicest output, but I am unsure about the performance implications. In the matrices example, converting a large matrix to a string might take quite some time (and a lot of memory for very large matrices).
We could restrict printing to certain types (e,g., integral and floating point types?) and use the enumeration for other types. We could also allow users to make other types printable by providing overloads/specializations of the functions/classes we are using to decide if we want to print or enumerate.
Without this change, there are lots of warnings about tests that have already been registered. Also, counting the number of asserts does not seem to work properly, see #581.
Problem:
When using the
operator|
syntax for parameterized tests, the test name is not adapted for each parameter and thus is not unique, which results in warnings (see #640) and problems with counting asserts (see #581). For steps to reproduce the warnings, see #640.Solution:
Deduplicate test names by adding information about the current parameter in parentheses after the test name. See
tests/ut.cpp
for examples.Issue:
#640, #581