On 03/30/2017 05:32 AM, Daniel Ferreira wrote:
> Create t/helper/test-dir-iterator.c, which prints relevant information
> about a directory tree iterated over with dir_iterator.
>
> Create t/t0065-dir-iterator.sh, which tests that dir_iterator does
> iterate through a whole directory tree and that post-order directory
> iteration is correctly implemented.
Thanks for adding these tests (which really I should have added when I
first introduced `dir_iterator`).
As a general rule, it's best to add tests:
* of pre-existing functionality as soon as possible in a patch
series so that the tests can be used to check your own work,
commit by commit
* of new functionality in the commit that adds the new functionality,
so that it is obvious that the new functionality works
* of fixes to existing bugs *before* the bug fix, as a
`test_expect_failure` test, to prove that the bug exists and that
your test catches it. Then in a second commit, actually fix the bug
and change the test from `test_expect_failure` to
`test_expect_success`, to prove that your change fixed the bug.
Concretely, for this patch series, that means that you should add part
of the helper program and part of t0065 (i.e., everything that applies
to pre-order iteration) as the first patch of your series. Then you
should add the rest in patch "dir_iterator: iterate over dir after its
contents", at the same time as you add post-order iteration.
> ---
> Makefile | 1 +
> t/helper/test-dir-iterator.c | 32 +++++++++++++++++++++++++++++++
> t/t0065-dir-iterator.sh | 45 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+)
> create mode 100644 t/helper/test-dir-iterator.c
> create mode 100755 t/t0065-dir-iterator.sh
>
> diff --git a/Makefile b/Makefile
> index a5a11e7..d0245f3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -607,6 +607,7 @@ TEST_PROGRAMS_NEED_X += test-ctype
> TEST_PROGRAMS_NEED_X += test-config
> TEST_PROGRAMS_NEED_X += test-date
> TEST_PROGRAMS_NEED_X += test-delta
> +TEST_PROGRAMS_NEED_X += test-dir-iterator
> TEST_PROGRAMS_NEED_X += test-dump-cache-tree
> TEST_PROGRAMS_NEED_X += test-dump-split-index
> TEST_PROGRAMS_NEED_X += test-dump-untracked-cache
> diff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c
> new file mode 100644
> index 0000000..b4a148f
> --- /dev/null
> +++ b/t/helper/test-dir-iterator.c
> @@ -0,0 +1,32 @@
> +#include "cache.h"
> +#include "blob.h"
> +#include "dir.h"
> +#include "streaming.h"
> +#include "iterator.h"
> +#include "dir-iterator.h"
Three of these includes are unneeded and should be removed.
> +
> +int cmd_main(int argc, const char **argv) {
> + if (argc < 2) {
> + return 1;
> + }
> +
> + struct strbuf path = STRBUF_INIT;
> + strbuf_add(&path, argv[1], strlen(argv[1]));
> +
> + unsigned flag = 0;
> + if (argc == 3 && strcmp(argv[2], "--post-order") == 0)
> + flag = DIR_ITERATOR_POST_ORDER_TRAVERSAL;
It's conventional to handle options *before* other arguments. Even
though this is only a test script, it's preferable to adhere to
convention unless there is a strong reason to deviate from it. You might
looks at `test-submodule-config.c` for example code.
> + struct dir_iterator *diter = dir_iterator_begin((&path)->buf, flag);
`(&path)->buf` can be shortened to `path.buf`.
> +
> + while (dir_iterator_advance(diter) == ITER_OK) {
> + if (S_ISDIR(diter->st.st_mode))
> + printf("[d] ");
> + else
> + printf("[f] ");
> +
> + printf("(%s) %s\n", diter->relative_path, diter->path.buf);
> + }
> +
> + return 0;
> +}
> diff --git a/t/t0065-dir-iterator.sh b/t/t0065-dir-iterator.sh
> new file mode 100755
> index 0000000..3c8ea9a
> --- /dev/null
> +++ b/t/t0065-dir-iterator.sh
> @@ -0,0 +1,45 @@
> +#!/bin/sh
> +
> +test_description='Test directory iteration.'
> +
> +. ./test-lib.sh
> +
> +ITER_SORTED_OUTPUT='[d] (a) ./dir/a
> +[d] (a/b) ./dir/a/b
> +[d] (a/b/c) ./dir/a/b/c
> +[d] (d) ./dir/d
> +[d] (d/e) ./dir/d/e
> +[d] (d/e/d) ./dir/d/e/d
> +[f] (a/b/c/d) ./dir/a/b/c/d
> +[f] (a/e) ./dir/a/e
> +[f] (b) ./dir/b
> +[f] (c) ./dir/c
> +[f] (d/e/d/a) ./dir/d/e/d/a'
> +
> +test_expect_success 'dir-iterator should iterate through all files' '
> + mkdir -p dir &&
> + mkdir -p dir/a/b/c/ &&
> + date >dir/b &&
Is there a special reason to write the date to the file as opposed to, say
touch dir/b
? (Some people use `: >dir/b` for this purpose, though I've never found
out why.) If you write the date to the file, the reader will be
distracted unnecessarily wondering whether the contents are important to
the test.
> + date >dir/c &&
> + mkdir -p dir/d/e/d/ &&
> + date >dir/a/b/c/d &&
> + date >dir/a/e &&
> + date >dir/d/e/d/a &&
> +
> + test-dir-iterator ./dir >it &&
> + test "$(sort it)" == "$ITER_SORTED_OUTPUT"
We generally prefer to write the expected and actual program outputs to
files, then `test_cmp` the files. This has a couple of advantages:
* It doesn't risk missing an error return from the program under test,
which could happen in your second test below.
* If there is a disagreement, `test_cmp` shows how the files differed.
* If you run the test using `-d`, you can inspect the contents of the
files by hand to help investigate what happened.
Typically that would be done like
cat >expected-sorted <<-\EOF &&
[d] (a) ./dir/a
[d] (a/b) ./dir/a/b
[d] (a/b/c) ./dir/a/b/c
[d] (d) ./dir/d
[d] (d/e) ./dir/d/e
[d] (d/e/d) ./dir/d/e/d
[f] (a/b/c/d) ./dir/a/b/c/d
[f] (a/e) ./dir/a/e
[f] (b) ./dir/b
[f] (c) ./dir/c
[f] (d/e/d/a) ./dir/d/e/d/a
EOF
test-dir-iterator ./dir >actual &&
sort actual >actual-sorted &&
test_cmp expected-sorted actual-sorted
I also think it would be prudent to run this test twice; once for
pre-order and once for post-order traversal. You could use the same
directory setup and the same `expected-sorted` file for both tests
(e.g., set them up in a preliminary "setup" step).
> +'
> +
> +ITER_POST_ORDER_OUTPUT='[f] (a/b/c/d) ./dir2/a/b/c/d
> +[d] (a/b/c) ./dir2/a/b/c
> +[d] (a/b) ./dir2/a/b
> +[d] (a) ./dir2/a'
> +
> +test_expect_success 'dir-iterator should list files properly on post-order mode' '
> + mkdir -p dir2/a/b/c/ &&
> + date >dir2/a/b/c/d &&
> +
> + test "$(test-dir-iterator ./dir2 --post-order)" == "$ITER_POST_ORDER_OUTPUT"
> +'
How about adding a test for pre-order traversal, too? It would look
almost identical to this test.
> [...]
Michael