Re: [PATCH v5 4/6] dir_iterator: add tests for dir_iterator API

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.

Three of these includes are unneeded and should be removed.

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`.

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.

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).

How about adding a test for pre-order traversal, too? It would look
almost identical to this test.

[...]

Michael