GNOME Bugzilla – Bug 752123
harness: don't run code inside g_assert()
Last modified: 2015-08-16 13:40:01 UTC
Created attachment 307072 [details] [review] proposed patch Submitting this here for review in case anybody is against it.
Disabling asserts in unit tests makes no sense.
Doing things with side-effects in assertions is generally bad style though
Not clear to me this is a huge improvement over the existing code. And in case something does fail you now get less information printed (just the variable name, not the function plus parameters).
Olivier, in the case of GstHarness you can't disable them. They are forced in. Sebastian, I agree. Also strange to read. Tim, OH! didn't realize this. As it stands I feel inclined to mark this as rejected. Keeping it open for a little while so other people can comment.
Could use the fail_unless() from gstcheck instead ?
Yeah, fail_unless() would be nicer than g_assert(). Some of our other unit tests using g_assert() always annoys me ;)
However, there is a difference in fail_unless and g_assert in that fail_unless will keep going (in some cases) where g_assert will stop. Now, when writing a test fail_unless is always the right answer, but you are not "testing" the code inside the harness, and this is never supposed to fail, hence the assert.
Review of attachment 307072 [details] [review]: Marking the patch as rejected.
Review of attachment 307072 [details] [review]: After some discussion in IRC. It was decided to push this change. commit 60de1f26c78feb0cde6d3f82cf86cf35daa71cc0 Author: Luis de Bethencourt <luis@debethencourt.com> Date: Wed Aug 5 14:05:25 2015 +0100 harness: don't run code inside g_assert Even though asserts can't be disabled in GstHarness, Coverity still complains about running code inside them. Moving the code to outside the g_asserts(). CID #1311326, #1311327, #1311328