threads / patch / 31045

patch, 3 partsTesting: XDG config files: Fix broken tests

Subject: [PATCH 0/3] Testing: XDG config files: Fix broken tests

## tl;dr

8 messages between Jul 17, 2012 and Jul 17, 2012. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Michael Witten· Jul 17, 2012, 16:04 UTC · lore

The tests for the new XDG config file code are broken because the environoment variable `XDG_CONFIG_HOME' is never set properly, and the tests themselves do not use `XDG_CONFIG_HOME' or `HOME' explicitly.

The following patch series corrects this brittleness (and the failures to which it has led):

  t/t1306-xdg-files.sh | 69 +++++++++++++++++++++++++++-------------------------
  t/test-lib.sh        |  3 +++
  2 files changed, 39 insertions(+), 33 deletions(-)
  [1] Export a suitable `XDG_CONFIG_HOME' environment variable
  [2] Use "$HOME" and "$XDG_CONFIG_HOME" explicitly
  [3] Trivial: `xdg' -> `XDG'
-- 
1.7.11.1.29.gf71be5c
Michael Witten· Jul 17, 2012, 16:04 UTC · re: Michael Witten · lore

[PATCH 1/3] Testing: XDG config files: Export a suitable `XDG_CONFIG_HOME' environment variable

The tests in:
  t/t1306-xdg-files.sh

were failing because the git commands were using the environment variable `XDG_CONFIG_HOME' as it was set for the user's usual environment, rather than as set for the testing environment.

This commit provides the quickest, simplest hack to make things work; because there is already the setting and exporting of the environment variable `HOME' in:

  t/test-lib.sh

this commit simply adds to that file the setting and exporting of the variable `XDG_CONFIG_HOME' (based on the variable `HOME' that is provided there).

However, the existing tests [sometimes] don't use these variables explicitly, so the whole structure of this testing rests on the hope that people maintain the conventions captured by the values of these variables; another commit should fix this instability by using these variables explicitly.

(Note: Double quotes are not needed around the value assigned to the variable, as word splitting is not performed).

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 t/test-lib.sh | 3 +++
 1 file changed, 3 insertions(+)
