* Re: [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository
2015-09-20 1:25 [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository Nguyễn Thái Ngọc Duy
@ 2015-09-21 17:38 ` Junio C Hamano
2015-09-21 18:23 ` Jeff King
1 sibling, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2015-09-21 17:38 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> This could be convenient when tests are independent from the rest in the
> same file. Normally we would do this
>
> test_expect_success '...' '
> git init foo &&
> (
> cd foo &&
> <script>
> )
> '
>
> Now we can write a shorter version
>
> test_repo_expect_success '...' '
> <script>
> '
>
> The other function, test_subdir_expect_success, expands the script to
> "( cd <repo> && <script> )", which can be useful for grouping a series of
> tests that operate on the same repository in a subdir, e.g.
>
> test_expect_success 'create repo abc' 'test_create_repo abc'
> test_subdir_expect_success abc '...' <script>
> test_subdir_expect_success abc '...' <another-script>
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> Lately I start to add more and more tests in this style. So this
> looks like a good change to me.
Implementations of these helper functions are not all that
interesting for reviewers (except for finding unacceptable bits,
that is), I would think.
More interesting are how much cleaner the existing code would become.
I know we have tons of them that do "create a new, do this and that
in the repository".
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index e8d3c0f..45d7423 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -394,6 +394,26 @@ test_expect_success () {
> test_finish_
> }
Need a good doc here for a function that takes variable number of
parameters in a funny way.
> +test_subdir_expect_success () {
> + local subdir="$1"
This bash-ism will never be applied to my tree.
> + shift
> + case "$#" in
> + 2) test_expect_success "$1" "( cd $subdir && $2 )";;
Mental note: Two-arg form is for title and script.
By the way, unquoted subdir makes it too sloppy to live.
> + 3) test_expect_success "$1" "$2" "( cd $subdir && $3 )";;
Mental note: Three-arg form is for title, prereq and script.
> + 4) test_expect_success "$1" "$2" "$4 && ( cd $subdir && $3 )";;
What the heck is this one? That is why I said "implementations are
not interesting, the way they help existing ones more readable is".
It is totally unclear where the $4 comes from and has to be given in
a funny order to the helper (and I am sure it will make perfect
sense when it is explained).
> + *) error "bug in the test script: not 3-5 parameters to test-subdir-expect-success";;
Too deep an indent level here.
> + esac
> +}
> +
> +test_repo_expect_success () {
> + local repo=repo-$(($test_count+1))
> + case "$#" in
> + 2) test_subdir_expect_success "$repo" '' "$1" "$2" "test_create_repo $repo";;
> + 3) test_subdir_expect_success "$repo" "$1" "$2" "$3" "test_create_repo $repo";;
> + *) error "bug in the test script: not 2 or 3 parameters to test-repo-expect-success";;
> + esac
I do not like a few things in here (besides the same kind of
unacceptable stuff as the other one).
Often we need a new repository for testing a handful steps, and we
would want to be able to do this sequence:
- create a new repo
- do one thing in that repo
- do another thing in that repo
- do yet another thing in that repo
That would force tests to say "test_repo_expect_success" to do the
first thing (creating and running the first command) and then
subsequently do "test_subdir_expect_success $there" to run the
remainder.
I think we would only want to have test_expect_success_there (I am
avoiding "subdir" because the word has connotations that you do not
want in your use case. When we say "subdir" we typically do not
mean a separate repository or submodule) without the "auto creation
of a repository with unknown name" bit.
test_expect_success 'set up a new repository for testing' '
test_create_repo myrepo
'
test_expect_success_there mytest 'do one thing there' '
>empty &&
git add empty
git commit -m "add empty"
'
test_expect_success_there mytest 'do another thing there' '
git ls-files >actual &&
echo empty >expect &&
test_cmp expect actual
'
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository
2015-09-20 1:25 [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository Nguyễn Thái Ngọc Duy
2015-09-21 17:38 ` Junio C Hamano
@ 2015-09-21 18:23 ` Jeff King
1 sibling, 0 replies; 3+ messages in thread
From: Jeff King @ 2015-09-21 18:23 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
On Sun, Sep 20, 2015 at 08:25:02AM +0700, Nguyễn Thái Ngọc Duy wrote:
> This could be convenient when tests are independent from the rest in the
> same file. Normally we would do this
>
> test_expect_success '...' '
> git init foo &&
> (
> cd foo &&
> <script>
> )
> '
>
> Now we can write a shorter version
>
> test_repo_expect_success '...' '
> <script>
> '
I dunno. Like Junio, I would prefer to see some proposed conversions to
be able to evaluate it.
But I do not find the first one all that bad. Sure, it is more lines,
but it is also a lot more _flexible_. You can "init --bare". You can
create a repo, do some setup in it, then do some more stuff outside, and
then do some more setup inside it.
I also find it weird to happen at the "test_expect_success" layer,
rather than inside, as it seems somewhat orthogonal. E.g., how do I
expect failure?
It seems like it would be more flexible as a helper inside the test
snippet. But then I cannot think of a way to write it that is much
shorter than "(cd x && ...)".
For single git commands, I do find myself increasingly writing the first
one as:
test_expect_success '...' '
git init foo &&
git -C foo something
'
which is pretty short and clear, I think. It also keeps ancillary files
at the top-level. So if you do:
git clone . foo &&
git rev-parse refs/heads/master >expect &&
git -C foo rev-parse refs/remotes/origin/master >actual &&
test_cmp expect actual
all of the "expect" and "actual" appear at the same top-level, and you
do not have to worry about mentioning "../expect", etc.
My biggest problem with that technique is that you cannot run test_*
functions this way, so:
git -C foo test_commit bar
does not work, and you have to write:
(cd foo && test_commit bar)
though IMHO that is not so bad.
Like I said, though, I'd reserve judgement until I saw some proposed
conversions.
If we have a whole series of tests that are all supposed to be inside
"foo", I wonder if it would be that distasteful to simply "cd foo", run
the tests, and then cd back. Of course we'd want to notice if the "cd"
failed for whatever reason. Perhaps something like:
test_pushdir foo
test_expect_success ...
test_popdir
where "pushdir" would set an "error" flag if it fails, and any tests
inside it would auto-fail without running (which is much safer than
trying to run them in the wrong directory!).
This could also be extended to any other type of state mutation besides
"cd" if we wanted, though I do not have anything in mind (I guess we
could set env variables, but we usually just do that inside a
test_expect and assume it worked in subsequent tests).
-Peff
^ permalink raw reply [flat|nested] 3+ messages in thread