threads / patch / 54723

patch, 3 partst7900: fix test failures when invoked individually via --run

Subject: [PATCH 1/3] t7900: fix test failures when invoked individually via --run

## tl;dr

6 messages between Nov 27, 2020 and Nov 28, 2020. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Eric Sunshine· Nov 27, 2020, 07:50 UTC · lore

[PATCH 0/3] make macOS `git maintenance` test work on Windows

The `git maintenance start` and `git maintenance stop` tests -- with one minor exception -- have been carefully crafted to work correctly on any platform despite the fact that those commands necessarily invoke platform-specific scheduling utilities. The exception is that the macOS-specific test fails on Windows, thus is presently disabled on that platform. This patch series addresses that shortcoming.

This series is built atop ds/maintenance-part-4.
Eric Sunshine (3):
  t7900: fix test failures when invoked individually via --run
  test-tool: add `getuid` subcommand
  t7900: make macOS-specific test work on Windows
 Makefile               |  1 +
 t/helper/test-getuid.c |  7 +++++++
 t/helper/test-tool.c   |  1 +
 t/helper/test-tool.h   |  1 +
 t/t7900-maintenance.sh | 13 +++++++------
 5 files changed, 17 insertions(+), 6 deletions(-)
 create mode 100644 t/helper/test-getuid.c
-- 
2.29.2.576.ga3fc446d84
Eric Sunshine· Nov 27, 2020, 07:50 UTC · re: Eric Sunshine · lore

A couple tests use `rm expect` to remove a file created by earlier tests. The presence of this file is immaterial at the point the attempt is made to remove it, yet if it doesn't exist, the test fails unnecessarily. One case in which the file may validly not exist is when --run or GIT_SKIP_TESTS is used to selectively run particular tests. Fix these pointless failures.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t7900-maintenance.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to t/t7900-maintenance.sh +1 −2
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 0246e4ce30..ef3aec3253 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -429,7 +429,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
 	EOF
 	test_cmp expect actual &&
 
-	rm expect &&
+	rm -f expect &&
 	for frequency in hourly daily weekly
 	do
 		PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
@@ -491,7 +491,6 @@ test_expect_success 'start and stop Windows maintenance' '
 	# stop does not unregister the repo
 	git config --get --global maintenance.repo "$(pwd)" &&
 
-	rm expect &&
 	printf "/delete /tn Git Maintenance (%s) /f\n" \
 		hourly daily weekly >expect &&
 	test_cmp expect args
-- 
2.29.2.576.ga3fc446d84
Eric Sunshine· Nov 27, 2020, 07:50 UTC · re: Eric Sunshine · lore

[PATCH 2/3] test-tool: add `getuid` subcommand

It is difficult, if not impossible, for a test to obtain the user ID in a portable fashion on all platforms and be guaranteed that it is the same user ID Git itself sees when invoking getuid(). This is especially true on Microsoft Windows for which getuid() in C code and $(id -u) in a shell script may return entirely different values. Sidestep this problem by adding a `getuid` subcommand to test-tool which is guaranteed to provide tests with the same value as getuid() returns in Git code.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Makefile               | 1 +
 t/helper/test-getuid.c | 7 +++++++
 t/helper/test-tool.c   | 1 +
 t/helper/test-tool.h   | 1 +
 4 files changed, 10 insertions(+)
 create mode 100644 t/helper/test-getuid.c
Show changes to 4 files +10 −0

Makefile, t/helper/test-getuid.c, t/helper/test-tool.c, t/helper/test-tool.h

diff --git a/Makefile b/Makefile
index c39b39bd7d..a5c38aad5e 100644
--- a/Makefile
+++ b/Makefile
@@ -703,6 +703,7 @@ TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-genrandom.o
 TEST_BUILTINS_OBJS += test-genzeros.o
+TEST_BUILTINS_OBJS += test-getuid.o
 TEST_BUILTINS_OBJS += test-hash-speed.o
 TEST_BUILTINS_OBJS += test-hash.o
 TEST_BUILTINS_OBJS += test-hashmap.o
