Merge pull request git-lfs#5879 from chrisd8088/tests-cleanup-dirs-fix-counts

Run tests in parallel on Windows and always cleanup test directories

Author: chrisd8088

.github/workflows/ci.yml

@@ -81,7 +81,6 @@ jobs:
       shell: bash
     - run: set GOPATH=%HOME%\go
     - run: choco install -y InnoSetup
-    - run: choco install -y strawberryperl
     - run: make man
       shell: bash
     - run: GOPATH="$HOME/go" PATH="$HOME/go/bin:$PATH" go install github.com/josephspurrier/goversioninfo/cmd/goversioninfo@latest

.github/workflows/release.yml

@@ -33,7 +33,6 @@ jobs:
       shell: bash
     - run: set GOPATH=%HOME%\go
     - run: choco install -y InnoSetup
-    - run: choco install -y strawberryperl
     - run: choco install -y zip
     - run: choco install -y jq
     - run: GOPATH="$HOME/go" PATH="$HOME/go/bin:$PATH" go install github.com/josephspurrier/goversioninfo/cmd/goversioninfo@latest

rpm/SPECS/git-lfs.spec

@@ -71,7 +71,7 @@ export SKIPAPITESTCOMPILE=1
 
 pushd src/github.com/git-lfs/%{name}
   make test
-  make -C t PROVE_EXTRA_ARGS=-j4 test
+  make -C t PROVE_EXTRA_ARGS=-j9 test
 popd
 
 rm -rf ${GIT_LFS_TEST_DIR}

script/cibuild

@@ -9,8 +9,6 @@ X=""
 if [[ $UNAME == MINGW* || $UNAME == MSYS* || $UNAME == CYGWIN* ]]; then
   X=".exe"
   WINDOWS=1
-  export GIT_LFS_NO_TEST_COUNT=1
-  export GIT_LFS_LOCK_ACQUIRE_DISABLED=1
 fi
 
 # Build git-lfs-transfer from scutiger.
@@ -30,12 +28,6 @@ GIT_TRACE=1 make GOIMPORTS="$GOIMPORTS" PKGS=git test
 pushd t >/dev/null
   PROVE="prove"
   PROVE_EXTRA_ARGS="-j9"
-  if [ "$WINDOWS" ]; then
-    export PATH="/c/Strawberry/perl/bin:.:$PATH"
-    PROVE="prove.bat"
-    PROVE_EXTRA_ARGS="$PROVE_EXTRA_ARGS --exec bash"
-  fi
-
   VERBOSE_LOGS=1 make X="$X" clean
   VERBOSE_LOGS=1 make X="$X" PROVE="$PROVE" PROVE_EXTRA_ARGS="$PROVE_EXTRA_ARGS"
 popd >/dev/null

t/Makefile

@@ -45,9 +45,9 @@ test-commands : $(TEST_CMDS)
 
 test : test-commands
 	$(RM) -r remote test_count{,.lock}
-	@GIT_LFS_NO_TEST_COUNT= bash -c '. ./testenv.sh && setup'
-	$(PROVE) $(PROVE_EXTRA_ARGS) ./t-*.sh
-	@GIT_LFS_NO_TEST_COUNT= bash -c '. ./testenv.sh && shutdown'
+	@bash -c ". ./testenv.sh && setup && cd t && \
+		RM_GIT_LFS_TEST_DIR=no $(PROVE) $(PROVE_EXTRA_ARGS) t-*.sh && \
+		shutdown"
 
 .PHONY : $(TEST_SRCS)
 $(TEST_SRCS) : $(TEST_CMDS)

t/cmd/lfstest-count-tests.go

