| Message ID | cover.1723791831.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce clar testing framework | expand |
Hi Patrick On 16/08/2024 08:04, Patrick Steinhardt wrote: > Hi, > > this is the fifth version of my patch series that introduces the clar > testing framework for our unit tests. Thanks for working on this, I'm broadly in favor of this change. I like the way it keeps each test as a function and adds automatic test registration with support for setup and teardown functions. I am keen though to keep an emphasis on good diagnostic messages when tests fail. Looking at the conversions in this series all of the test_msg() lines that provide useful debugging context are removed. I'm not sure using yaml to report errors rather than human readable messages is an improvement either. I wonder if we want to either improve the assertions offered by clar or write our own. I find the names of the cl_assert_equal_?() functions are a bit cumbersome. The aim of the check_* names was to try and be both concise and descriptive. Adding our own check_* macros on top of clar would also make it easier to port our existing tests. Here are some thought having read through the assertion and error reporting code: - As I think you've pointed out elsewhere there are no equivalents for check_int(a, <|<=|>|>=, b) so we're forced to use cl_assert() and forego the better diagnostic messages that come from a dedicated comparison macro. We should fix this as a priority. - cl_assert_equal_i() casts its arguments to int whereas check_int() and check_uint() are careful to avoid truncation and keep the original signedness (if that's a word). I think that's unlikely to be a problem with our current test but could trip us up in the future. - cl_assert_equal_s() prints each argument as-is. This means that it passes NULL arguments through to snprintf() which is undefined according to the C standard. Compare this to check_str() that is NULL safe and is careful to escape control characters and add delimiters to the beginning and end of the string to make it obvious when a string contains leading or trailing whitespace. - The cl_assert_equal_?() macros lack type safety for the arguments being compared as they are wrappers around a variadic function. That could be fixed by having each macros wrap a dedicated function that wraps clar__fail(). - There is no equivalent of test_todo() to mark assertions that are expected to fail. We're not using that yet in our tests but our experience with the integration tests suggests that we are likely to want this in the future. - To me the "sandbox" feature is mis-named as it does not provide any confinement. It is instead a useful mechanism for running a test in a temporary directory created from a template. - There are no checks for failing memory allocations - the return value of calloc() and strdup() are used without checking for NULL. - The use of longjmp is a bit of a double edged sword as it makes it easy to leak resources on test failures. Best Wishes Phillip > Changes compared to v4: > > - The whitespace fixes have been merged upstream, so I've updated the > embedded copy of clar and dropped the subsequent patch that fixed > them in our copy. The NonStop compatibility fixes have not yet been > merged as the pull request needs some more work. > > - Both "clar-decls.h" and "clar.suite" are now part of GENERATED_H. > This brings removal of these files via "make clean" for free. > > - The "sparse" target already depends on GENERATED_H, but in a broken > way. I've fixed that in a new commit. > > - The "sparse" target no longer checks external sources, including the > clar sources. > > - The "hdr-check" target now depends on GENERATED_H, as well. This > avoids having to manually wire up dependencies on generated headers > per file, which seems rather unmaintainable to me. > > With this, the "hdr-check" and "sparse" targets all work on my machine > now. > > Thanks! > > Patrick > > Patrick Steinhardt (9): > t: do not pass GIT_TEST_OPTS to unit tests with prove > t: import the clar unit testing framework > t/clar: fix compatibility with NonStop > Makefile: fix sparse dependency on GENERATED_H > Makefile: make hdr-check depend on generated headers > Makefile: do not use sparse on third-party sources > Makefile: wire up the clar unit testing framework > t/unit-tests: convert strvec tests to use clar > t/unit-tests: convert ctype tests to use clar > > .gitignore | 1 + > Documentation/technical/unit-tests.txt | 2 + > Makefile | 53 +- > t/Makefile | 4 +- > t/run-test.sh | 2 +- > t/unit-tests/.gitignore | 2 + > t/unit-tests/clar-generate.awk | 50 ++ > t/unit-tests/clar/.github/workflows/ci.yml | 23 + > t/unit-tests/clar/COPYING | 15 + > t/unit-tests/clar/README.md | 329 ++++++++ > t/unit-tests/clar/clar.c | 842 +++++++++++++++++++++ > t/unit-tests/clar/clar.h | 173 +++++ > t/unit-tests/clar/clar/fixtures.h | 50 ++ > t/unit-tests/clar/clar/fs.h | 522 +++++++++++++ > t/unit-tests/clar/clar/print.h | 211 ++++++ > t/unit-tests/clar/clar/sandbox.h | 159 ++++ > t/unit-tests/clar/clar/summary.h | 143 ++++ > t/unit-tests/clar/generate.py | 266 +++++++ > t/unit-tests/clar/test/.gitignore | 4 + > t/unit-tests/clar/test/Makefile | 39 + > t/unit-tests/clar/test/clar_test.h | 16 + > t/unit-tests/clar/test/main.c | 40 + > t/unit-tests/clar/test/main.c.sample | 27 + > t/unit-tests/clar/test/resources/test/file | 1 + > t/unit-tests/clar/test/sample.c | 84 ++ > t/unit-tests/{t-ctype.c => ctype.c} | 71 +- > t/unit-tests/{t-strvec.c => strvec.c} | 119 ++- > t/unit-tests/unit-test.c | 17 + > t/unit-tests/unit-test.h | 3 + > 29 files changed, 3166 insertions(+), 102 deletions(-) > create mode 100644 t/unit-tests/clar-generate.awk > create mode 100644 t/unit-tests/clar/.github/workflows/ci.yml > create mode 100644 t/unit-tests/clar/COPYING > create mode 100644 t/unit-tests/clar/README.md > create mode 100644 t/unit-tests/clar/clar.c > create mode 100644 t/unit-tests/clar/clar.h > create mode 100644 t/unit-tests/clar/clar/fixtures.h > create mode 100644 t/unit-tests/clar/clar/fs.h > create mode 100644 t/unit-tests/clar/clar/print.h > create mode 100644 t/unit-tests/clar/clar/sandbox.h > create mode 100644 t/unit-tests/clar/clar/summary.h > create mode 100755 t/unit-tests/clar/generate.py > create mode 100644 t/unit-tests/clar/test/.gitignore > create mode 100644 t/unit-tests/clar/test/Makefile > create mode 100644 t/unit-tests/clar/test/clar_test.h > create mode 100644 t/unit-tests/clar/test/main.c > create mode 100644 t/unit-tests/clar/test/main.c.sample > create mode 100644 t/unit-tests/clar/test/resources/test/file > create mode 100644 t/unit-tests/clar/test/sample.c > rename t/unit-tests/{t-ctype.c => ctype.c} (71%) > rename t/unit-tests/{t-strvec.c => strvec.c} (54%) > create mode 100644 t/unit-tests/unit-test.c > create mode 100644 t/unit-tests/unit-test.h > > Range-diff against v4: > 1: 086dd728a7 ! 1: 832dc0496f t: do not pass GIT_TEST_OPTS to unit tests with prove > @@ Commit message > environment variable. Like this, we can conditionally forward it to our > test scripts, only. > > + Signed-off-by: Patrick Steinhardt <ps@pks.im> > + > ## t/Makefile ## > @@ t/Makefile: failed: > test -z "$$failed" || $(MAKE) $$failed > 2: 5c22e0b3b9 ! 2: 3690607933 t: import the clar unit testing framework > @@ Metadata > ## Commit message ## > t: import the clar unit testing framework > > - Import the clar unit testing framework at commit faa8419 (Merge pull > - request #93 from clar-test/ethomson/fixtures, 2023-12-14). The framework > + Import the clar unit testing framework at commit 1516124 (Merge pull > + request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework > will be wired up in subsequent commits. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > @@ t/unit-tests/clar/clar/fs.h (new) > + ERROR_PATH_NOT_FOUND == last_error) > + return 0; > + > -+ Sleep(RM_RETRY_DELAY * retries * retries); > ++ Sleep(RM_RETRY_DELAY * retries * retries); > + } > + while (retries++ <= RM_RETRY_COUNT); > + > @@ t/unit-tests/clar/clar/sandbox.h (new) > + static const size_t var_count = 5; > + static const char *env_vars[] = { > + "CLAR_TMP", "TMPDIR", "TMP", "TEMP", "USERPROFILE" > -+ }; > ++ }; > + > -+ size_t i; > ++ size_t i; > + > + for (i = 0; i < var_count; ++i) { > + const char *env = getenv(env_vars[i]); > @@ t/unit-tests/clar/clar/sandbox.h (new) > +{ > + return _clar_path; > +} > -+ > > ## t/unit-tests/clar/clar/summary.h (new) ## > @@ > @@ t/unit-tests/clar/generate.py (new) > + suite.disable(options.excluded) > + if suite.write(): > + print("Written `clar.suite` (%d tests in %d suites)" % (suite.callback_count(), suite.suite_count())) > -+ > > ## t/unit-tests/clar/test/.gitignore (new) ## > @@ > @@ t/unit-tests/clar/test/.gitignore (new) > +.clarcache > +clar_test > +*.o > -+ > > ## t/unit-tests/clar/test/Makefile (new) ## > @@ > 4: 75e097dfa4 = 3: db53673294 t/clar: fix compatibility with NonStop > 3: e0f99874cc ! 4: b6199c88dd t/clar: fix whitespace errors > @@ Metadata > Author: Patrick Steinhardt <ps@pks.im> > > ## Commit message ## > - t/clar: fix whitespace errors > - > - Fix whitespace errors in the clar that make git-apply(1) unhappy. This > - has been cherry-picked from the upstream pull request at [1]. > - > - [1]: https://github.com/clar-test/clar/pull/97 > + Makefile: fix sparse dependency on GENERATED_H > + > + The "check" Makefile target is essentially an alias around the "sparse" > + target. The one difference though is that it will tell users to instead > + run the "test" target in case they do not have sparse(1) installed, as > + chances are high that they wanted to execute the test suite rather than > + doing semantic checks. > + > + But even though the "check" target ultimately just ends up executing > + `make sparse`, it still depends on our generated headers. This does not > + make any sense though: they are irrelevant for the "test" target advice, > + and if these headers are required for the "sparse" target they must be > + declared as a dependency on the aliased target, not the alias. > + > + But even moving the dependency to the "sparse" target is wrong, as > + concurrent builds may then end up generating the headers and running > + sparse concurrently. Instead, we make them a dependency of the specific > + objects. While that is overly broad, it does ensure correct ordering. > + The alternative, specifying which file depends on what generated header > + explicitly, feels rather unmaintainable. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > - ## t/unit-tests/clar/clar/fs.h ## > -@@ t/unit-tests/clar/clar/fs.h: fs_rm_wait(WCHAR *_wpath) > - ERROR_PATH_NOT_FOUND == last_error) > - return 0; > - > -- Sleep(RM_RETRY_DELAY * retries * retries); > -+ Sleep(RM_RETRY_DELAY * retries * retries); > - } > - while (retries++ <= RM_RETRY_COUNT); > + ## Makefile ## > +@@ Makefile: check-sha1:: t/helper/test-tool$X > > - > - ## t/unit-tests/clar/clar/sandbox.h ## > -@@ t/unit-tests/clar/clar/sandbox.h: find_tmp_path(char *buffer, size_t length) > - static const size_t var_count = 5; > - static const char *env_vars[] = { > - "CLAR_TMP", "TMPDIR", "TMP", "TEMP", "USERPROFILE" > -- }; > -+ }; > + SP_OBJ = $(patsubst %.o,%.sp,$(OBJECTS)) > > -- size_t i; > -+ size_t i; > +-$(SP_OBJ): %.sp: %.c %.o > ++$(SP_OBJ): %.sp: %.c %.o $(GENERATED_H) > + $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ > + -Wsparse-error \ > + $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< && \ > +@@ Makefile: style: > + git clang-format --style file --diff --extensions c,h > > - for (i = 0; i < var_count; ++i) { > - const char *env = getenv(env_vars[i]); > -@@ t/unit-tests/clar/clar/sandbox.h: const char *clar_sandbox_path(void) > - { > - return _clar_path; > - } > -- > - > - ## t/unit-tests/clar/generate.py ## > -@@ t/unit-tests/clar/generate.py: def write(self): > - suite.disable(options.excluded) > - if suite.write(): > - print("Written `clar.suite` (%d tests in %d suites)" % (suite.callback_count(), suite.suite_count())) > -- > - > - ## t/unit-tests/clar/test/.gitignore ## > -@@ t/unit-tests/clar/test/.gitignore: clar.suite > - .clarcache > - clar_test > - *.o > -- > + .PHONY: check > +-check: $(GENERATED_H) > ++check: > + @if sparse; \ > + then \ > + echo >&2 "Use 'make sparse' instead"; \ > -: ---------- > 5: 06364b2b72 Makefile: make hdr-check depend on generated headers > -: ---------- > 6: 88ea94ce16 Makefile: do not use sparse on third-party sources > 5: 5b8a64ae79 ! 7: 05bcb5bef6 Makefile: wire up the clar unit testing framework > @@ .gitignore > /bin-wrappers/ > > ## Makefile ## > +@@ Makefile: REFTABLE_TEST_LIB = reftable/libreftable_test.a > + GENERATED_H += command-list.h > + GENERATED_H += config-list.h > + GENERATED_H += hook-list.h > ++GENERATED_H += $(UNIT_TEST_DIR)/clar-decls.h > ++GENERATED_H += $(UNIT_TEST_DIR)/clar.suite > + > + .PHONY: generated-hdrs > + generated-hdrs: $(GENERATED_H) > @@ Makefile: THIRD_PARTY_SOURCES += sha1dc/% > THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/% > THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/% > @@ Makefile: endif > > bin-wrappers/%: wrap-for-bin.sh > $(call mkdir_p_parent_template) > -@@ Makefile: CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(LIB_H)) > - HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) > - HCC = $(HCO:hco=hcc) > - > -+$(UNIT_TEST_DIR)/unit-test.hcc: $(UNIT_TEST_DIR)/unit-test.h $(UNIT_TEST_DIR)/clar-decls.h > - %.hcc: %.h > - @echo '#include "git-compat-util.h"' >$@ > - @echo '#include "$<"' >>$@ > @@ Makefile: endif > > artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \ > @@ Makefile: cocciclean: > > clean: profile-clean coverage-clean cocciclean > $(RM) -r .build $(UNIT_TEST_BIN) > -+ $(RM) GIT-TEST-SUITES $(UNIT_TEST_DIR)/clar.suite $(UNIT_TEST_DIR)/clar-decls.h > ++ $(RM) GIT-TEST-SUITES > $(RM) po/git.pot po/git-core.pot > $(RM) git.res > $(RM) $(OBJECTS) > 6: bc4e23d666 = 8: 8f56b4d626 t/unit-tests: convert strvec tests to use clar > 7: 0a7fe8775a = 9: ca09d19fd5 t/unit-tests: convert ctype tests to use clar > > base-commit: 406f326d271e0bacecdb00425422c5fa3f314930
On Fri, Aug 16, 2024 at 02:37:34PM +0100, Phillip Wood wrote: > Hi Patrick > > On 16/08/2024 08:04, Patrick Steinhardt wrote: > > Hi, > > > > this is the fifth version of my patch series that introduces the clar > > testing framework for our unit tests. > > Thanks for working on this, I'm broadly in favor of this change. I > like the way it keeps each test as a function and adds automatic test > registration with support for setup and teardown functions. I am keen > though to keep an emphasis on good diagnostic messages when tests > fail. Looking at the conversions in this series all of the test_msg() > lines that provide useful debugging context are removed. I'm not sure > using yaml to report errors rather than human readable messages is an > improvement either. > > I wonder if we want to either improve the assertions offered by clar > or write our own. I find the names of the cl_assert_equal_?() > functions are a bit cumbersome. The aim of the check_* names was to > try and be both concise and descriptive. Adding our own check_* macros > on top of clar would also make it easier to port our existing tests. > > Here are some thought having read through the assertion and error > reporting code: > > - As I think you've pointed out elsewhere there are no equivalents > for check_int(a, <|<=|>|>=, b) so we're forced to use cl_assert() > and forego the better diagnostic messages that come from a > dedicated comparison macro. We should fix this as a priority. Agreed, this one also feels rather limiting to me. Are you okay with me doing this as a follow-up in case this series lands? > - cl_assert_equal_i() casts its arguments to int whereas check_int() > and check_uint() are careful to avoid truncation and keep the > original signedness (if that's a word). I think that's unlikely to > be a problem with our current test but could trip us up in the > future. Yeah. If it ever becomes a problem we can likely just introduce something like `cl_assert_equal_u()` to achieve the same for unsigned. Both should probably end up casting to `intmax_t` and `uintmax_t`, respectively. > - cl_assert_equal_s() prints each argument as-is. This means > that it passes NULL arguments through to snprintf() which is > undefined according to the C standard. Compare this to check_str() > that is NULL safe and is careful to escape control characters and > add delimiters to the beginning and end of the string to make it > obvious when a string contains leading or trailing whitespace. Good point indeed, and something I'm happy to fix upstream. > - The cl_assert_equal_?() macros lack type safety for the arguments > being compared as they are wrappers around a variadic function. > That could be fixed by having each macros wrap a dedicated > function that wraps clar__fail(). Some of them do indeed, others generate issues. I don't think we have to have dedicated functions, but could do something about this with `__attribute__((format (printf, ...)))`. > - There is no equivalent of test_todo() to mark assertions that are > expected to fail. We're not using that yet in our tests but our > experience with the integration tests suggests that we are likely > to want this in the future. Heh, funny that you mention this. I had this discussion some 6 years ago I think, where I also mentioned that this should exist as a feature. In any case, I agree. > - To me the "sandbox" feature is mis-named as it does not provide any > confinement. It is instead a useful mechanism for running a test in > a temporary directory created from a template. I guess we'll either just have to not use it or ignore that it's named a bit awkwardly. Changing this in clar probably wouldn't work well because other projects may depend on it. > - There are no checks for failing memory allocations - the return > value of calloc() and strdup() are used without checking for NULL. I'll commit to fixing this upstream if this lands. > - The use of longjmp is a bit of a double edged sword as it makes it > easy to leak resources on test failures. I have to say that this is one of the best features of the clar to me. The current test framework we use doesn't, which in theory requires you to always `return` whenever there was an error. But that results in code that is both awful to read and write, so for most of the tests simply don't bother at all. And consequently, the tests are quite likely to cause segfaults once one of the checks fails because we didn't abort running the testcase, but things are broken. In practice, I'd claim that you don't typically care all that much about memory leaks once your basic assertions start to fail. So, things that need addressing and that I'm happy to do as follow ups: - Introduce functions that compare integers. - Improve type safety of the `cl_assert_equal_?()` macros. - Adapt `cl_assert_equal_s()` to handle NULL pointers. - Introduce checks for failing memory allocations. Nice to have would be support for known-failing tests. Patrick
Hi Patrick On 20/08/2024 13:59, Patrick Steinhardt wrote: > On Fri, Aug 16, 2024 at 02:37:34PM +0100, Phillip Wood wrote: >> Hi Patrick >> >> On 16/08/2024 08:04, Patrick Steinhardt wrote: >> >> - As I think you've pointed out elsewhere there are no equivalents >> for check_int(a, <|<=|>|>=, b) so we're forced to use cl_assert() >> and forego the better diagnostic messages that come from a >> dedicated comparison macro. We should fix this as a priority. > > Agreed, this one also feels rather limiting to me. Are you okay with me > doing this as a follow-up in case this series lands? Yes >> - cl_assert_equal_i() casts its arguments to int whereas check_int() >> and check_uint() are careful to avoid truncation and keep the >> original signedness (if that's a word). I think that's unlikely to >> be a problem with our current test but could trip us up in the >> future. > > Yeah. If it ever becomes a problem we can likely just introduce > something like `cl_assert_equal_u()` to achieve the same for unsigned. > Both should probably end up casting to `intmax_t` and `uintmax_t`, > respectively. Supporting wider arguments would make sense. At the moment clar__assert_equal() does not support PRIiMAX, only the non-standard PRIuZ. >> - cl_assert_equal_s() prints each argument as-is. This means >> that it passes NULL arguments through to snprintf() which is >> undefined according to the C standard. Compare this to check_str() >> that is NULL safe and is careful to escape control characters and >> add delimiters to the beginning and end of the string to make it >> obvious when a string contains leading or trailing whitespace. > > Good point indeed, and something I'm happy to fix upstream. That's great >> - The cl_assert_equal_?() macros lack type safety for the arguments >> being compared as they are wrappers around a variadic function. >> That could be fixed by having each macros wrap a dedicated >> function that wraps clar__fail(). > > Some of them do indeed, others generate issues. I don't think we have to > have dedicated functions, but could do something about this with > `__attribute__((format (printf, ...)))`. I wondered about suggesting '__attribute__((format (printf, ...)))' but we'd need to double up the format argument in order to use it which is kind of messy. At the moment we pass "%i" with two integers. >> - There is no equivalent of test_todo() to mark assertions that are >> expected to fail. We're not using that yet in our tests but our >> experience with the integration tests suggests that we are likely >> to want this in the future. > > Heh, funny that you mention this. I had this discussion some 6 years ago > I think, where I also mentioned that this should exist as a feature. In > any case, I agree. Excellent! >> - To me the "sandbox" feature is mis-named as it does not provide any >> confinement. It is instead a useful mechanism for running a test in >> a temporary directory created from a template. > > I guess we'll either just have to not use it or ignore that it's named a > bit awkwardly. Changing this in clar probably wouldn't work well because > other projects may depend on it. Yes it's probably too late to rename it. I think being able to create a test directory from a template directory could be useful, we just need to be mindful that the test code is not confined by a sandbox. >> - There are no checks for failing memory allocations - the return >> value of calloc() and strdup() are used without checking for NULL. > > I'll commit to fixing this upstream if this lands. Great >> - The use of longjmp is a bit of a double edged sword as it makes it >> easy to leak resources on test failures. > > I have to say that this is one of the best features of the clar to me. > The current test framework we use doesn't, which in theory requires you > to always `return` whenever there was an error. But that results in code > that is both awful to read and write, so for most of the tests simply > don't bother at all. And consequently, the tests are quite likely to > cause segfaults once one of the checks fails because we didn't abort > running the testcase, but things are broken. I thought that the tests took care to bail out early where it made sense. Sometimes it is useful to continue for example when checking an strbuf we might want to check alloc and len before bailing out. We're probably not losing much by not doing that though. > In practice, I'd claim that you don't typically care all that much about > memory leaks once your basic assertions start to fail. I tend to agree. I was thinking more about exhausting fds and cleaning up files but that's probably not a big issue in practice. > So, things that need addressing and that I'm happy to do as follow ups: > > - Introduce functions that compare integers. > > - Improve type safety of the `cl_assert_equal_?()` macros. > > - Adapt `cl_assert_equal_s()` to handle NULL pointers. > > - Introduce checks for failing memory allocations. > > Nice to have would be support for known-failing tests. This all sounds good to me Sorry for missing this mail earlier. Phillip > Patrick
Hi, this is the fifth version of my patch series that introduces the clar testing framework for our unit tests. Changes compared to v4: - The whitespace fixes have been merged upstream, so I've updated the embedded copy of clar and dropped the subsequent patch that fixed them in our copy. The NonStop compatibility fixes have not yet been merged as the pull request needs some more work. - Both "clar-decls.h" and "clar.suite" are now part of GENERATED_H. This brings removal of these files via "make clean" for free. - The "sparse" target already depends on GENERATED_H, but in a broken way. I've fixed that in a new commit. - The "sparse" target no longer checks external sources, including the clar sources. - The "hdr-check" target now depends on GENERATED_H, as well. This avoids having to manually wire up dependencies on generated headers per file, which seems rather unmaintainable to me. With this, the "hdr-check" and "sparse" targets all work on my machine now. Thanks! Patrick Patrick Steinhardt (9): t: do not pass GIT_TEST_OPTS to unit tests with prove t: import the clar unit testing framework t/clar: fix compatibility with NonStop Makefile: fix sparse dependency on GENERATED_H Makefile: make hdr-check depend on generated headers Makefile: do not use sparse on third-party sources Makefile: wire up the clar unit testing framework t/unit-tests: convert strvec tests to use clar t/unit-tests: convert ctype tests to use clar .gitignore | 1 + Documentation/technical/unit-tests.txt | 2 + Makefile | 53 +- t/Makefile | 4 +- t/run-test.sh | 2 +- t/unit-tests/.gitignore | 2 + t/unit-tests/clar-generate.awk | 50 ++ t/unit-tests/clar/.github/workflows/ci.yml | 23 + t/unit-tests/clar/COPYING | 15 + t/unit-tests/clar/README.md | 329 ++++++++ t/unit-tests/clar/clar.c | 842 +++++++++++++++++++++ t/unit-tests/clar/clar.h | 173 +++++ t/unit-tests/clar/clar/fixtures.h | 50 ++ t/unit-tests/clar/clar/fs.h | 522 +++++++++++++ t/unit-tests/clar/clar/print.h | 211 ++++++ t/unit-tests/clar/clar/sandbox.h | 159 ++++ t/unit-tests/clar/clar/summary.h | 143 ++++ t/unit-tests/clar/generate.py | 266 +++++++ t/unit-tests/clar/test/.gitignore | 4 + t/unit-tests/clar/test/Makefile | 39 + t/unit-tests/clar/test/clar_test.h | 16 + t/unit-tests/clar/test/main.c | 40 + t/unit-tests/clar/test/main.c.sample | 27 + t/unit-tests/clar/test/resources/test/file | 1 + t/unit-tests/clar/test/sample.c | 84 ++ t/unit-tests/{t-ctype.c => ctype.c} | 71 +- t/unit-tests/{t-strvec.c => strvec.c} | 119 ++- t/unit-tests/unit-test.c | 17 + t/unit-tests/unit-test.h | 3 + 29 files changed, 3166 insertions(+), 102 deletions(-) create mode 100644 t/unit-tests/clar-generate.awk create mode 100644 t/unit-tests/clar/.github/workflows/ci.yml create mode 100644 t/unit-tests/clar/COPYING create mode 100644 t/unit-tests/clar/README.md create mode 100644 t/unit-tests/clar/clar.c create mode 100644 t/unit-tests/clar/clar.h create mode 100644 t/unit-tests/clar/clar/fixtures.h create mode 100644 t/unit-tests/clar/clar/fs.h create mode 100644 t/unit-tests/clar/clar/print.h create mode 100644 t/unit-tests/clar/clar/sandbox.h create mode 100644 t/unit-tests/clar/clar/summary.h create mode 100755 t/unit-tests/clar/generate.py create mode 100644 t/unit-tests/clar/test/.gitignore create mode 100644 t/unit-tests/clar/test/Makefile create mode 100644 t/unit-tests/clar/test/clar_test.h create mode 100644 t/unit-tests/clar/test/main.c create mode 100644 t/unit-tests/clar/test/main.c.sample create mode 100644 t/unit-tests/clar/test/resources/test/file create mode 100644 t/unit-tests/clar/test/sample.c rename t/unit-tests/{t-ctype.c => ctype.c} (71%) rename t/unit-tests/{t-strvec.c => strvec.c} (54%) create mode 100644 t/unit-tests/unit-test.c create mode 100644 t/unit-tests/unit-test.h Range-diff against v4: 1: 086dd728a7 ! 1: 832dc0496f t: do not pass GIT_TEST_OPTS to unit tests with prove @@ Commit message environment variable. Like this, we can conditionally forward it to our test scripts, only. + Signed-off-by: Patrick Steinhardt <ps@pks.im> + ## t/Makefile ## @@ t/Makefile: failed: test -z "$$failed" || $(MAKE) $$failed 2: 5c22e0b3b9 ! 2: 3690607933 t: import the clar unit testing framework @@ Metadata ## Commit message ## t: import the clar unit testing framework - Import the clar unit testing framework at commit faa8419 (Merge pull - request #93 from clar-test/ethomson/fixtures, 2023-12-14). The framework + Import the clar unit testing framework at commit 1516124 (Merge pull + request #97 from pks-t/pks-whitespace-fixes, 2024-08-15). The framework will be wired up in subsequent commits. Signed-off-by: Patrick Steinhardt <ps@pks.im> @@ t/unit-tests/clar/clar/fs.h (new) + ERROR_PATH_NOT_FOUND == last_error) + return 0; + -+ Sleep(RM_RETRY_DELAY * retries * retries); ++ Sleep(RM_RETRY_DELAY * retries * retries); + } + while (retries++ <= RM_RETRY_COUNT); + @@ t/unit-tests/clar/clar/sandbox.h (new) + static const size_t var_count = 5; + static const char *env_vars[] = { + "CLAR_TMP", "TMPDIR", "TMP", "TEMP", "USERPROFILE" -+ }; ++ }; + -+ size_t i; ++ size_t i; + + for (i = 0; i < var_count; ++i) { + const char *env = getenv(env_vars[i]); @@ t/unit-tests/clar/clar/sandbox.h (new) +{ + return _clar_path; +} -+ ## t/unit-tests/clar/clar/summary.h (new) ## @@ @@ t/unit-tests/clar/generate.py (new) + suite.disable(options.excluded) + if suite.write(): + print("Written `clar.suite` (%d tests in %d suites)" % (suite.callback_count(), suite.suite_count())) -+ ## t/unit-tests/clar/test/.gitignore (new) ## @@ @@ t/unit-tests/clar/test/.gitignore (new) +.clarcache +clar_test +*.o -+ ## t/unit-tests/clar/test/Makefile (new) ## @@ 4: 75e097dfa4 = 3: db53673294 t/clar: fix compatibility with NonStop 3: e0f99874cc ! 4: b6199c88dd t/clar: fix whitespace errors @@ Metadata Author: Patrick Steinhardt <ps@pks.im> ## Commit message ## - t/clar: fix whitespace errors - - Fix whitespace errors in the clar that make git-apply(1) unhappy. This - has been cherry-picked from the upstream pull request at [1]. - - [1]: https://github.com/clar-test/clar/pull/97 + Makefile: fix sparse dependency on GENERATED_H + + The "check" Makefile target is essentially an alias around the "sparse" + target. The one difference though is that it will tell users to instead + run the "test" target in case they do not have sparse(1) installed, as + chances are high that they wanted to execute the test suite rather than + doing semantic checks. + + But even though the "check" target ultimately just ends up executing + `make sparse`, it still depends on our generated headers. This does not + make any sense though: they are irrelevant for the "test" target advice, + and if these headers are required for the "sparse" target they must be + declared as a dependency on the aliased target, not the alias. + + But even moving the dependency to the "sparse" target is wrong, as + concurrent builds may then end up generating the headers and running + sparse concurrently. Instead, we make them a dependency of the specific + objects. While that is overly broad, it does ensure correct ordering. + The alternative, specifying which file depends on what generated header + explicitly, feels rather unmaintainable. Signed-off-by: Patrick Steinhardt <ps@pks.im> - ## t/unit-tests/clar/clar/fs.h ## -@@ t/unit-tests/clar/clar/fs.h: fs_rm_wait(WCHAR *_wpath) - ERROR_PATH_NOT_FOUND == last_error) - return 0; - -- Sleep(RM_RETRY_DELAY * retries * retries); -+ Sleep(RM_RETRY_DELAY * retries * retries); - } - while (retries++ <= RM_RETRY_COUNT); + ## Makefile ## +@@ Makefile: check-sha1:: t/helper/test-tool$X - - ## t/unit-tests/clar/clar/sandbox.h ## -@@ t/unit-tests/clar/clar/sandbox.h: find_tmp_path(char *buffer, size_t length) - static const size_t var_count = 5; - static const char *env_vars[] = { - "CLAR_TMP", "TMPDIR", "TMP", "TEMP", "USERPROFILE" -- }; -+ }; + SP_OBJ = $(patsubst %.o,%.sp,$(OBJECTS)) -- size_t i; -+ size_t i; +-$(SP_OBJ): %.sp: %.c %.o ++$(SP_OBJ): %.sp: %.c %.o $(GENERATED_H) + $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ + -Wsparse-error \ + $(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $< && \ +@@ Makefile: style: + git clang-format --style file --diff --extensions c,h - for (i = 0; i < var_count; ++i) { - const char *env = getenv(env_vars[i]); -@@ t/unit-tests/clar/clar/sandbox.h: const char *clar_sandbox_path(void) - { - return _clar_path; - } -- - - ## t/unit-tests/clar/generate.py ## -@@ t/unit-tests/clar/generate.py: def write(self): - suite.disable(options.excluded) - if suite.write(): - print("Written `clar.suite` (%d tests in %d suites)" % (suite.callback_count(), suite.suite_count())) -- - - ## t/unit-tests/clar/test/.gitignore ## -@@ t/unit-tests/clar/test/.gitignore: clar.suite - .clarcache - clar_test - *.o -- + .PHONY: check +-check: $(GENERATED_H) ++check: + @if sparse; \ + then \ + echo >&2 "Use 'make sparse' instead"; \ -: ---------- > 5: 06364b2b72 Makefile: make hdr-check depend on generated headers -: ---------- > 6: 88ea94ce16 Makefile: do not use sparse on third-party sources 5: 5b8a64ae79 ! 7: 05bcb5bef6 Makefile: wire up the clar unit testing framework @@ .gitignore /bin-wrappers/ ## Makefile ## +@@ Makefile: REFTABLE_TEST_LIB = reftable/libreftable_test.a + GENERATED_H += command-list.h + GENERATED_H += config-list.h + GENERATED_H += hook-list.h ++GENERATED_H += $(UNIT_TEST_DIR)/clar-decls.h ++GENERATED_H += $(UNIT_TEST_DIR)/clar.suite + + .PHONY: generated-hdrs + generated-hdrs: $(GENERATED_H) @@ Makefile: THIRD_PARTY_SOURCES += sha1dc/% THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/% THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/% @@ Makefile: endif bin-wrappers/%: wrap-for-bin.sh $(call mkdir_p_parent_template) -@@ Makefile: CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(LIB_H)) - HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) - HCC = $(HCO:hco=hcc) - -+$(UNIT_TEST_DIR)/unit-test.hcc: $(UNIT_TEST_DIR)/unit-test.h $(UNIT_TEST_DIR)/clar-decls.h - %.hcc: %.h - @echo '#include "git-compat-util.h"' >$@ - @echo '#include "$<"' >>$@ @@ Makefile: endif artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \ @@ Makefile: cocciclean: clean: profile-clean coverage-clean cocciclean $(RM) -r .build $(UNIT_TEST_BIN) -+ $(RM) GIT-TEST-SUITES $(UNIT_TEST_DIR)/clar.suite $(UNIT_TEST_DIR)/clar-decls.h ++ $(RM) GIT-TEST-SUITES $(RM) po/git.pot po/git-core.pot $(RM) git.res $(RM) $(OBJECTS) 6: bc4e23d666 = 8: 8f56b4d626 t/unit-tests: convert strvec tests to use clar 7: 0a7fe8775a = 9: ca09d19fd5 t/unit-tests: convert ctype tests to use clar base-commit: 406f326d271e0bacecdb00425422c5fa3f314930