Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57555 )
Change subject: tests: Add lib/lzma-test test case ......................................................................
Patch Set 7:
(9 comments)
Patchset:
PS7: Sorry for the long review delay.
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/57555/comment/69bcf91f_9bc78c25 PS7, Line 240: lzma-test-cflags += -I "$(src)/lib" Why not just write `#include <lib/lzmadecode.h> in the code? I think that would make it easier to follow where that file is found.
https://review.coreboot.org/c/coreboot/+/57555/comment/6a188a3b_2817baa3 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.inc and use that for all tests that need this kind of thing? (I'd like to reduce the amount of special magic that is hidden in test-specific cflags to the minimum necessary.)
File tests/lib/lzma-test.c:
https://review.coreboot.org/c/coreboot/+/57555/comment/e89e2ca3_748d0810 PS7, Line 52: goto finish; Weird way to write `return 1`?
https://review.coreboot.org/c/coreboot/+/57555/comment/9443c5f0_5248a00b 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.
https://review.coreboot.org/c/coreboot/+/57555/comment/b74ac85e_a3ff3b31 PS7, Line 65: goto error; Nope. teardown() expects *state to point to s, but at this point it still points to fname_base. (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.)
https://review.coreboot.org/c/coreboot/+/57555/comment/9b5a14f0_0563f1d9 PS7, Line 88: goto finish; nit: just write `return 0`?
https://review.coreboot.org/c/coreboot/+/57555/comment/6b28f991_f4d2eded PS7, Line 154: ( Do these parens do anything? (Also, in general please prefer struct initializers with designated member names rather than just relying on order, as it's easier to read... e.g. .name = "test...(...)", .test_func = test_ulzman_correct_file, etc.)
https://review.coreboot.org/c/coreboot/+/57555/comment/0c09ec9d_a6ad3726 PS7, Line 161: ULZMAN_CORRECT_FILE_TEST("data.1"), It would be good if somewhere (e.g. maybe here, with line comments?) it was documented what these 4 test files each represent.