@@ -62,7 +62,11 @@ func main() {
 	if err := acquire(ctx); err != nil {
 		fatal(err)
 	}
-	defer release()
+	defer func() {
+		if err := release(); err != nil {
+			fmt.Fprintf(os.Stderr, "unable to release lock file: %s\n", err)
+		}
+	}()
 
 	if len(os.Args) == 1 {
 		// Calling with no arguments indicates that we simply want to
@@ -166,10 +170,6 @@ var (
 // acquire acquires the lock file necessary to perform updates to test_count,
 // and returns an error if that lock cannot be acquired.
 func acquire(ctx context.Context) error {
-	if disabled() {
-		return nil
-	}
-
 	path, err := path(lockFile)
 	if err != nil {
 		return err
@@ -183,8 +183,11 @@ func acquire(ctx context.Context) error {
 		case <-tick.C:
 			// Try every tick of the above ticker before giving up
 			// and trying again.
-			_, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL, 0666)
+			f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666)
 			if err == nil || !os.IsExist(err) {
+				if err == nil {
+					f.Close()
+				}
 				return err
 			}
 		case <-ctx.Done():
@@ -198,10 +201,6 @@ func acquire(ctx context.Context) error {
 // release releases the lock file so that another process can take over, or
 // returns an error.
 func release() error {
-	if disabled() {
-		return nil
-	}
-
 	path, err := path(lockFile)
 	if err != nil {
 		return err
@@ -266,22 +265,12 @@ func callWithCount(fn countFn) error {
 // 't' directory of the current checkout of Git LFS.
 func path(s string) (string, error) {
 	p := filepath.Join(filepath.Dir(os.Getenv("LFSTEST_DIR")), s)
-	if err := os.MkdirAll(filepath.Dir(p), 0666); err != nil {
+	if err := os.MkdirAll(filepath.Dir(p), 0777); err != nil {
 		return "", err
 	}
 	return p, nil
 }
 
-// disabled returns true if and only if the lock acquisition phase is disabled.
-func disabled() bool {
-	s := os.Getenv("GIT_LFS_LOCK_ACQUIRE_DISABLED")
-	b, err := strconv.ParseBool(s)
-	if err != nil {
-		return false
-	}
-	return b
-}
-
 // fatal reports the given error (if non-nil), and then dies. If the error was
 // nil, nothing happens.
 func fatal(err error) {

t/t-env.sh

@@ -6,16 +6,14 @@ envInitConfig='git config filter.lfs.process = "git-lfs filter-process"
 git config filter.lfs.smudge = "git-lfs smudge -- %f"
 git config filter.lfs.clean = "git-lfs clean -- %f"'
 
-unset_vars() {
-    # If set, these will cause the test to fail.
-    unset GIT_LFS_NO_TEST_COUNT GIT_LFS_LOCK_ACQUIRE_DISABLED
-}
+if [ "$IS_WINDOWS" -eq 1 ]; then
+  export MSYS2_ENV_CONV_EXCL="GIT_LFS_TEST_DIR"
+fi
 
 begin_test "env with no remote"
 (
   set -e
   reponame="env-no-remote"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -67,7 +65,6 @@ begin_test "env with origin remote"
 (
   set -e
   reponame="env-origin-remote"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -125,7 +122,6 @@ begin_test "env with multiple remotes"
 (
   set -e
   reponame="env-multiple-remotes"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -186,7 +182,6 @@ begin_test "env with other remote"
 (
   set -e
   reponame="env-other-remote"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -245,7 +240,6 @@ begin_test "env with multiple remotes and lfs.url config"
 (
   set -e
   reponame="env-multiple-remotes-with-lfs-url"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -305,7 +299,6 @@ begin_test "env with multiple remotes and lfs configs"
 (
   set -e
   reponame="env-multiple-remotes-lfs-configs"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -367,7 +360,6 @@ begin_test "env with multiple remotes and batch configs"
 (
   set -e
   reponame="env-multiple-remotes-lfs-batch-configs"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -429,8 +421,6 @@ begin_test "env with .lfsconfig"
 (
   set -e
   reponame="env-with-lfsconfig"
-  unset_vars
-
   git init $reponame
   cd $reponame
 
@@ -499,7 +489,6 @@ begin_test "env with environment variables"
 (
   set -e
   reponame="env-with-envvars"
-  unset_vars
   git init $reponame
   mkdir -p $reponame/a/b/c
 
@@ -677,7 +666,6 @@ begin_test "env with bare repo"
 (
   set -e
   reponame="env-with-bare-repo"
-  unset_vars
   git init --bare $reponame
   cd $reponame
 
@@ -725,7 +713,6 @@ begin_test "env with multiple ssh remotes"
 (
   set -e
   reponame="env-with-ssh"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -877,7 +864,6 @@ begin_test "env with extra transfer methods"
 (
   set -e
   reponame="env-with-transfers"
-  unset_vars
   git init $reponame
   cd $reponame
 
@@ -938,7 +924,6 @@ begin_test "env with multiple remotes and ref"
 (
   set -e
   reponame="env-multiple-remotes-ref"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -1001,7 +986,6 @@ begin_test "env with unicode"
   # This contains a Unicode apostrophe, an E with grave accent, and a Euro sign.
   # Only the middle one is representable in ISO-8859-1.
   reponame="env-d’autre-nom-très-bizarr€"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init
@@ -1063,7 +1047,6 @@ end_test
 begin_test "env outside a repository"
 (
   set -e
-  unset_vars
 
   # This may or may not work, depending on the system, but it should at least
   # potentially cause Git to print non-English messages.
@@ -1113,7 +1096,6 @@ begin_test "env with duplicate endpoints"
 (
   set -e
   reponame="env-duplicate-endpoints"
-  unset_vars
   mkdir $reponame
   cd $reponame
   git init

t/t-worktree.sh

@@ -7,16 +7,14 @@ envInitConfig='git config filter.lfs.process = "git-lfs filter-process"
 git config filter.lfs.smudge = "git-lfs smudge -- %f"
 git config filter.lfs.clean = "git-lfs clean -- %f"'
 
-unset_vars () {
-    # If set, these will cause the test to fail.
-    unset GIT_LFS_NO_TEST_COUNT GIT_LFS_LOCK_ACQUIRE_DISABLED
-}
+if [ "$IS_WINDOWS" -eq 1 ]; then
+  export MSYS2_ENV_CONV_EXCL="GIT_LFS_TEST_DIR"
+fi
 
 begin_test "git worktree"
 (
     set -e
     reponame="worktree-main"
-    unset_vars
     mkdir $reponame
     cd $reponame
     git init
@@ -99,7 +97,6 @@ begin_test "git worktree with hooks"
 (
     set -e
     reponame="worktree-hooks"
-    unset_vars
     mkdir $reponame
     cd $reponame
     git init

t/testenv.sh

@@ -62,7 +62,6 @@ resolve_symlink() {
   else
     readlink -f "$arg"
   fi
-
 }
 
 # The root directory for the git-lfs repository by default.
@@ -84,11 +83,17 @@ else
 fi
 
 if [ -z "$GIT_LFS_TEST_DIR" ]; then
-    GIT_LFS_TEST_DIR=$(mktemp -d -t "$TEMPDIR_PREFIX")
-    GIT_LFS_TEST_DIR=$(resolve_symlink $GIT_LFS_TEST_DIR)
+    GIT_LFS_TEST_DIR="$(mktemp -d -t "$TEMPDIR_PREFIX")"
+    GIT_LFS_TEST_DIR="$(resolve_symlink "$GIT_LFS_TEST_DIR")"
     # cleanup either after single test or at end of integration (except on fail)
-    RM_GIT_LFS_TEST_DIR=yes
+    RM_GIT_LFS_TEST_DIR="yes"
 fi
+
+# Make these variables available to all test files run in the same shell,
+# particularly when setup() is run first by itself to start a single
+# common lfstest-gitserver instance.
+export GIT_LFS_TEST_DIR RM_GIT_LFS_TEST_DIR
+
 # create a temporary work space
 TMPDIR=$GIT_LFS_TEST_DIR
 
@@ -111,10 +116,6 @@ REMOTEDIR="$ROOTDIR/t/remote"
 #
 CREDSDIR="$REMOTEDIR/creds/"
 
-# This is the prefix for Git config files.  See the "Test Suite" section in
-# t/README.md
-LFS_CONFIG="$REMOTEDIR/config"
-
 # This file contains the URL of the test Git server. See the "Test Suite"
 # section in t/README.md
 LFS_URL_FILE="$REMOTEDIR/url"

t/testhelpers.sh

@@ -578,17 +578,15 @@ setup() {
   git lfs version | sed -e 's/^/# /g'
   git version | sed -e 's/^/# /g'
 
-  if [ -z "$GIT_LFS_NO_TEST_COUNT" ]; then
-    LFSTEST_URL="$LFS_URL_FILE" \
-    LFSTEST_SSL_URL="$LFS_SSL_URL_FILE" \
-    LFSTEST_CLIENT_CERT_URL="$LFS_CLIENT_CERT_URL_FILE" \
-    LFSTEST_DIR="$REMOTEDIR" \
-    LFSTEST_CERT="$LFS_CERT_FILE" \
-    LFSTEST_CLIENT_CERT="$LFS_CLIENT_CERT_FILE" \
-    LFSTEST_CLIENT_KEY="$LFS_CLIENT_KEY_FILE" \
-    LFSTEST_CLIENT_KEY_ENCRYPTED="$LFS_CLIENT_KEY_FILE_ENCRYPTED" \
-      lfstest-count-tests increment
-  fi
+  LFSTEST_URL="$LFS_URL_FILE" \
+  LFSTEST_SSL_URL="$LFS_SSL_URL_FILE" \
+  LFSTEST_CLIENT_CERT_URL="$LFS_CLIENT_CERT_URL_FILE" \
+  LFSTEST_DIR="$REMOTEDIR" \
+  LFSTEST_CERT="$LFS_CERT_FILE" \
+  LFSTEST_CLIENT_CERT="$LFS_CLIENT_CERT_FILE" \
+  LFSTEST_CLIENT_KEY="$LFS_CLIENT_KEY_FILE" \
+  LFSTEST_CLIENT_KEY_ENCRYPTED="$LFS_CLIENT_KEY_FILE_ENCRYPTED" \
+    lfstest-count-tests increment
 
   wait_for_file "$LFS_URL_FILE"
   wait_for_file "$LFS_SSL_URL_FILE"
@@ -646,10 +644,13 @@ shutdown() {
   # every t/t-*.sh file should cleanup its trashdir
   [ -z "$KEEPTRASH" ] && rm -rf "$TRASHDIR"
 
-  if [ -z "$GIT_LFS_NO_TEST_COUNT" ]; then
-    LFSTEST_DIR="$REMOTEDIR" \
-    LFS_URL_FILE="$LFS_URL_FILE" \
-      lfstest-count-tests decrement
+  LFSTEST_DIR="$REMOTEDIR" \
+  LFS_URL_FILE="$LFS_URL_FILE" \
+    lfstest-count-tests decrement
+
+  # delete entire lfs test root if we created it (double check pattern)
+  if [ -z "$KEEPTRASH" ] && [ "$RM_GIT_LFS_TEST_DIR" = "yes" ] && [[ $GIT_LFS_TEST_DIR == *"$TEMPDIR_PREFIX"* ]]; then
+    rm -rf "$GIT_LFS_TEST_DIR"
   fi
 }