| Message ID | 20200916102918.29805-8-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote-mediawiki: various fixes to make tests pass | expand |
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?
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)
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
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.
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.
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 --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 )
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(+)