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

Deduplicate test names in parameterised tests #656

Conversation

tobiasleibner
Copy link
Contributor

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

@tobiasleibner tobiasleibner force-pushed the deduplicate_parameterised_test_names branch 4 times, most recently from b4cb115 to a40c780 Compare January 31, 2025 15:54
@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

@tobiasleibner tobiasleibner Jan 31, 2025

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Done.

apply(
[f, name](const auto&... args) {
[f, name, unique_name, &counter](const auto&... args) {
(detail::on<F>(events::test<F, Ts>{.type = "test",
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a mechanism to print the values for some types. A user could extend this for their custom type by adding a specialization for the struct. What do you think?

@tobiasleibner tobiasleibner force-pushed the deduplicate_parameterised_test_names branch 2 times, most recently from 01360a3 to 5a5034e Compare February 3, 2025 09:55
};

template <class T, class... Types>
concept is_any_of = (std::is_same_v<T, Types> || ...);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the minimum C++ version ut supports? I thought it is C++20 but I just saw that there is a CI workflow (ClangCl, 17) which sets the C++ version to 17 (which, however, does not seem to work properly, because the C++20 requires clause I use below compiles just fine).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the minimum supported version is below 20, I will have to modify this code not to use concepts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, ut was aiming for c++20 but at the moment of development concepts werent available in the compilers hence is_valid etc has been used instead, making ut working with c++17, ideally we would keep that not to break that if there are devs relying on it, BTW besides that the PR is fine, I beleive it's a nice improvement, not ideal with the #ifdefs but oh well, we need to make compiler happy in the end, it's not strictly required but I think it would be nice to have printing actual paramater values for POD types only (so let's int, float, etc.) can be printed as value instead of type and everything else would be as it's already added, IMHO, otherwise it's good and let's get it in as soon as it ready, thanks again on working on that!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried to compile ut with C++17 and there are already a lot of C++20 features used. Here is the first page of errors in VSCode:
image

The problem is that the C++17 CI workflow uses C++20 because the standard given via `CMAKE_CXX_STANDARD is only a lower limit and does not enforce C++17 usage (I did not know that before, either) and most compilers use C++20 by default now.

So we have two options here:

  1. Fix the places where C++20 is already used and fix the CI workflow to actually use C++17.
  2. Set the minimum supported C++ version to 20

I would vote for option 2 since you are already using C++20 and apparently there were no complaints 😊.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back to using C++17 and removed the C++17 workflows now (which did not use C++17 anyway, see above). If you would rather add back C++17 support, let me know.

@tobiasleibner tobiasleibner force-pushed the deduplicate_parameterised_test_names branch 2 times, most recently from 7fc861e to 43c44fd Compare February 3, 2025 15:03
@tobiasleibner tobiasleibner force-pushed the deduplicate_parameterised_test_names branch from 2c6c2fd to 3a5b9ce Compare February 4, 2025 07:50
@tobiasleibner
Copy link
Contributor Author

The tests are finally passing. Assuming we go with C++20, this PR is ready for review/merge.

@tobiasleibner tobiasleibner force-pushed the deduplicate_parameterised_test_names branch from 3a5b9ce to fbb06e3 Compare February 4, 2025 08:57
@tobiasleibner tobiasleibner deleted the deduplicate_parameterised_test_names branch February 4, 2025 08:57
@tobiasleibner
Copy link
Contributor Author

The commts here were already contained in #658, so closing this PR.

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

Successfully merging this pull request may close these issues.

2 participants