diff mbox series

[07/15] remote-mediawiki tests: guard test_cmp with test_path_is_file

Message ID 20200916102918.29805-8-avarab@gmail.com
State New
Headers show
Series remote-mediawiki: various fixes to make tests pass | expand

Commit Message

Ævar Arnfjörð Bjarmason Sept. 16, 2020, 10:29 a.m. UTC
Change a test that used a plain test_cmp to first check the file(s)
using test_path_is_file. If some of these file(s) don't exist (as
happened to me during debugging), test_cmp will emit a way less useful
message about the failure.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Đoàn Trần Công Danh Sept. 16, 2020, 2:04 p.m. UTC | #1
On 2020-09-16 12:29:10+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Change a test that used a plain test_cmp to first check the file(s)
> using test_path_is_file. If some of these file(s) don't exist (as
> happened to me during debugging), test_cmp will emit a way less useful
> message about the failure.

IIRC, <20200809174209.15466-1-sunshine@sunshineco.com> was meant to
solve this problem.

The version you're using (v2.28.0-297-g1956fa8f8d) should have it
integrated already. The test should barf with:

	error: bug in the test script: test_cmp '<file-name>' missing

Am I missed anything?
Eric Sunshine Sept. 16, 2020, 4:53 p.m. UTC | #2
On Wed, Sep 16, 2020 at 8:17 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Change a test that used a plain test_cmp to first check the file(s)
> using test_path_is_file. If some of these file(s) don't exist (as
> happened to me during debugging), test_cmp will emit a way less useful
> message about the failure.

An alternative would be to update test_cmp() to present a more helpful
error message so that all test scripts can benefit rather than just
this script. By the way, were you testing with a reasonably recent
version of Git? I ask because test_cmp() was updated not long ago to
provide better diagnostics when one of the files is missing.

[1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)
Jeff King Sept. 16, 2020, 6:41 p.m. UTC | #3
On Wed, Sep 16, 2020 at 12:29:10PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Change a test that used a plain test_cmp to first check the file(s)
> using test_path_is_file. If some of these file(s) don't exist (as
> happened to me during debugging), test_cmp will emit a way less useful
> message about the failure.

This seems like a problem that would exist in basically every test that
uses test_cmp (well, many of them do ">actual", so it's unlikely that
the content would be missing, but it would be true of any test that
expects a command to generate a file).

Rather than annotate each site, could we teach test_cmp to show a nicer
message?

-Peff
Junio C Hamano Sept. 16, 2020, 9:13 p.m. UTC | #4
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Sep 16, 2020 at 8:17 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Change a test that used a plain test_cmp to first check the file(s)
>> using test_path_is_file. If some of these file(s) don't exist (as
>> happened to me during debugging), test_cmp will emit a way less useful
>> message about the failure.
>
> An alternative would be to update test_cmp() to present a more helpful
> error message so that all test scripts can benefit rather than just
> this script. By the way, were you testing with a reasonably recent
> version of Git? I ask because test_cmp() was updated not long ago to
> provide better diagnostics when one of the files is missing.
>
> [1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)

Yes, you did this with the commit,

test_cmp() {
	test $# -eq 2 || BUG "test_cmp requires two arguments"
	if ! eval "$GIT_TEST_CMP" '"$@"'
	then
		test "x$1" = x- || test -e "$1" || BUG "test_cmp '$1' missing"
		test "x$2" = x- || test -e "$2" || BUG "test_cmp '$2' missing"
		return 1
	fi
}

and I do not immediately see why "test -e" shouldn't be "test -f".
It should ideally be "stdin is OK, otherwise it must be a readable
regular file".

Thanks.
Ævar Arnfjörð Bjarmason Sept. 21, 2020, 8:54 a.m. UTC | #5
On Wed, Sep 16 2020, Eric Sunshine wrote:

