| Message ID | cover.1723095269.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce clar testing framework | expand |
On 2024.08.08 07:38, Patrick Steinhardt wrote: > Hi, > > this is the third version of my RFC patch series that introduces the > clar testing framework into our unit tests. The intent is to not have to > hand-craft all features of a proper unit testing framework, while still > not painting us into a corner. As such, the clar itself is small and > extensible while still bringing some nice features to the table. > > Changes compared to v2: > > - Fix a copy/paste error for the clar license. It's ISC, not LGPL. > > - Include "clar.h" via "clar/clar.h" such that we do not have to add > "clar/" as in preprocessor include directive. > > - Adapt strvec unit test to use `cl_assert_equal_i()` instead of > `cl_assert()`. > > Thanks! > > Patrick > > Patrick Steinhardt (7): > t: do not pass GIT_TEST_OPTS to unit tests with prove > t: import the clar unit testing framework > t/clar: fix whitespace errors > t/clar: fix compatibility with NonStop > 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 I'm generally in favor of this change, but I'm still unsure what our plan is for importing this from upstream clar. Are we going to vendor our own copy here and (hopefully) someone will pay attention to upstream fixes and apply them to our copy? Or will we replace this with a submodule?
On Monday, August 12, 2024 2:11 PM, Josh Steadmon wrote: >On 2024.08.08 07:38, Patrick Steinhardt wrote: >> Hi, >> >> this is the third version of my RFC patch series that introduces the >> clar testing framework into our unit tests. The intent is to not have >> to hand-craft all features of a proper unit testing framework, while >> still not painting us into a corner. As such, the clar itself is small >> and extensible while still bringing some nice features to the table. >> >> Changes compared to v2: >> >> - Fix a copy/paste error for the clar license. It's ISC, not LGPL. >> >> - Include "clar.h" via "clar/clar.h" such that we do not have to add >> "clar/" as in preprocessor include directive. >> >> - Adapt strvec unit test to use `cl_assert_equal_i()` instead of >> `cl_assert()`. >> >> Thanks! >> >> Patrick >> >> Patrick Steinhardt (7): >> t: do not pass GIT_TEST_OPTS to unit tests with prove >> t: import the clar unit testing framework >> t/clar: fix whitespace errors >> t/clar: fix compatibility with NonStop >> 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 > >I'm generally in favor of this change, but I'm still unsure what our plan is for >importing this from upstream clar. Are we going to vendor our own copy here and >(hopefully) someone will pay attention to upstream fixes and apply them to our >copy? Or will we replace this with a submodule? I think we have our own copy of clar, customized to integrate with the existing make infrastructure.
Josh Steadmon <steadmon@google.com> writes: > I'm generally in favor of this change, but I'm still unsure what our > plan is for importing this from upstream clar. Are we going to vendor > our own copy here and (hopefully) someone will pay attention to upstream > fixes and apply them to our copy? Or will we replace this with a > submodule? As long as we do not have to make any changes to the "vendored" code ourselves, that would not matter. We will not randomly update the gitlink that specifies "we want to use _this_ version and not other version of upstream clar" without good reasons if you are using it as a submodule, and we would need to justify why we are updating the hierarchy if we import the hierarchy as vendored source. So the hassle of "updating from upstream" is pretty much the same. For something as small as "clar", I think it is fine to start with the currently proposed layout and see what happens. If we can keep going without touching the imported part of the sources at all, and the system proves to be useful and stable, that is a good time to suggest moving it out and binding the selected version of the upstream as a submodule. Thanks.
On Monday, August 12, 2024 4:50 PM, Junio C Hamano wrote: >Josh Steadmon <steadmon@google.com> writes: > >> I'm generally in favor of this change, but I'm still unsure what our >> plan is for importing this from upstream clar. Are we going to vendor >> our own copy here and (hopefully) someone will pay attention to >> upstream fixes and apply them to our copy? Or will we replace this >> with a submodule? > >As long as we do not have to make any changes to the "vendored" code ourselves, >that would not matter. We will not randomly update the gitlink that specifies "we >want to use _this_ version and not other version of upstream clar" without good >reasons if you are using it as a submodule, and we would need to justify why we >are updating the hierarchy if we import the hierarchy as vendored source. So the >hassle of "updating from upstream" is pretty much the same. > >For something as small as "clar", I think it is fine to start with the currently proposed >layout and see what happens. If we can keep going without touching the imported >part of the sources at all, and the system proves to be useful and stable, that is a >good time to suggest moving it out and binding the selected version of the >upstream as a submodule. I think we already have a copy customized for git's use. The main clar repo on its own has portability issues. I have contributed a few fixes, but they need work.
<rsbecker@nexbridge.com> writes: >>For something as small as "clar", I think it is fine to start with the currently proposed >>layout and see what happens. If we can keep going without touching the imported >>part of the sources at all, and the system proves to be useful and stable, that is a >>good time to suggest moving it out and binding the selected version of the >>upstream as a submodule. > > I think we already have a copy customized for git's use. The main clar repo on its own > has portability issues. I have contributed a few fixes, but they need work. Yup, but as long as the changes we make are all upstreamable, the story does not change all that much. Changes like "#ifdef TANDEM" would be totally uncontroversial thing for them to accept and we should be able to upstream them fairly easily, and once we thin our local customization down to zero, we'd reach the state I outlined. Starting out with a local copy helps us making these portability and other changes without much friction, regardless of how responsive the upstream is, and the request upstream would see is "here are the changes to make it available on more platforms and/or making it generally more useful. all of these changes have been used and battle-tested in the context of the Git project for N months, please apply."
On Mon, Aug 12, 2024 at 03:13:39PM -0700, Junio C Hamano wrote: > <rsbecker@nexbridge.com> writes: > > >>For something as small as "clar", I think it is fine to start with the currently proposed > >>layout and see what happens. If we can keep going without touching the imported > >>part of the sources at all, and the system proves to be useful and stable, that is a > >>good time to suggest moving it out and binding the selected version of the > >>upstream as a submodule. > > > > I think we already have a copy customized for git's use. The main clar repo on its own > > has portability issues. I have contributed a few fixes, but they need work. > > Yup, but as long as the changes we make are all upstreamable, the > story does not change all that much. Changes like "#ifdef TANDEM" > would be totally uncontroversial thing for them to accept and we > should be able to upstream them fairly easily, and once we thin our > local customization down to zero, we'd reach the state I outlined. > > Starting out with a local copy helps us making these portability and > other changes without much friction, regardless of how responsive > the upstream is, and the request upstream would see is "here are the > changes to make it available on more platforms and/or making it > generally more useful. all of these changes have been used and > battle-tested in the context of the Git project for N months, please > apply." Yeah, agreed. My intention certainly is to upstream all required changes. Right now, it's only two minor fixes that we need, one for NonStop and one for whitespace fixes. While I'm still one of the maintainers of the clar, I first want to connect with Ed before merging any of these fixes. I would otherwise consider it a bit overreaching to just go in and merge things after having been absent from that project for such a long time. But when things are sorted out I think the upstreaming process should be easy enough. Meanwhile, I think keeping this in the tree is fine. We can then optionally convert the code to a submodule once the necessary changes are upstream. Patrick
Hi, this is the third version of my RFC patch series that introduces the clar testing framework into our unit tests. The intent is to not have to hand-craft all features of a proper unit testing framework, while still not painting us into a corner. As such, the clar itself is small and extensible while still bringing some nice features to the table. Changes compared to v2: - Fix a copy/paste error for the clar license. It's ISC, not LGPL. - Include "clar.h" via "clar/clar.h" such that we do not have to add "clar/" as in preprocessor include directive. - Adapt strvec unit test to use `cl_assert_equal_i()` instead of `cl_assert()`. Thanks! Patrick Patrick Steinhardt (7): t: do not pass GIT_TEST_OPTS to unit tests with prove t: import the clar unit testing framework t/clar: fix whitespace errors t/clar: fix compatibility with NonStop 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 | 42 +- 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, 3159 insertions(+), 98 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 v2: 1: 78a9cc1162 = 1: 78a9cc1162 t: do not pass GIT_TEST_OPTS to unit tests with prove 2: 6a88cf22a5 ! 2: b6c066ee4e t: import the clar unit testing framework @@ Documentation/technical/unit-tests.txt: Framework,"<<license,License>>","<<vendo {criterion},{mit},{false},{partial},{true},{true},{true},{true},{true},{false},{true},19,1800 {c-tap},{expat},{true},{partial},{partial},{true},{false},{true},{false},{false},{false},4,33 {check},{lgpl},{false},{partial},{true},{true},{true},{false},{false},{false},{true},17,973 -+{clar},{lgpl},{false},{partial},{true},{true},{true},{true},{false},{false},{true},1,192 ++{clar},{isc},{false},{partial},{true},{true},{true},{true},{false},{false},{true},1,192 |===== === Additional framework candidates 3: a52ee59bf4 = 3: 35682b7686 t/clar: fix whitespace errors 4: 02fb86dfbc = 4: 7a76c21bcb t/clar: fix compatibility with NonStop 5: 848dc673c4 ! 5: 68b3c65951 Makefile: wire up the clar unit testing framework @@ Makefile: endif bin-wrappers/%: wrap-for-bin.sh $(call mkdir_p_parent_template) -@@ Makefile: $(SP_OBJ): %.sp: %.c %.o - .PHONY: sparse - sparse: $(SP_OBJ) - --EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/% $(UNIT_TEST_DIR)/clar/% $(UNIT_TEST_DIR)/clar/clar/% -+EXCEPT_HDRS := $(GENERATED_H) unicode-width.h compat/% xdiff/% $(UNIT_TEST_DIR)/unit-test.h $(UNIT_TEST_DIR)/clar/% $(UNIT_TEST_DIR)/clar/clar/% - ifndef OPENSSL_SHA1 - EXCEPT_HDRS += sha1/openssl.h - endif @@ Makefile: endif artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \ @@ Makefile: $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o \ +$(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h + $(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite +$(UNIT_TESTS_OBJS): $(UNIT_TEST_DIR)/clar-decls.h -+$(UNIT_TESTS_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) -I$(UNIT_TEST_DIR)/clar ++$(UNIT_TESTS_OBJS): EXTRA_CPPFLAGS = -I$(UNIT_TEST_DIR) +$(UNIT_TESTS_PROG): $(UNIT_TEST_DIR)/clar.suite $(UNIT_TESTS_OBJS) $(GITLIBS) GIT-LDFLAGS + $(call mkdir_p_parent_template) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) @@ t/unit-tests/unit-test.c (new) ## t/unit-tests/unit-test.h (new) ## @@ +#include "git-compat-util.h" -+#include "clar.h" ++#include "clar/clar.h" +#include "clar-decls.h" 6: 578e657269 ! 6: 4a0888380e t/unit-tests: convert strvec tests to use clar @@ t/unit-tests/t-strvec.c => t/unit-tests/strvec.c - check_uint(vec.nr, ==, 0); - check_uint(vec.alloc, ==, 0); + cl_assert_equal_p(vec.v, empty_strvec); -+ cl_assert(vec.nr == 0); -+ cl_assert(vec.alloc == 0); ++ cl_assert_equal_i(vec.nr, 0); ++ cl_assert_equal_i(vec.alloc, 0); } -static void t_dynamic_init(void) @@ t/unit-tests/t-strvec.c => t/unit-tests/strvec.c - check_uint(vec.nr, ==, 0); - check_uint(vec.alloc, ==, 0); + cl_assert_equal_p(vec.v, empty_strvec); -+ cl_assert(vec.nr == 0); -+ cl_assert(vec.alloc == 0); ++ cl_assert_equal_i(vec.nr, 0); ++ cl_assert_equal_i(vec.alloc, 0); } -static void t_clear(void) @@ t/unit-tests/t-strvec.c => t/unit-tests/strvec.c - check_uint(vec.nr, ==, 0); - check_uint(vec.alloc, ==, 0); + cl_assert_equal_p(vec.v, empty_strvec); -+ cl_assert(vec.nr == 0); -+ cl_assert(vec.alloc == 0); ++ cl_assert_equal_i(vec.nr, 0); ++ cl_assert_equal_i(vec.alloc, 0); } -static void t_push(void) @@ t/unit-tests/strvec.c: static void t_detach(void) - check_uint(vec.nr, ==, 0); - check_uint(vec.alloc, ==, 0); + cl_assert_equal_p(vec.v, empty_strvec); -+ cl_assert(vec.nr == 0); -+ cl_assert(vec.alloc == 0); ++ cl_assert_equal_i(vec.nr, 0); ++ cl_assert_equal_i(vec.alloc, 0); free((char *) detached[0]); free(detached); 7: 238de33b93 = 7: f423b01c05 t/unit-tests: convert ctype tests to use clar