kamping-site / kamping

KaMPIng: (Near) zero-overhead MPI wrapper for modern C++
https://kamping-site.github.io/kamping/
GNU Lesser General Public License v3.0
24 stars 0 forks source link

Remove obsolete tests/test_assertions.hpp and all references to it #553

Closed lukashuebner closed 8 months ago

lukashuebner commented 10 months ago

We have multiple (different) redefinitions of KASSERT_KASSERT_HPP_KASSERT_IMPL for testing with KASSERTs. One in tests/test_assertions.hpp (used in three files) and one in tests/test_helpers.hpp (used everywhere else).

@niklas-uhl and I decided to just try to remove the lesser used version and see what happens even though we vaguely remembered what happened now: If the KASSERT fails only one some ranks, the test will fail.

The two redefinitions of KASSERT_KASSERT_HPP_KASSERT_IMPL take the following two approaches:

tests/test_assertions.hpp: Make KASSERT throw an exception on error. Will give a compilation warning (=error) for KASSERTs in destructors because they might throw during stack unwinding which will std::terminate().

tests/assertion_helpers.hpp: Use GoogleTest's death tests to check if the program failed with a specific error message. Breaks some assertions as described above.

We thus need to do some of the following:

@DanielSeemaier originally implemented those two methods, so his input might be valueable.

Hespian commented 10 months ago

What was this doing? (seems like removing it makes tests fail?)

Also, this is not a bug ;)

lukashuebner commented 10 months ago

@Hespian I updated the comment in the first message to include more details.

niklas-uhl commented 8 months ago

while currently the death test version is used more often, after writing some more death tests, it became clear that the version which overrides KASSERT by throwing exceptions is a lot more flexible and makes it easier to test. I renamed the death test version, so that we do not have an ambiguous name. It is currently used nowhere, because using only the exception version all tests succeed.