-
Notifications
You must be signed in to change notification settings - Fork 247
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
Add Valgrind plugin #807
base: test/ceedling_0_32_rc
Are you sure you want to change the base?
Add Valgrind plugin #807
Conversation
It would be better at this point if this were targeting the prerelease branch that is soon to be released. |
fa1a9fe
to
735b354
Compare
735b354
to
a3ba518
Compare
@mvandervoord Sure, I have changed the base branch of the pull request to test/ceedling_0_32_rc. I have also added a simple README file for the Valgrind plugin in the latest version of the commit. |
Why is this still not merged ? |
@PasVegan Because it hasn't been reviewed yet. Have you tried the plugin? How was your experience? |
Yes I tried the plugin and it works very nicely, the only thing that I have to say is that even if valgrind reports errors the exit code is still 0 at the end. I think that need to be changed for proper CI use. |
Hi @PasVegan, you can add the following flags to let Valgrind exit on the first error and return a different exit code:
In a CI context, it makes sense to add said flags. If you feel that those flags should be the default options, I can add them (with the default exit code on error being 1). |
Hi, yes I think those flags should be on by default with the default exit code on error being 1 👍🏻 |
This commit adds the Valgrind plugin to Ceedling. The plugin implementation closely follows the Gcov plugin. Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
Done! Added the 2 aforementioned flags in the latest commit (68fe5c3). |
This commit adds the Valgrind plugin to Ceedling. The plugin implementation closely follows the Gcov plugin.
Fixes #277.