Show changes to t/test-lib.sh +3 −0
diff --git a/t/test-lib.sh b/t/test-lib.sh
index acda33d..69bcc75 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -544,6 +544,9 @@ rm -fr "$test" || {
 HOME="$TRASH_DIRECTORY"
 export HOME
 
+XDG_CONFIG_HOME=$HOME/.config
+export XDG_CONFIG_HOME
+
 if test -z "$TEST_NO_CREATE_REPO"; then
 	test_create_repo "$test"
 else

-- 
1.7.11.1.29.gf71be5c
Jonathan Nieder· Jul 17, 2012, 17:57 UTC · re: Michael Witten · lore

Re: [PATCH 1/3] Testing: XDG config files: Export a suitable `XDG_CONFIG_HOME' environment variable

Hi,
Michael Witten wrote:
Show 9 quoted lines
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -544,6 +544,9 @@ rm -fr "$test" || {
>  HOME="$TRASH_DIRECTORY"
>  export HOME
>  
> +XDG_CONFIG_HOME=$HOME/.config
> +export XDG_CONFIG_HOME
> +

I think this is a bad idea. The typical case is for XDG_CONFIG_HOME not to be set, and we need to make sure git works well in the typical case.

But the general idea seems sane --- how about
	unset VISUAL EMAIL LANGUAGE COLUMNS XDG_CONFIG_HOME $(perl -e '
		...
?
Matthieu Moy· Jul 17, 2012, 18:14 UTC · re: Michael Witten · lore

Re: [PATCH 1/3] Testing: XDG config files: Export a suitable `XDG_CONFIG_HOME' environment variable

Michael Witten <mfwitten@gmail.com> writes:
Show 9 quoted lines
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -544,6 +544,9 @@ rm -fr "$test" || {
>  HOME="$TRASH_DIRECTORY"
>  export HOME
>  
> +XDG_CONFIG_HOME=$HOME/.config
> +export XDG_CONFIG_HOME
> +
Why not just unset XDG_CONFIG_HOME?

Your match makes it look like XDG_CONFIG_HOME is required to use the configuration directory, but it is not. To me, the main feature is the ability to use $HOME/.config/git/ as a configuration directory (this is not just a convention, this is a documented feature), and the management of the variable $XDG_CONFIG_HOME is just a bonnus.

Before your patches, the correct management of $XDG_CONFIG_HOME to override $HOME/.config/git/ was untested (which is unfortunate, indeed), but after your patch serie, the fact that the default is $HOME/.config/git/ is untested, which IMHO is even worse.

Unsetting XDG_CONFIG_HOME and adding one test like this would be better IMHO.

Show changes to t/t1306-xdg-files.sh +13 −0
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 3c75c3f..f1ea9f1 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -38,6 +38,19 @@ test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''
        test_cmp expected actual
 '
 
+test_expect_success '"$XDG_CONFIG_HOME overrides $HOME/.config/git' '
+       mkdir -p "$HOME"/xdg/git/ &&
+       echo "[user]" >"$HOME"/xdg/git/config &&
+       echo "  name = in_xdg" >>"$HOME"/xdg/git/config &&
+       echo in_xdg >expected &&
+       (
+               XDG_CONFIG_HOME="$HOME"/xdg/ &&
+               export XDG_CONFIG_HOME &&
+               git config --get-all user.name >actual
+       ) &&
+       test_cmp expected actual
+'
+
 
 test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
        >.gitconfig &&

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jul 17, 2012, 18:31 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/3] Testing: XDG config files: Export a suitable `XDG_CONFIG_HOME' environment variable

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 7 quoted lines
> Before your patches, the correct management of $XDG_CONFIG_HOME to
> override $HOME/.config/git/ was untested (which is unfortunate, indeed),
> but after your patch serie, the fact that the default is
> $HOME/.config/git/ is untested, which IMHO is even worse.
>
> Unsetting XDG_CONFIG_HOME and adding one test like this would be better
> IMHO.

Absolutely. We would want to make sure that the new code does not interfere with established uses when the user does not ask for the new feature (i.e. XDG not set), and also make sure it does what it was meant to do when the feature is called for (i.e. XDG set). It might be true that the set of tests in the series did not test the full spectrum of the latter, but then we would want to see the gap filled by adding missing tests, not by converting tests for former into the ones that test for the latter.

Even with the patch below there may be other gaps in the test. For example, core.excludesfile and core.attributesfile must default to the XDG location when they exist, whether these variables are set; we may want to make sure that is not broken in the future.

Michael, could you change the direction of the patch and look into filling such gaps?

Thanks.
Show 24 quoted lines
> diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
> index 3c75c3f..f1ea9f1 100755
> --- a/t/t1306-xdg-files.sh
> +++ b/t/t1306-xdg-files.sh
> @@ -38,6 +38,19 @@ test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''
>         test_cmp expected actual
>  '
>  
> +test_expect_success '"$XDG_CONFIG_HOME overrides $HOME/.config/git' '
> +       mkdir -p "$HOME"/xdg/git/ &&
> +       echo "[user]" >"$HOME"/xdg/git/config &&
> +       echo "  name = in_xdg" >>"$HOME"/xdg/git/config &&
> +       echo in_xdg >expected &&
> +       (
> +               XDG_CONFIG_HOME="$HOME"/xdg/ &&
> +               export XDG_CONFIG_HOME &&
> +               git config --get-all user.name >actual
> +       ) &&
> +       test_cmp expected actual
> +'
> +
>  
>  test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
>         >.gitconfig &&
Michael Witten· Jul 17, 2012, 17:44 UTC · re: Michael Witten · lore

[PATCH 2/3] Testing: XDG config files: Use "$HOME" and "$XDG_CONFIG_HOME" explicitly

The tests in:
  t/t1306-xdg-files.sh
relied on brittle conventions:
  * "$HOME" and "$XDG_CONFIG_HOME" having certain values.
  * The testing commands having a certain current working
    directory; at least one test failed as a result.

This commit mitigates the problem by using the variables "$HOME" and "$XDG_CONFIG_HOME" explicitly.

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 t/t1306-xdg-files.sh | 69 +++++++++++++++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 33 deletions(-)
Show changes to t/t1306-xdg-files.sh +36 −33
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 3c75c3f..2327047 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -9,58 +9,60 @@ test_description='Compatibility with $XDG_CONFIG_HOME/git/ files'
 
 . ./test-lib.sh
 
+GIT_CONFIG_DIR=$XDG_CONFIG_HOME/git
+
-test_expect_success 'read config: xdg file exists and ~/.gitconfig doesn'\''t' '
+test_expect_success 'read config: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
-	mkdir -p .config/git &&
+	mkdir -p "$GIT_CONFIG_DIR" &&
-	echo "[alias]" >.config/git/config &&
+	echo "[alias]" >"$GIT_CONFIG_DIR"/config &&
-	echo "	myalias = !echo in_config" >>.config/git/config &&
+	echo "	myalias = !echo in_config" >>"$GIT_CONFIG_DIR"/config &&
 	echo in_config >expected &&
 	git myalias >actual &&
 	test_cmp expected actual
 '
 
 
-test_expect_success 'read config: xdg file exists and ~/.gitconfig exists' '
+test_expect_success 'read config: xdg file exists and "$HOME"/.gitconfig exists' '
-	>.gitconfig &&
+	>"$HOME"/.gitconfig &&
-	echo "[alias]" >.gitconfig &&
+	echo "[alias]" >"$HOME"/.gitconfig &&
-	echo "	myalias = !echo in_gitconfig" >>.gitconfig &&
+	echo "	myalias = !echo in_gitconfig" >>"$HOME"/.gitconfig &&
 	echo in_gitconfig >expected &&
 	git myalias >actual &&
 	test_cmp expected actual
 '
 
 
-test_expect_success 'read with --get: xdg file exists and ~/.gitconfig doesn'\''t' '
+test_expect_success 'read with --get: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
-	rm .gitconfig &&
+	rm "$HOME"/.gitconfig &&
-	echo "[user]" >.config/git/config &&
+	echo "[user]" >"$GIT_CONFIG_DIR"/config &&
-	echo "	name = read_config" >>.config/git/config &&
+	echo "	name = read_config" >>"$GIT_CONFIG_DIR"/config &&
 	echo read_config >expected &&
 	git config --get user.name >actual &&
 	test_cmp expected actual
 '
 
 
-test_expect_success 'read with --get: xdg file exists and ~/.gitconfig exists' '
+test_expect_success 'read with --get: xdg file exists and "$HOME"/.gitconfig exists' '
-	>.gitconfig &&
+	>"$HOME"/.gitconfig &&
-	echo "[user]" >.gitconfig &&
+	echo "[user]" >"$HOME"/.gitconfig &&
-	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo "	name = read_gitconfig" >>"$HOME"/.gitconfig &&
 	echo read_gitconfig >expected &&
 	git config --get user.name >actual &&
 	test_cmp expected actual
 '
 
 
-test_expect_success 'read with --list: xdg file exists and ~/.gitconfig doesn'\''t' '
+test_expect_success 'read with --list: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
-	rm .gitconfig &&
+	rm "$HOME"/.gitconfig &&
 	echo user.name=read_config >expected &&
 	git config --global --list >actual &&
 	test_cmp expected actual
 '
 
 
-test_expect_success 'read with --list: xdg file exists and ~/.gitconfig exists' '
+test_expect_success 'read with --list: xdg file exists and "$HOME"/.gitconfig exists' '
-	>.gitconfig &&
+	>"$HOME"/.gitconfig &&
-	echo "[user]" >.gitconfig &&
+	echo "[user]" >"$HOME"/.gitconfig &&
-	echo "	name = read_gitconfig" >>.gitconfig &&
+	echo "	name = read_gitconfig" >>"$HOME"/.gitconfig &&
 	echo user.name=read_gitconfig >expected &&
 	git config --global --list >actual &&
 	test_cmp expected actual
@@ -75,8 +77,8 @@ test_expect_success 'Setup' '
 
 
 test_expect_success 'Exclusion of a file in the XDG ignore file' '
-	mkdir -p "$HOME"/.config/git/ &&
+	mkdir -p "$GIT_CONFIG_DIR" &&
-	echo to_be_excluded >"$HOME"/.config/git/ignore &&
+	echo to_be_excluded >"$GIT_CONFIG_DIR"/ignore &&
 	test_must_fail git add to_be_excluded
 '
 
@@ -89,7 +91,7 @@ test_expect_success 'Exclusion in both XDG and local ignore files' '
 
 test_expect_success 'Exclusion in a non-XDG global ignore file' '
 	rm .gitignore &&
-	echo >"$HOME"/.config/git/ignore &&
+	echo >"$GIT_CONFIG_DIR"/ignore &&
 	echo to_be_excluded >"$HOME"/my_gitignore &&
 	git config core.excludesfile "$HOME"/my_gitignore &&
 	test_must_fail git add to_be_excluded
@@ -100,7 +102,7 @@ test_expect_success 'Checking attributes in the XDG attributes file' '
 	echo foo >f &&
 	git check-attr -a f >actual &&
 	test_line_count -eq 0 actual &&
-	echo "f attr_f" >"$HOME"/.config/git/attributes &&
+	echo "f attr_f" >"$GIT_CONFIG_DIR"/attributes &&
 	echo "f: attr_f: set" >expected &&
 	git check-attr -a f >actual &&
 	test_cmp expected actual
@@ -125,18 +127,18 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 '
 
 
-test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
+test_expect_success 'write: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
-	mkdir -p "$HOME"/.config/git &&
+	mkdir -p "$GIT_CONFIG_DIR" &&
-	>"$HOME"/.config/git/config &&
+	>"$GIT_CONFIG_DIR"/config &&
 	test_might_fail rm "$HOME"/.gitconfig &&
 	git config --global user.name "write_config" &&
 	echo "[user]" >expected &&
 	echo "	name = write_config" >>expected &&
-	test_cmp expected "$HOME"/.config/git/config
+	test_cmp expected "$GIT_CONFIG_DIR"/config
 '
 
 
-test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
+test_expect_success 'write: xdg file exists and "$HOME"/.gitconfig exists' '
 	>"$HOME"/.gitconfig &&
 	git config --global user.name "write_gitconfig" &&
 	echo "[user]" >expected &&
@@ -145,14 +147,15 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
 '
 
 
-test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
+test_expect_success 'write: "$XDG_CONFIG_HOME/git" exists and config file doesn'\''t' '
 	test_might_fail rm "$HOME"/.gitconfig &&
-	test_might_fail rm "$HOME"/.config/git/config &&
+	test_might_fail rm "$GIT_CONFIG_DIR"/config &&
 	git config --global user.name "write_gitconfig" &&
 	echo "[user]" >expected &&
 	echo "	name = write_gitconfig" >>expected &&
 	test_cmp expected "$HOME"/.gitconfig
 '
 
+unset GIT_CONFIG_DIR
 
 test_done

-- 
1.7.11.1.29.gf71be5c
Matthieu Moy· Jul 17, 2012, 18:19 UTC · re: Michael Witten · lore

Re: [PATCH 2/3] Testing: XDG config files: Use "$HOME" and "$XDG_CONFIG_HOME" explicitly

Michael Witten <mfwitten@gmail.com> writes:
Show 10 quoted lines
> The tests in:
>
>   t/t1306-xdg-files.sh
>
> relied on brittle conventions:
>
>   * "$HOME" and "$XDG_CONFIG_HOME" having certain values.
>
>   * The testing commands having a certain current working
>     directory;

Other tests (t1305-config-include.sh at least) use the fact that the tests are ran in $HOME. I'm not sure if we want to change that.

About XDG_CONFIG_HOME, the tests were just assuming it was unset.
> +GIT_CONFIG_DIR=$XDG_CONFIG_HOME/git

As I said in my earlier message, I'd rather test the default value ($HOME/.config/git) than the particular case whe $XDG_CONFIG_HOME is set.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Michael Witten· Jul 17, 2012, 17:44 UTC · re: Michael Witten · lore

[PATCH 3/3] Testing: XDG config files: Trivial: `xdg' -> `XDG'

For the sake of consistency and correctness, the acronymn `xdg' has been capitalized (`XDG').

Signed-off-by: Michael Witten <mfwitten@gmail.com>
---
 t/t1306-xdg-files.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Show changes to t/t1306-xdg-files.sh +8 −8
diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 2327047..6a3ab09 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -11,7 +11,7 @@
 
 GIT_CONFIG_DIR=$XDG_CONFIG_HOME/git
 
-test_expect_success 'read config: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
+test_expect_success 'read config: XDG file exists and "$HOME"/.gitconfig doesn'\''t' '
 	mkdir -p "$GIT_CONFIG_DIR" &&
 	echo "[alias]" >"$GIT_CONFIG_DIR"/config &&
 	echo "	myalias = !echo in_config" >>"$GIT_CONFIG_DIR"/config &&
@@ -21,7 +21,7 @@
 '
 
 
-test_expect_success 'read config: xdg file exists and "$HOME"/.gitconfig exists' '
+test_expect_success 'read config: XDG file exists and "$HOME"/.gitconfig exists' '
 	>"$HOME"/.gitconfig &&
 	echo "[alias]" >"$HOME"/.gitconfig &&
 	echo "	myalias = !echo in_gitconfig" >>"$HOME"/.gitconfig &&
@@ -31,7 +31,7 @@
 '
 
 
-test_expect_success 'read with --get: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
+test_expect_success 'read with --get: XDG file exists and "$HOME"/.gitconfig doesn'\''t' '
 	rm "$HOME"/.gitconfig &&
 	echo "[user]" >"$GIT_CONFIG_DIR"/config &&
 	echo "	name = read_config" >>"$GIT_CONFIG_DIR"/config &&
@@ -41,7 +41,7 @@
 '
 
 
-test_expect_success 'read with --get: xdg file exists and "$HOME"/.gitconfig exists' '
+test_expect_success 'read with --get: XDG file exists and "$HOME"/.gitconfig exists' '
 	>"$HOME"/.gitconfig &&
 	echo "[user]" >"$HOME"/.gitconfig &&
 	echo "	name = read_gitconfig" >>"$HOME"/.gitconfig &&
@@ -51,7 +51,7 @@
 '
 
 
-test_expect_success 'read with --list: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
+test_expect_success 'read with --list: XDG file exists and "$HOME"/.gitconfig doesn'\''t' '
 	rm "$HOME"/.gitconfig &&
 	echo user.name=read_config >expected &&
 	git config --global --list >actual &&
@@ -59,7 +59,7 @@
 '
 
 
-test_expect_success 'read with --list: xdg file exists and "$HOME"/.gitconfig exists' '
+test_expect_success 'read with --list: XDG file exists and "$HOME"/.gitconfig exists' '
 	>"$HOME"/.gitconfig &&
 	echo "[user]" >"$HOME"/.gitconfig &&
 	echo "	name = read_gitconfig" >>"$HOME"/.gitconfig &&
@@ -127,7 +127,7 @@
 '
 
 
-test_expect_success 'write: xdg file exists and "$HOME"/.gitconfig doesn'\''t' '
+test_expect_success 'write: XDG file exists and "$HOME"/.gitconfig doesn'\''t' '
 	mkdir -p "$GIT_CONFIG_DIR" &&
 	>"$GIT_CONFIG_DIR"/config &&
 	test_might_fail rm "$HOME"/.gitconfig &&
@@ -138,7 +138,7 @@
 '
 
 
-test_expect_success 'write: xdg file exists and "$HOME"/.gitconfig exists' '
+test_expect_success 'write: XDG file exists and "$HOME"/.gitconfig exists' '
 	>"$HOME"/.gitconfig &&
 	git config --global user.name "write_gitconfig" &&
 	echo "[user]" >expected &&

-- 
1.7.11.1.29.gf71be5c

← back to recent threads