Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57555 )
Change subject: tests: Add lib/lzma-test test case ......................................................................
Patch Set 8:
(8 comments)
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/57555/comment/a95aff3e_9a43460f PS7, Line 240: lzma-test-cflags += -I "$(src)/lib"
Why not just write `#include <lib/lzmadecode. […]
Done
https://review.coreboot.org/c/coreboot/+/57555/comment/fceae1d6_e22d855e PS7, Line 241: lzma-test-cflags += -DLZMA_TEST_DATA_PATH="$(testsrc)/data/lib/lzma-test/"
Maybe just define a `-DTEST_DATA_DIR="$(testsrc)/data" in the base tests/Makefile. […]
Done
File tests/lib/lzma-test.c:
https://review.coreboot.org/c/coreboot/+/57555/comment/c023a46e_ffbc0468 PS7, Line 52: goto finish;
Weird way to write `return 1`?
Done
https://review.coreboot.org/c/coreboot/+/57555/comment/84f09922_3f2778ac PS7, Line 57: s->raw_filename = test_malloc(raw_filename_size);
nit: you can just use asprintf() to make all of this a lot easier.
Oh, I didn't know this function existed. But, unfortunately, coreboot does not have prototype of this function in its headers (and these headers are overriding system ones), so I am not able to use it. I do not want to copy prototype of every libc function missing form coreboot headers.
https://review.coreboot.org/c/coreboot/+/57555/comment/2006541d_799bcbe5 PS7, Line 65: goto error;
Nope. teardown() expects *state to point to s, but at this point it still points to fname_base.
If setup function exits with non-zero exit code, the teardown function is not called by CMocka. I am calling it in the error section and passing correct pointer `(void **)&s)` before returning from setup function.
In general, I don't think you need to worry about allocation cleanup in error cases that much, the program is gonna exit soon after anyway... I think it's fine to just return here. Just don't make it segfault.
One test setup failing does not make other tests fail/skip instantly. In this test we can skip error handling and just return. But in some cases we still should handle them. If we are using test_malloc(), CMocka will report non-freed memory blocks if we return without calling test_free().
https://review.coreboot.org/c/coreboot/+/57555/comment/95e29e29_0b2f6178 PS7, Line 88: goto finish;
nit: just write `return 0`?
Done
https://review.coreboot.org/c/coreboot/+/57555/comment/7324b887_aec43c0f PS7, Line 154: (
Do these parens do anything? (Also, in general please prefer struct initializers with designated mem […]
I was following convention of CMocma macro cmocka_unit_test_prestate_setup_teardown(). But yes, it looks better, when written with member names.
https://review.coreboot.org/c/coreboot/+/57555/comment/0d4507de_a5c2ea64 PS7, Line 161: ULZMAN_CORRECT_FILE_TEST("data.1"),
It would be good if somewhere (e.g. […]
Done