Disable EXPECT_N_LEAKS() if memleak detection disabled #1009
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
|
The failure is not intended, but it would sort-of make sense to fail, or at least print a warning. However, the fix isn't ok. You'd need to work on the check, and if possible, do it runtime and not compile time. Also there is no test provided. So, as is, I won't accept this pull request. |
|
Ok, thanks. I really just wanted to get feedback on whether or not a failure was intended. I can fix this up if you think printing a warning would be acceptable while allowing other tests to continue running. |
bkylerussell
force-pushed
the
memleak
branch
3 times, most recently
from
83c35af
to
cd9f2fd
Compare
Print a warning message during tests that expect leaks when CppUTest has been configured with --disable-memory-leak-detection. This permits the rest of a test's value to remain in effect if the desired configuration is really to disable memory leak detection.
bkylerussell
force-pushed
the
memleak
branch
from
cd9f2fd
to
768e048
Compare
|
I think I've fixed this up per the previous discussion so that a warning is printed and the change is accompanied by a test. Checks seem to be happy now. |
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.
EXPECT_N_LEAKS() can't be supported when CppUTest is configured with --disable-memory-leak-detection, since there's nothing to make sure we hit the expected number of leaks.
In one sense, it would be nice for tests that use EXPECT_N_LEAKS() to go ahead and continue running with whatever capabilities CppUTest is configured with. This change would allow that behavior.
But on the other hand, I guess someone that really expected leaks might want to see the failure to know that CppUTest wasn't configured like they expected.
Is the intended behavior for tests using EXPECT_N_LEAKS() to fail when CppUTest is configured with --disable-memory-leak-detection, or was this just a miss? Thoughts?