Change in flashrom[master]: Enable dynamic memory allocation checks for cmocka unit tests
Attention is currently required from: David Hendricks, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51243 ) Change subject: Enable dynamic memory allocation checks for cmocka unit tests ...................................................................... Patch Set 6: Code-Review+1 (5 comments) Patchset: PS6:
Even better news: now after adding includes in meson. […] This looks quite nice now, thank you!
Only one remark, the -DUNIT_TESt_ENV. Do we still need it? About `unittest.h`, I'm not sure if I fully understood its role. Maybe the idea is to not test allocations that are done by the tests itself? i.e. the tests itself wouldn't have to be error free, only the tested code has to be. As long as the tests work, that's fine. Just something to keep in mind if the tests ever get more complex. File meson.build: https://review.coreboot.org/c/flashrom/+/51243/comment/7097eb6b_0bc3a179 PS6, Line 477: -include Can we have spaces after -include? That should work, I guess :) File tests/flashrom.c: https://review.coreboot.org/c/flashrom/+/51243/comment/4b97477c_1f95e59c PS2, Line 27: #define assert_string_not_equal_with_free(text, expected) \
I tried to make it better, dropped _string (7 chars) and replaced free->and (1 char) so in total I r […] Looks better indeed.
File tests/helpers.c: https://review.coreboot.org/c/flashrom/+/51243/comment/038b3d6e_0dca9457 PS6, Line 21: #include <stdlib.h> I'm not sure if we should remove this. The files under tests/ shouldn't have the -include arguments, right? It's probably indirectly included by one of the header files above. File tests/meson.build: https://review.coreboot.org/c/flashrom/+/51243/comment/5b850bfb_8cc9a3bc PS6, Line 38: '-DUNIT_TEST_ENV', Is this still needed? Now that `unittest_env.h` is only when building for tests, it seems redundant. -- To view, visit https://review.coreboot.org/c/flashrom/+/51243 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0c6b6b8dc17aaee28640e3fca3d1fc9f7feabf5f Gerrit-Change-Number: 51243 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Daniel Kurtz <djkurtz@google.com> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Daniel Kurtz <djkurtz@chromium.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-CC: Thomas Heijligen <src@posteo.de> Gerrit-CC: Victor Ding <victording@chromium.org> Gerrit-Attention: David Hendricks <david.hendricks@gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Attention: Angel Pons <th3fanbus@gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm@chromium.org> Gerrit-Comment-Date: Tue, 30 Mar 2021 22:15:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm@chromium.org> Gerrit-MessageType: comment
participants (1)
-
Nico Huber (Code Review)