From: Junio C Hamano <gitster@pobox•com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH/RFC] Add test_repo_expect_success for running tests in a new repository
Date: Mon, 21 Sep 2015 10:38:43 -0700 [thread overview]
Message-ID: <xmqq4minbrks.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1442712302-7912-1-git-send-email-pclouds@gmail.com> ("Nguyễn Thái Ngọc Duy"'s message of "Sun, 20 Sep 2015 08:25:02 +0700")
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
'
next prev parent reply other threads:[~2015-09-21 17:38 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2015-09-21 18:23 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4minbrks.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox