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

[Lars Schneider]

On 15 Feb 2016, at 19:05, Jeff King  wrote:

> On Mon, Feb 15, 2016 at 11:17:43AM +0100, larsxschnei...@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.
Good argument - I can't disagree.

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

OK, I will remove the block in the next roll. Thanks for explaining
your thoughts on this.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:43AM +0100, larsxschnei...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[larsxschneider]

From: Lars Schneider 

diff to v3:

* pass type as parameter to "git_config_from_mem" (renamed from
  "git_config_from_buf") and "do_config_from_file"
* split current_config_type_name into two functions
* explain usage of fwrite with a comment
* use tabs instead of spaces to fix indentation error
* squash Peff's commit to split the test cases
* sort all test configs key/value pairs in the same way
* add comment to explain newline introduce by here-doc
* use real tabs for test file name
* add a test teardown
* fix hiding of git exit code t1300 and t7008
* drop "git-config.txt: describe '--includes' default behavior" patch

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?

In "t: do not hide Git's exit code in tests" I also fixed a few more
places where Git's exit code was hidden. Please drop this patch if you
think that this should not be part of this series.

Thanks a lot for the reviews and explainations,
Lars

Lars Schneider (3):
  t: do not hide Git's exit code in tests
  config: add 'type' to config_source struct that identifies config type
  config: add '--show-origin' option to print the origin of a config
value

 Documentation/git-config.txt |  15 ++--
 builtin/config.c |  33 +
 cache.h  |   6 +-
 config.c |  36 +++---
 submodule-config.c   |   4 +-
 t/t1300-repo-config.sh   | 167 ++-
 t/t1308-config-set.sh|   4 +-
 t/t7008-grep-binary.sh   |   3 +-
 8 files changed, 242 insertions(+), 26 deletions(-)

--
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html