-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: panic.test: actually revert env var to original value #602
Conversation
There is no change in the changelog. This PR will not produce a new releasable version. |
3cbc153
to
ae2eda0
Compare
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 commit message should be something like "test" - this should not trigger a new release.
ae2eda0
to
d2e9fb9
Compare
try { | ||
DEBUG_ASSERT_GE(1 + 5, 7); | ||
} catch (const std::exception& ex) { | ||
assertMsg(ex.what(), "DEBUG_ASSERT_GE failure: 1 + 5 >= 7: 6 >= 7"); | ||
}; |
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.
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("...")
)
);
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.
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.
The tests did set the environment variable SILO_PANIC to "" even if it
was "abort", hence debugging e.g. in GDB (when setting that variable
to "abort") didn't work if one of these tests ran before the issue to
debug was triggered.
PR Checklist
- [ ] All necessary documentation has been adapted or there is an issue to do so.- [ ] The implemented feature is covered by an appropriate test.