| Message ID | ded8d502d97d3d48dc0e4397b6153f4b06fa319b.1606545878.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Minimal patches to let reftable pass the CI builds | expand |
On Sat, Nov 28, 2020 at 06:44:38AM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The stack_test hard-codes `/tmp/`. That is a particular problem on > Windows where the temp directory is never at that location. > > Let's not do that, but instead use `TMPDIR` as we do in similar > scenarios in the rest of Git's source code. Yeah, I noticed this, as well. This seems like a good band-aid, but it would probably be nice if the test tool was able to write into a specified directory (or even just the current directory). I don't see it being invoked anywhere, but presumably if we were to add support to our test suite, we'd have a script which invokes it within a scratch directory. -Peff
Hi Peff, On Tue, 1 Dec 2020, Jeff King wrote: > On Sat, Nov 28, 2020 at 06:44:38AM +0000, Johannes Schindelin via GitGitGadget wrote: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > The stack_test hard-codes `/tmp/`. That is a particular problem on > > Windows where the temp directory is never at that location. > > > > Let's not do that, but instead use `TMPDIR` as we do in similar > > scenarios in the rest of Git's source code. > > Yeah, I noticed this, as well. This seems like a good band-aid, but it > would probably be nice if the test tool was able to write into a > specified directory (or even just the current directory). You can force it to do that by setting `TMPDIR` to the path of the current directory... > I don't see it being invoked anywhere, It is invoked in `t/t0032-reftable-unittest.sh`: https://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh > but presumably if we were to add support to our test suite, we'd have a > script which invokes it within a scratch directory. I agree that it would make most sense for t0032 to prefix the call to `test-tool` with `TMPDIR=$PWD`. Ciao, Dscho
On Tue, Dec 01, 2020 at 03:24:01PM +0100, Johannes Schindelin wrote: > > I don't see it being invoked anywhere, > > It is invoked in `t/t0032-reftable-unittest.sh`: > https://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh Ah, thank you. I was looking at what Junio has queued in hn/reftable. > > but presumably if we were to add support to our test suite, we'd have a > > script which invokes it within a scratch directory. > > I agree that it would make most sense for t0032 to prefix the call to > `test-tool` with `TMPDIR=$PWD`. Yep, that would be fine. I'm not sure if the long-term goal is to have this opaque unit-test program or not. If it is, I was likewise going to suggest that its ad-hoc output be replaced with TAP. But it looks like on your branch that "test-tool reftable" does not produce output at all. So I may be a bit behind on what the current state and forward plans are... -Peff
On Wed, Dec 2, 2020 at 2:50 AM Jeff King <peff@peff.net> wrote: > > > I don't see it being invoked anywhere, > > > > It is invoked in `t/t0032-reftable-unittest.sh`: > > https://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh > > Ah, thank you. I was looking at what Junio has queued in hn/reftable. > > > > but presumably if we were to add support to our test suite, we'd have a > > > script which invokes it within a scratch directory. > > > > I agree that it would make most sense for t0032 to prefix the call to > > `test-tool` with `TMPDIR=$PWD`. > > Yep, that would be fine. > > I'm not sure if the long-term goal is to have this opaque unit-test > program or not. If it is, I was likewise going to suggest that its > ad-hoc output be replaced with TAP. But it looks like on your branch > that "test-tool reftable" does not produce output at all. So I may be a > bit behind on what the current state and forward plans are... The most important requirement is that something fails if the unittests don't work. I surmised that that meant running tests from test-helper in some way, so this is what happens now. Looking for "unit.?test" across the git codebase didn't turn up much info. Happy to explore other solutions if you can give me pointers.
On Wed, Dec 02, 2020 at 12:01:49PM +0100, Han-Wen Nienhuys wrote: > > I'm not sure if the long-term goal is to have this opaque unit-test > > program or not. If it is, I was likewise going to suggest that its > > ad-hoc output be replaced with TAP. But it looks like on your branch > > that "test-tool reftable" does not produce output at all. So I may be a > > bit behind on what the current state and forward plans are... > > The most important requirement is that something fails if the > unittests don't work. I surmised that that meant running tests from > test-helper in some way, so this is what happens now. Looking for > "unit.?test" across the git codebase didn't turn up much info. Happy > to explore other solutions if you can give me pointers. Normally we do a combination of: - git plumbing exposes scriptable commands, and we make sure those work. This is a much coarser-grained unit than testing individual C functions, but produces resilient tests because that interface is user-visible and stable (and in fact seeing test breakages is often a sign that one will be breaking real users). These are obviously driven by shell script tests emulating what users might run. - for unit tests of individual data types where it's not appropriate to have a user-facing command, we often add a test-tool helper that exposes specific functions (e.g., t/helper/test-date.c exposes date parsing and formatting routines, test-oid-array.c exercises methods of that data type, etc). The C parts of these are usually generic, and then are driven by shell scripts providing the actual data in individual tests (and handling success/failure, skipping tests, etc). This is still coarser than you might get unit-testing inside C. E.g., you could not generally check the difference between passing an empty array vs NULL to a function. But our philosophy has generally been _not_ to test at that level. The C interfaces are internal, and Git is not that big a project. If there's a function whose caller does something unexpected, it's usually easier to fix the caller and add a test that triggers the caller's code path. I agree that a test that simply runs a bunch of C code and either exits with failure or success is better than nothing, in the sense of finding tests. And wrapping that with a single test_expect_success does that. But it's unfortunate that we get none of the fine-grained control that the test suite provides, nor much support in debugging failing tests. One middle ground would be for a battery of C tests to output TAP-compatible output (outputting "ok 1 - foo works", and "not ok 2 - bar does not work", etc). That at least gives more info on what fails, and does it in a way that the rest of the test suite can interpret (though not manipulate). -Peff
diff --git a/reftable/stack_test.c b/reftable/stack_test.c index cf2e32a25c..c9beaf4dbf 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -28,9 +28,19 @@ static void clear_dir(const char *dirname) strbuf_release(&path); } +static char *get_tmp_template(const char *prefix) +{ + static struct strbuf path = STRBUF_INIT; + const char *tmp = getenv("TMPDIR"); + + strbuf_reset(&path); + strbuf_addf(&path, "%s/%s.XXXXXX", tmp ? tmp : "/tmp", prefix); + return path.buf; +} + static void test_read_file(void) { - char fn[256] = "/tmp/stack.test_read_file.XXXXXX"; + char *fn = get_tmp_template("stack.test_read_file"); int fd = mkstemp(fn); char out[1024] = "line1\n\nline2\nline3"; int n, err; @@ -99,7 +109,7 @@ static int write_test_log(struct reftable_writer *wr, void *arg) static void test_reftable_stack_add_one(void) { - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; @@ -132,7 +142,7 @@ static void test_reftable_stack_uptodate(void) struct reftable_write_options cfg = { 0 }; struct reftable_stack *st1 = NULL; struct reftable_stack *st2 = NULL; - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); int err; struct reftable_ref_record ref1 = { .refname = "HEAD", @@ -171,7 +181,7 @@ static void test_reftable_stack_uptodate(void) static void test_reftable_stack_transaction_api(void) { - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; @@ -216,7 +226,7 @@ static void test_reftable_stack_validate_refname(void) struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); int i; struct reftable_ref_record ref = { .refname = "a/b", @@ -254,7 +264,7 @@ static int write_error(struct reftable_writer *wr, void *arg) static void test_reftable_stack_update_index_check(void) { - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; @@ -284,7 +294,7 @@ static void test_reftable_stack_update_index_check(void) static void test_reftable_stack_lock_failure(void) { - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err, i; @@ -309,7 +319,7 @@ static void test_reftable_stack_add(void) .exact_log_message = 1, }; struct reftable_stack *st = NULL; - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_ref_record refs[2] = { { NULL } }; struct reftable_log_record logs[2] = { { NULL } }; int N = ARRAY_SIZE(refs); @@ -385,7 +395,7 @@ static void test_reftable_stack_log_normalize(void) 0, }; struct reftable_stack *st = NULL; - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); uint8_t h1[SHA1_SIZE] = { 0x01 }, h2[SHA1_SIZE] = { 0x02 }; @@ -436,7 +446,7 @@ static void test_reftable_stack_log_normalize(void) static void test_reftable_stack_tombstone(void) { int i = 0; - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; @@ -511,7 +521,7 @@ static void test_reftable_stack_tombstone(void) static void test_reftable_stack_hash_id(void) { - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; @@ -621,7 +631,7 @@ static void test_suggest_compaction_segment_nothing(void) static void test_reflog_expire(void) { - char dir[256] = "/tmp/stack.test_reflog_expire.XXXXXX"; + char *dir = get_tmp_template("stack.test_reflog_expire"); struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; struct reftable_log_record logs[20] = { { NULL } }; @@ -701,7 +711,7 @@ static void test_empty_add(void) struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; int err; - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); struct reftable_stack *st2 = NULL; EXPECT(mkdtemp(dir)); @@ -723,7 +733,7 @@ static void test_reftable_stack_auto_compaction(void) { struct reftable_write_options cfg = { 0 }; struct reftable_stack *st = NULL; - char dir[256] = "/tmp/stack_test.XXXXXX"; + char *dir = get_tmp_template("stack_test"); int err, i; int N = 100; EXPECT(mkdtemp(dir));