Re: [PATCH v4 0/3] config: add '--sources' option to print the source of a config value

[Jeff King <peff@peff.net>]

On Mon, Feb 15, 2016 at 11:17:43AM +0100, larsxschneider@gmail.com wrote:

> I like the idea of a "test set up block" within a test script. In order
> to clean up nicely before any subsequent tests I would like to propose
> a "tear down" block. Would that work as a compromise in our "test cases
> depend on earlier test cases" discussion?

I don't have any real problem with what you've written in the final
patch, but I also don't think it's accomplishing much (and is more lines
of code, and more running processes).

If you want to run test N without having run all of 1..N-1, what you
really want is some known, reliable state when that test starts. But the
tests before it do not necessarily know what that state is.  The best
they can do is roughly restore the original state before they ran. But:

  1. What does the state consist of? Which files (and their contents)
     are important to the test?

     In your tear-down you get rid of $INCLUDE_DIR, and you zero-out the
     config files. But you leave expect, output, output.raw, and the
     oddly named $CUSTOM_CONFIG_FILE. Nor do you clean up the
     environment variables.

     To be clear, I think it's perfectly fine to leave those. But you
     are still making assumptions about what the next test relies on.

  2. We may create a clean slate, but that is probably not what the next
     test wants. It will want to do its own setup. I.e., it will
     probably not want a blank .git/config, and will create it itself,
     just as you did in your setup step.

So rather than tearing down, I think we are better off trying to make
tests themselves (or blocks of them) set up their own assumptions. E.g.,
by overwriting files rather than appending to them. By using unique
filenames, commit messages, etc for their tests. That's less of a big
deal here, but in many tests that create commits, "test_commit foo"
would fail a second time, because there are no changes to "foo". Doing
"test_commit subdir/check-diff-in-subdir" is less likely to clash
without another test.

Sometimes we _are_ better off with a teardown step, because subsequent
tests would not reasonably think to clear some state we've set (e.g., in
non-config tests, if we set some random config variable, we use
test_config to tear it down afterwards rather than have each test clean
out all of the config). So there's definitely a subjective judgement
call on what is "reasonable" there. But I find it unlikely that your
tear-down will help anybody in this case. Further tests will not care
about $INCLUDE_DIR unless they reference it, and any further tests would
set up their own .git/config, etc.

-Peff