diff --git a/t/helper/test-getuid.c b/t/helper/test-getuid.c
new file mode 100644
index 0000000000..d741302461
--- /dev/null
+++ b/t/helper/test-getuid.c
@@ -0,0 +1,7 @@
+#include "test-tool.h"
+
+int cmd__getuid(int argc, const char **argv)
+{
+	printf("%d\n", getuid());
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 432b49d948..ad5f681e68 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -31,6 +31,7 @@ static struct test_cmd cmds[] = {
 	{ "example-decorate", cmd__example_decorate },
 	{ "genrandom", cmd__genrandom },
 	{ "genzeros", cmd__genzeros },
+	{ "getuid", cmd__getuid },
 	{ "hashmap", cmd__hashmap },
 	{ "hash-speed", cmd__hash_speed },
 	{ "index-version", cmd__index_version },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7c3281e071..765976e1eb 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -21,6 +21,7 @@ int cmd__dump_untracked_cache(int argc, const char **argv);
 int cmd__example_decorate(int argc, const char **argv);
 int cmd__genrandom(int argc, const char **argv);
 int cmd__genzeros(int argc, const char **argv);
+int cmd__getuid(int argc, const char **argv);
 int cmd__hashmap(int argc, const char **argv);
 int cmd__hash_speed(int argc, const char **argv);
 int cmd__index_version(int argc, const char **argv);
-- 
2.29.2.576.ga3fc446d84
Eric Sunshine· Nov 27, 2020, 07:50 UTC · re: Eric Sunshine · lore

[PATCH 3/3] t7900: make macOS-specific test work on Windows

Although `git maintenance start` and `git maintenance stop` necessarily invoke platform-specific scheduling utilities, their related tests have been carefully crafted -- with one minor exception -- to work correctly on any platform, thus improving overall coverage. The exception is that the macOS-specific test fails on Windows due to unportable use of `$(id -u)` and comparison involving the value of $HOME which suffers from the typical shortcoming on that platform in which the same path may be represented two different ways depending upon its source (i.e. as a Windows path `C:/git-sdk-64/usr/src/git/foo` versus as a Unix path `/usr/src/git/foo`). Fix both problems and drop the !MINGW prerequisite from the macOS-specific test, thus allowing the test to run on Windows, as well.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t7900-maintenance.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
Show changes to t/t7900-maintenance.sh +6 −4
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index ef3aec3253..500eaae4fd 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -408,8 +408,10 @@ test_expect_success 'start preserves existing schedule' '
 	grep "Important information!" cron.txt
 '
 
-test_expect_success !MINGW 'start and stop macOS maintenance' '
-	uid=$(id -u) &&
+test_expect_success 'start and stop macOS maintenance' '
+	uid=$(test-tool getuid) &&
+	# ensure $HOME can be compared against hook arguments on all platforms
+	pfx=$(cd "$HOME" && pwd) &&
 
 	write_script print-args <<-\EOF &&
 	echo $* >>args
@@ -432,7 +434,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
 	rm -f expect &&
 	for frequency in hourly daily weekly
 	do
-		PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
+		PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
 		test_xmllint "$PLIST" &&
 		grep schedule=$frequency "$PLIST" &&
 		echo "bootout gui/$uid $PLIST" >>expect &&
@@ -446,7 +448,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
 	# stop does not unregister the repo
 	git config --get --global maintenance.repo "$(pwd)" &&
 
-	printf "bootout gui/$uid $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
+	printf "bootout gui/$uid $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
 		hourly daily weekly >expect &&
 	test_cmp expect args &&
 	ls "$HOME/Library/LaunchAgents" >actual &&
-- 
2.29.2.576.ga3fc446d84
Ævar Arnfjörð Bjarmason· Nov 27, 2020, 15:05 UTC · re: Eric Sunshine · lore

Re: [PATCH 3/3] t7900: make macOS-specific test work on Windows

On Fri, Nov 27 2020, Eric Sunshine wrote:
Show 32 quoted lines
> Although `git maintenance start` and `git maintenance stop` necessarily
> invoke platform-specific scheduling utilities, their related tests have
> been carefully crafted -- with one minor exception -- to work correctly
> on any platform, thus improving overall coverage. The exception is that
> the macOS-specific test fails on Windows due to unportable use of
> `$(id -u)` and comparison involving the value of $HOME which suffers
> from the typical shortcoming on that platform in which the same path may
> be represented two different ways depending upon its source (i.e. as a
> Windows path `C:/git-sdk-64/usr/src/git/foo` versus as a Unix path
> `/usr/src/git/foo`). Fix both problems and drop the !MINGW prerequisite
> from the macOS-specific test, thus allowing the test to run on Windows,
> as well.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t7900-maintenance.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index ef3aec3253..500eaae4fd 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -408,8 +408,10 @@ test_expect_success 'start preserves existing schedule' '
>  	grep "Important information!" cron.txt
>  '
>  
> -test_expect_success !MINGW 'start and stop macOS maintenance' '
> -	uid=$(id -u) &&
> +test_expect_success 'start and stop macOS maintenance' '
> +	uid=$(test-tool getuid) &&
> +	# ensure $HOME can be compared against hook arguments on all platforms
> +	pfx=$(cd "$HOME" && pwd) &&
This seems equally portable, and means your 2/3 isn't needed, no?
    
    diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
    index c3dcb9cb4d..b23f77aebc 100755
    --- a/t/t7900-maintenance.sh
    +++ b/t/t7900-maintenance.sh
    @@ -458,10 +458,10 @@ test_expect_success 'start preserves existing schedule' '
     '
     
     test_expect_success !MINGW 'start and stop macOS maintenance' '
    -	uid=$(id -u) &&
    +	uid=FAKE_UID &&
     
    -	write_script print-args <<-\EOF &&
    -	echo $* >>args
    +	write_script print-args <<-EOF &&
    +	echo \$* | perl -pe "s[(?<= gui/)-?[0-9]+][$uid]g" >>args
     	EOF
     
     	rm -f args &&

I.e. the context here is that the test is already hardcoding an assumption about "gui/%d" (per code in gc.c). It seems more robust & to the point of the test to not care about the specific UID number that comes back, since we're really testing whether we invoke our own code, not platform getuid() sanity.

Show 20 quoted lines
>  	write_script print-args <<-\EOF &&
>  	echo $* >>args
> @@ -432,7 +434,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
>  	rm -f expect &&
>  	for frequency in hourly daily weekly
>  	do
> -		PLIST="$HOME/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
> +		PLIST="$pfx/Library/LaunchAgents/org.git-scm.git.$frequency.plist" &&
>  		test_xmllint "$PLIST" &&
>  		grep schedule=$frequency "$PLIST" &&
>  		echo "bootout gui/$uid $PLIST" >>expect &&
> @@ -446,7 +448,7 @@ test_expect_success !MINGW 'start and stop macOS maintenance' '
>  	# stop does not unregister the repo
>  	git config --get --global maintenance.repo "$(pwd)" &&
>  
> -	printf "bootout gui/$uid $HOME/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
> +	printf "bootout gui/$uid $pfx/Library/LaunchAgents/org.git-scm.git.%s.plist\n" \
>  		hourly daily weekly >expect &&
>  	test_cmp expect args &&
>  	ls "$HOME/Library/LaunchAgents" >actual &&
Eric Sunshine· Nov 28, 2020, 05:55 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/3] t7900: make macOS-specific test work on Windows

On Fri, Nov 27, 2020 at 10:05 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines
> On Fri, Nov 27 2020, Eric Sunshine wrote:
> > +test_expect_success 'start and stop macOS maintenance' '
> > +     uid=$(test-tool getuid) &&
>
> This seems equally portable, and means your 2/3 isn't needed, no?
>     +   uid=FAKE_UID &&
>     +   echo \$* | perl -pe "s[(?<= gui/)-?[0-9]+][$uid]g" >>args
> I.e. the context here is that the test is already hardcoding an
> assumption about "gui/%d" (per code in gc.c). It seems more robust & to
> the point of the test to not care about the specific UID number that
> comes back, since we're really testing whether we invoke our own code,
> not platform getuid() sanity.
Good idea. That's a reasonable viewpoint to take, and makes the series simpler.

← back to recent threads