Pete Wyckoff <pw@xxxxxxxx> writes:
> Introduce a library for functions that are common to
> multiple git-p4 test files.
>
> Separate the tests related to detecting p4 branches
> into their own file, and add a few more.
>
> Signed-off-by: Pete Wyckoff <pw@xxxxxxxx>
> ---
> t/lib-git-p4.sh | 55 ++++++++++++
> t/t9800-git-p4.sh | 108 ++---------------------
> t/t9801-git-p4-branch.sh | 221 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 283 insertions(+), 101 deletions(-)
> create mode 100644 t/lib-git-p4.sh
> create mode 100755 t/t9801-git-p4-branch.sh
I take that you meant "coding style" by "generic test beauty" in the cover
letter, so here are some minor nitpicks.
> diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
> new file mode 100644
> index 0000000..dbc1499
> --- /dev/null
> +++ b/t/lib-git-p4.sh
> @@ -0,0 +1,55 @@
> +#
> +# Library code for git-p4 tests
> +#
> +
> +. ./test-lib.sh
> +
> +( p4 -h && p4d -h ) >/dev/null 2>&1 || {
> + skip_all='skipping git-p4 tests; no p4 or p4d'
> + test_done
> +}
> +
> +GITP4=$GIT_BUILD_DIR/contrib/fast-import/git-p4
> +P4DPORT=10669
What happens when tests are run in parallel (make -j or prove --jobs) and
9800 and 9801 are run at the same time?
> +export P4PORT=localhost:$P4DPORT
> +export P4CLIENT=client
> +
> +db="$TRASH_DIRECTORY/db"
> +cli="$TRASH_DIRECTORY/cli"
> +git="$TRASH_DIRECTORY/git"
> +
> +start_p4d()
> +{
Prevalent style in t/ and scripted part of Git in general is to begin a
shell function like this, with SP on both sides of () and opening brace
on the same line.
start_p4d () {
> + mkdir -p "$db" &&
> + p4d -q -d -r "$db" -p $P4DPORT &&
> + mkdir -p "$cli" &&
> + mkdir -p "$git" &&
> + cd "$cli" &&
> + p4 client -i <<-EOF
> + Client: client
> + Description: client
> + Root: $cli
> + View: //depot/... //client/...
> + EOF
> +}
> +
> +kill_p4d()
> +{
> + pid=`pgrep -f p4d` &&
> + test -n "$pid" &&
It is unfortunate that you have to use pgrep. I am unfamiliar with p4, but
do you have any control how p4d is started during this test? If the first
use of client automagically starts p4d without your control, that would be
harder to arrange, but the point I am getting at is that if you know when
you start p4d yourself and that is the only p4d process you use, you
should be keep its pid in $TRASH_DIRECTORY somewhere and replace these
with
pid=$(cat "$TRASH_DIRECTORY/p4d_pid") &&
kill -0 "$pid"
to see if that daemon is still alive.
You call kill_p4d at the very beginning of t9800; what instance of p4d are
you trying to kill? Who could have started it? For you to be able to kill
(and nobody sane would be running the test suite as "root", I hope), it
would be your process, but would it be possible that you are doing some
other important thing using p4 that is not related to git-p4 development
or testing at all, perhaps listening to a port different from 10669? Would
it be necessary to kill that other p4d to run these tests in a predicatable
and reproducible environment?
I would very much more prefer if at the very beginning you started p4d at
the port assigned for the test and fail the test if it cannot start for
whatever reason. Perhaps the reason you cannot start a p4d is because a
stale p4d instance is hanging around from previous round of test, and if
that is the case, then that is the bug we need to fix in the _previous_
test, not something we want to sweep under the rug by killing it during
this round of test.
> + for i in {1..5} ; do
That {1..5} does not pass POSIX shells, such as /bin/dash.
for i in 1 2 3 4 5
do
...
> + test_debug "ps wl `echo $pid`" &&
> + kill $pid 2>/dev/null &&
> + pgrep -f p4d >/dev/null || break &&
You are saying "all of these things would hold true if we attempt to kill
it and it still is alive" with the chain of "&&" up to that last pgrep,
and then with "||", you say "otherwise we do not have to keep sending the
signal to it anymore". But the extra "&&" after "break" is unneeded and
misleading.
> + sleep 0.2
That 0.2 does not look like a non-negative decimal interger like POSIX
wants to have.
> + done &&
> + rm -rf "$db" &&
> + rm -rf "$cli"
> +}
> +
> +cleanup_git() {
> + cd "$TRASH_DIRECTORY" &&
> + rm -rf "$git" &&
> + mkdir "$git"
> +}
> diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
> index 01ba041..bb89b63 100755
> --- a/t/t9800-git-p4.sh
> +++ b/t/t9800-git-p4.sh
> @@ -2,40 +2,16 @@
>
> test_description='git-p4 tests'
>
> -. ./test-lib.sh
> +. ./lib-git-p4.sh
>
> +test_expect_success 'start p4d' '
> + kill_p4d || : &&
> + start_p4d &&
> + cd "$TRASH_DIRECTORY"
> '
Don't "chdir" around in the test, and worse yet hide some "cd" in helper
functions. The seemingly unnecessary "cd $TRASH_DIRECTORY" at the end,
which may not even happen if start_p4d fails, is because start_p4d has a
hidden "cd" somewhere (which in turn may or may not run depending on where
in the && chain you have a failure).
One way to keep the test cleaner is to do the helper functions like the
following, so that the callers do not have to worry about where they end
up with:
start_p4d () {
mkdir -p "$db" "$cli" "$git" &&
p4d -q -d -r "$db" -p "$P4DPORT" &&
(
cd "$cli" &&
p4 client -i <<-EOF
...
EOF
)
}
> test_expect_success 'add p4 files' '
> cd "$cli" &&
> echo file1 >file1 &&
> p4 add file1 &&
> p4 submit -d "file1" &&
> ...
> cd "$TRASH_DIRECTORY"
> '
The same issue here.
test_expect_success 'add p4 files' '
(
cd "$cli" &&
echo file1 >file1 &&
...
)
'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html