> On Wed, Sep 16, 2020 at 8:17 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Change a test that used a plain test_cmp to first check the file(s)
>> using test_path_is_file. If some of these file(s) don't exist (as
>> happened to me during debugging), test_cmp will emit a way less useful
>> message about the failure.
>
> An alternative would be to update test_cmp() to present a more helpful
> error message so that all test scripts can benefit rather than just
> this script. By the way, were you testing with a reasonably recent
> version of Git? I ask because test_cmp() was updated not long ago to
> provide better diagnostics when one of the files is missing.
>
> [1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)

Thanks (and also to Đoàn Trần Công Danh in a side-thread). I've dropped
this patch. It's indeed better to leave this to a more general facility
as in your now-integrated test_cmp patch.

The reason I came up with this now-useless patch is because I originally
started hacking this series on a slightly older version of git, which
didn't have that patch.
Ævar Arnfjörð Bjarmason Sept. 21, 2020, 10:42 a.m. UTC | #6
On Mon, Sep 21 2020, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Sep 16 2020, Eric Sunshine wrote:
>
>> On Wed, Sep 16, 2020 at 8:17 AM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>> Change a test that used a plain test_cmp to first check the file(s)
>>> using test_path_is_file. If some of these file(s) don't exist (as
>>> happened to me during debugging), test_cmp will emit a way less useful
>>> message about the failure.
>>
>> An alternative would be to update test_cmp() to present a more helpful
>> error message so that all test scripts can benefit rather than just
>> this script. By the way, were you testing with a reasonably recent
>> version of Git? I ask because test_cmp() was updated not long ago to
>> provide better diagnostics when one of the files is missing.
>>
>> [1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)
>
> Thanks (and also to Đoàn Trần Công Danh in a side-thread). I've dropped
> this patch. It's indeed better to leave this to a more general facility
> as in your now-integrated test_cmp patch.
>
> The reason I came up with this now-useless patch is because I originally
> started hacking this series on a slightly older version of git, which
> didn't have that patch.

Correction: It was the other way around, but I ran into the case with
your patch + converting a test to test_expect_failure, where before that
would be an "ok" failure since a file was missing, but now hard errosr
with a BUG.

I think that behavior is OK, but in going back&forth and rebasing
managed to miss it the first time around. The v2 of this series has a
more narrow fix for that.
diff mbox series

Patch

diff --git a/contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh b/contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh
index 43580af3cf..d3de6c204a 100755
--- a/contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh
+++ b/contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh
@@ -52,9 +52,13 @@  test_expect_success 'git clone works on previously created wiki with media files
 	test_when_finished "rm -rf mw_dir mw_dir_clone" &&
 	git clone -c remote.origin.mediaimport=true \
 		mediawiki::'"$WIKI_URL"' mw_dir_clone &&
+	test_path_is_file mw_dir_clone/Foo.txt &&
+	test_path_is_file mw_dir/Foo.txt &&
 	test_cmp mw_dir_clone/Foo.txt mw_dir/Foo.txt &&
 	(cd mw_dir_clone && git checkout HEAD^) &&
 	(cd mw_dir && git checkout HEAD^) &&
+	test_path_is_file mw_dir_clone/Foo.txt &&
+	test_path_is_file mw_dir/Foo.txt &&
 	test_cmp mw_dir_clone/Foo.txt mw_dir/Foo.txt
 '
 
@@ -74,6 +78,8 @@  test_expect_success 'git clone works on previously created wiki with media files
 	test_when_finished "rm -rf mw_dir mw_dir_clone" &&
 	git clone -c remote.origin.mediaimport=true \
 		mediawiki::'"$WIKI_URL"' mw_dir_clone &&
+	test_path_is_file mw_dir_clone/Bar.txt &&
+	test_path_is_file mw_dir/Bar.txt &&
 	test_cmp mw_dir_clone/Bar.txt mw_dir/Bar.txt
 '
 
@@ -90,6 +96,7 @@  test_expect_success 'git push & pull work with locally renamed media files' '
 		git commit -m "Rename a file" &&
 		test_git_reimport &&
 		echo "A File" >expect &&
+		test_path_is_file Bar.txt &&
 		test_cmp expect Bar.txt &&
 		test_path_is_missing Foo.txt
 	)