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

Improved {EXPECT,ASSERT}_KASSERT_FAILS #344

Open
lukashuebner opened this issue Jun 24, 2022 · 5 comments
Open

Improved {EXPECT,ASSERT}_KASSERT_FAILS #344

lukashuebner opened this issue Jun 24, 2022 · 5 comments

Comments

@lukashuebner
Copy link
Member

  1. Currently, these routines do not support checking the error message in their MPI version.
  2. They also do not yet support KASSERTs which fail only on some ranks.

I propose to introduce a {EXPECT,ASSERT}_KASSERT_FAILS which does not relegate the check to {EXPECT,ASSERT}_THROWS() but rather uses it's own try/catch block to check if the KASSERT exception is thrown. We can then check the error message of this exception and allreduce the information if the local KASSERT fired or not over all ranks.

@Hespian
Copy link
Member

Hespian commented Jun 24, 2022

I think an issue with that would be that if one rank throws, and the others don't, they will be stuck in their collective MPI operation and the test would never finish.

@lukashuebner
Copy link
Member Author

lukashuebner commented Jun 24, 2022

I was thinking about something along those lines:

#define EXPECT_KASSERT_FAILS_MPI(code, failure_message) \
       bool kassert_failed = false;
       bool exception_message_matched = true;
       try {
          code;
        } except (::kamping::testing::KassertTestingException& e) {
          kassert_failes = true;
          if (e.msg != failure_message) {
              exception_message_matched = false;
          }
       }
      // Allreduce over kassert_failed with operation OR and exception_message_matched with operation AND
      // Output error messages and fail test if !kassert_failed
      // else if !exception_message_matched output error message and fail

@lukashuebner
Copy link
Member Author

I'm assuming here that we turn all failed assertions into exceptions and can thus catch them.

@Hespian
Copy link
Member

Hespian commented Jun 24, 2022

Maybe I'm missing something. But

try {
          code;
}

Will never finish on the ranks that don't throw so this will deadlock.

@lukashuebner
Copy link
Member Author

Oh ... right, there might be an additional communication inside code; which might cause it to deadlock.
I solved this once using still_alive() calls, but this would need to happen inside code; here, which seems weird. Hm ... use fault tolerance during tests? ;-)

We're already overwriting KASSERT during tests, we could add code like above directly to KASSERT? We would then need to replace

if (comm.is_root()) {
 KASSERT(...)
}

with

KASSERT((!comm.is_root() || ...));

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

No branches or pull requests

3 participants