Re: [PATCH 1/5] git-p4 tests: refactor, split out common functions
Pete Wyckoff <pw@padd.com> writes:
Show 14 quoted lines
> 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@padd.com>
> ---
> 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.
Show 19 quoted lines
> 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=10669What happens when tests are run in parallel (make -j or prove --jobs) and 9800 and 9801 are run at the same time?
Show 9 quoted lines
> +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 () {Show 17 quoted lines
> + 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} ; doThat {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.
Show 26 quoted lines
> + 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
)
}Show 8 quoted lines
> 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 &&
...
)
'
