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.

Sign up for GitHub

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

Jump to bottom

fix: panic.test: actually revert env var to original value #602

Merged
merged 1 commit into from
Oct 14, 2024
Merged

fix: panic.test: actually revert env var to original value #602

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/silo/common/panic.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,22 @@ void assertMsg(std::string got, std::string expected) {
}
// good enough, ignore the rest of the remainder.
}

void nullCapableSetenv(const char* name, const char* value, int overwrite) {
if (value) {
setenv(name, value, overwrite);
} else {
unsetenv(name);
}
}

} // namespace

// NOLINTNEXTLINE(readability-identifier-naming,readability-function-cognitive-complexity)
TEST(panic, assertEqPanicModes) {
ASSERT_EQ(1 + 1, 2);

const char* old_env = getenv("SILO_PANIC");
setenv("SILO_PANIC", "", 1);
try {
ASSERT_EQ(1 + 1, 3);
Expand All @@ -51,7 +61,7 @@ TEST(panic, assertEqPanicModes) {
ASSERT_DEATH(ASSERT_EQ(1 + 1, 3), "ASSERT_EQ failure: 1 \\+ 1 == 3: 2 == 3");

// revert it back
setenv("SILO_PANIC", "", 1);
nullCapableSetenv("SILO_PANIC", old_env, 1);
}

// NOLINTNEXTLINE(readability-identifier-naming,readability-function-cognitive-complexity)
Expand All @@ -65,12 +75,14 @@ TEST(panic, debugAssertBehavesAsPerCompilationMode) {

#if DEBUG_ASSERTIONS

const char* old_env = getenv("SILO_PANIC");
setenv("SILO_PANIC", "", 1);
try {
DEBUG_ASSERT(1 + 1 == 3);
} catch (const std::exception& ex) {
assertMsg(ex.what(), "DEBUG_ASSERT failure: 1 + 1 == 3");
};
nullCapableSetenv("SILO_PANIC", old_env, 1);

#else
// check that DEBUG_ASSERT is disabled
Expand All @@ -87,12 +99,14 @@ TEST(panic, debugAssertGeWorks) {

#if DEBUG_ASSERTIONS

const char* old_env = getenv("SILO_PANIC");
setenv("SILO_PANIC", "", 1);
try {
DEBUG_ASSERT_GE(1 + 5, 7);
} catch (const std::exception& ex) {
assertMsg(ex.what(), "DEBUG_ASSERT_GE failure: 1 + 5 >= 7: 6 >= 7");
};
Comment on lines 104 to 108
Copy link
Contributor

Choose a reason for hiding this comment

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

Those tests are not ideal, because they will also run green if DEBUG_ASSERT_GE doesn't throw (and I assume that the tests are supposed to check that it throws)

It would be better to use

   EXPECT_THAT(
      []() {
         executeSomething();
      },
      ThrowsMessage<ConfigException>(
         ::testing::HasSubstr("...")
      )
   );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be better. The panics should always either throw or abort though, I'm not that worried that they wouldn't do either, basically the test here is that it doesn't abort (and checks the message at the same time). But it would be better. But that will have to be another PR (it's independent), and low priority, I'll just add a note on my side.

nullCapableSetenv("SILO_PANIC", old_env, 1);

#else
// check that DEBUG_ASSERT is disabled
Expand Down