fix: panic.test: actually revert env var to original value #602
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There is no change in the changelog. This PR will not produce a new releasable version. |
pflanze
force-pushed
the
fix_panic_test
branch
from
3cbc153
to
ae2eda0
Compare
fengelniederhammer
requested changes
Oct 7, 2024
pflanze
force-pushed
the
fix_panic_test
branch
from
ae2eda0
to
d2e9fb9
Compare
fengelniederhammer
approved these changes
Oct 9, 2024
Comment on lines
104
to
108
| 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.
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.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.