Fix race conditions and add hooks (#500)

Summary:
**Added:**
- Created `.hooks/run-go-tests.sh` to run Go tests with options for coverage,
  all tests, short tests, and tests for modified files.
- Added `go-unit-tests` pre-commit hook to `.pre-commit-config.yaml` to run
  unit tests on modified Go files.
- Addressed multiple race conditions in tests
  - Introduced `logMutex` to sync logging in `cmd/run_test.go`.
  - Added `initOnce` to ensure `InitLog` runs only once in `pkg/logging/logger.go`

**Changed:**
- Updated copyright in `pkg/logging/logger.go` and `pkg/logging/logger_test.go`
  to © 2024-present.
- Refactored `pkg/logging/logger.go` to use `sync.Once` for logger initialization.

Pull Request resolved: #500

Reviewed By: d0n601

Differential Revision: D59009307

Pulled By: l50

fbshipit-source-id: a8223b2163c02fe4d306da91ad5a15a748c9382c

.hooks/go-copyright.sh

@@ -3,7 +3,7 @@
 set -ex
 
 copyright_header='/*
-Copyright © 2023-present, Meta Platforms, Inc. and affiliates
+Copyright © 2024-present, Meta Platforms, Inc. and affiliates
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal
 in the Software without restriction, including without limitation the rights

.hooks/go-licenses.sh

@@ -0,0 +1,43 @@
+#!/bin/bash
+
+# Function to check if go mod vendor should run or not
+run_vendor() {
+    echo "Running go mod vendor..."
+    go mod vendor
+}
+
+# Function to check licenses
+check_licenses() {
+    action=$1
+
+    go install github.com/google/go-licenses@latest
+
+    # Decide action based on input
+    if [[ $action == "check_forbidden" ]]; then
+        echo "Checking for forbidden licenses..."
+        output=$(go-licenses check ./... 2> /dev/null)
+        if [[ "${output}" == *"ERROR: forbidden license found"* ]]; then
+            echo "Forbidden licenses found. Please remove them."
+            exit 1
+        else
+            echo "No forbidden licenses found."
+        fi
+    elif [[ $action == "output_csv" ]]; then
+        echo "Outputting licenses to csv..."
+        status=go-licenses csv ./... 2> /dev/null
+    elif [[ $action == "vendor" ]]; then
+        echo "Vendoring dependencies..."
+        run_vendor
+    fi
+}
+
+# Ensure input is provided
+if [[ $# -lt 1 ]]; then
+    echo "Incorrect number of arguments."
+    echo "Usage: $0 <licenses action>"
+    echo "Example: $0 check_forbidden"
+    exit 1
+fi
+
+# Run checks
+check_licenses "${1}"

.hooks/go-vet.sh

@@ -0,0 +1,11 @@
+#!/bin/bash
+set -e
+
+pkgs=$(go list ./...)
+
+for pkg in $pkgs; do
+    dir="$(basename "$pkg")/"
+    if [[ "${dir}" != .*/ ]]; then
+        go vet "${pkg}"
+    fi
+done

.hooks/run-go-tests.sh

@@ -0,0 +1,91 @@
+#!/bin/bash
+
+set -e
+
+TESTS_TO_RUN=$1
+PROJECT=TTPFORGE
+RETURN_CODE=0
+
+TIMESTAMP=$(date +"%Y%m%d%H%M%S")
+LOGFILE="/tmp/$PROJECT-unit-test-results-$TIMESTAMP.log"
+MODULE_ROOT=$(go list -m -f "{{.Dir}}")
+
+if [[ -z "${TESTS_TO_RUN}" ]]; then
+    echo "No tests input" | tee -a "$LOGFILE"
+    echo "Example - Run all shorter collection of tests: bash run-go-tests.sh short" | tee -a "$LOGFILE"
+    echo "Example - Run all tests: bash run-go-tests.sh all" | tee -a "$LOGFILE"
+    echo "Example - Run coverage for a specific version: bash run-go-tests.sh coverage" | tee -a "$LOGFILE"
+    echo "Example - Run tests for modified files: bash run-go-tests.sh modified" | tee -a "$LOGFILE"
+    exit 1
+fi
+
+run_tests() {
+    local coverage_file=$1
+    repo_root=$(git rev-parse --show-toplevel 2> /dev/null) || exit
+    pushd "${repo_root}" || exit
+    echo "Logging output to ${LOGFILE}" | tee -a "$LOGFILE"
+    echo "Run the following command to see the output in real time:" | tee -a "$LOGFILE"
+    echo "tail -f ${LOGFILE}" | tee -a "$LOGFILE"
+    echo "Running tests..." | tee -a "$LOGFILE"
+
+    # Check if go.mod and go.sum exist
+    if [[ -f "go.mod" && -f "go.sum" ]]; then
+        # Check if `go mod tidy` is necessary
+        MOD_TMP=$(mktemp)
+        SUM_TMP=$(mktemp)
+        cp go.mod "$MOD_TMP"
+        cp go.sum "$SUM_TMP"
+        go mod tidy
+        if ! cmp -s go.mod "$MOD_TMP" || ! cmp -s go.sum "$SUM_TMP"; then
+            echo "Running 'go mod tidy' to clean up module dependencies..." | tee -a "$LOGFILE"
+            go mod tidy 2>&1 | tee -a "$LOGFILE"
+        fi
+        rm "$MOD_TMP" "$SUM_TMP"
+    fi
+
+    if [[ "${TESTS_TO_RUN}" == 'coverage' ]]; then
+        go test -v -race -failfast -tags=integration -coverprofile="${coverage_file}" ./... 2>&1 | tee -a "$LOGFILE"
+    elif [[ "${TESTS_TO_RUN}" == 'all' ]]; then
+        go test -v -race -failfast ./... 2>&1 | tee -a "$LOGFILE"
+    elif [[ "${TESTS_TO_RUN}" == 'short' ]] && [[ "${GITHUB_ACTIONS}" != "true" ]]; then
+        go test -v -short -failfast -race ./... 2>&1 | tee -a "$LOGFILE"
+    elif [[ "${TESTS_TO_RUN}" == 'modified' ]]; then
+        # Run tests for modified files
+        local modified_files
+        IFS=$'\n' read -r -a modified_files <<< "$(git diff --name-only --cached | grep '\.go$')"
+
+        local pkg_dirs=()
+
+        for file in "${modified_files[@]}"; do
+            local pkg_dir
+            pkg_dir=$(dirname "$file")
+            pkg_dir=${pkg_dir#"$MODULE_ROOT/"}
+            pkg_dirs+=("$pkg_dir")
+        done
+
+        # Remove duplicate package directories
+        IFS=$'\n' read -r -a pkg_dirs <<< "$(sort -u <<< "${pkg_dirs[*]}")"
+        unset IFS
+
+        for dir in "${pkg_dirs[@]}"; do
+            go test -v -race -failfast "./$dir/..." 2>&1 | tee -a "$LOGFILE"
+        done
+    else
+        if [[ "${GITHUB_ACTIONS}" != 'true' ]]; then
+            go test -v -failfast -race "./.../${TESTS_TO_RUN}" 2>&1 | tee -a "$LOGFILE"
+        fi
+    fi
+
+    RETURN_CODE=$?
+}
+
+if [[ "${TESTS_TO_RUN}" == 'coverage' ]]; then
+    run_tests 'coverage-all.out'
+else
+    run_tests
+fi
+
+if [[ "${RETURN_CODE}" -ne 0 ]]; then
+    echo "unit tests failed" | tee -a "$LOGFILE"
+    exit 1
+fi

.pre-commit-config.yaml

@@ -28,6 +28,38 @@ repos:
         language: script
         files: go.mod
 
+      - id: go-unit-tests
+        name: Go unit tests
+        language: script
+        entry: .hooks/run-go-tests.sh modified
+        files: '\.go$'
+        pass_filenames: true
+
+      - id: go-vet
+        name: Run go vet
+        language: script
+        entry: .hooks/go-vet.sh
+        files: '\.go$'
+        always_run: true
+        pass_filenames: true
+        require_serial: true
+        log_file: /tmp/go-vet.log
+
+      - id: go-licenses
+        name: Run go-licenses
+        language: script
+        entry: .hooks/go-licenses.sh check_forbidden
+
+      - id: go-vet
+        name: Run go vet
+        language: script
+        entry: .hooks/go-vet.sh
+        files: '\.go$'
+        always_run: true
+        pass_filenames: true
+        require_serial: true
+        log_file: /tmp/go-vet.log
+
       - id: go-copyright
         name: Ensure all go files have the copyright header
         language: script

cmd/run_test.go

@@ -23,7 +23,7 @@ import (
 	"bytes"
 	"os"
 	"path/filepath"
-
+	"sync"
 	"testing"
 
 	"github.com/spf13/afero"
@@ -44,14 +44,18 @@ type runCmdTestCase struct {
 	wantError      bool
 }
 
+var logMutex sync.Mutex
+
 func checkRunCmdTestCase(t *testing.T, tc runCmdTestCase) {
 	var stdoutBuf, stderrBuf bytes.Buffer
 	rc := BuildRootCommand(&TestConfig{
 		Stdout: &stdoutBuf,
 		Stderr: &stderrBuf,
 	})
 	rc.SetArgs(append([]string{"run"}, tc.args...))
+	logMutex.Lock()
 	err := rc.Execute()
+	logMutex.Unlock()
 	if tc.wantError {
 		require.Error(t, err)
 		return
@@ -155,11 +159,9 @@ func TestRun(t *testing.T) {
 	}
 }
 
-// TestRunPathArguments checks that
-// referencing relative paths in `--arg` values
-// when executing `ttpforge run` works as expected.
-// One typically needs to specify `type: path` in
-// the argument specification in order to get desired
+// TestRunPathArguments checks that referencing relative paths in `--arg` values
+// when executing `ttpforge run` works as expected. One typically needs to
+// specify `type: path` in the argument specification in order to get desired
 // behavior.
 func TestRunPathArguments(t *testing.T) {
 	// in this test, we initially execute every test case from a

go.mod

@@ -5,6 +5,7 @@ go 1.21
 require (
 	github.com/Masterminds/sprig/v3 v3.2.3
 	github.com/google/go-cmp v0.6.0
+	github.com/google/uuid v1.6.0
 	github.com/otiai10/copy v1.14.0
 	github.com/spf13/afero v1.11.0
 	github.com/spf13/cobra v1.8.0
@@ -18,7 +19,6 @@ require (
 	github.com/Masterminds/goutils v1.1.1 // indirect
 	github.com/Masterminds/semver/v3 v3.2.1 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
-	github.com/google/uuid v1.6.0 // indirect
 	github.com/huandu/xstrings v1.4.0 // indirect
 	github.com/imdario/mergo v0.3.16 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect

pkg/logging/logger.go

@@ -1,5 +1,5 @@
 /*
-Copyright © 2023-present, Meta Platforms, Inc. and affiliates
+Copyright © 2024-present, Meta Platforms, Inc. and affiliates
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal
 in the Software without restriction, including without limitation the rights
@@ -21,6 +21,7 @@ package logging
 
 import (
 	"path/filepath"
+	"sync"
 
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
@@ -34,7 +35,10 @@ type Config struct {
 	Stacktrace bool
 }
 
-var logger *zap.SugaredLogger
+var (
+	logger   *zap.SugaredLogger
+	initOnce sync.Once
+)
 
 func init() {
 	// default logger - will be used in tests
@@ -65,40 +69,38 @@ func DividerThin() {
 
 // InitLog initializes TTPForge global logger
 func InitLog(config Config) (err error) {
-	zcfg := zap.NewDevelopmentConfig()
-	if config.NoColor {
-		zcfg.EncoderConfig.EncodeLevel = zapcore.CapitalLevelEncoder
-	} else {
-		zcfg.EncoderConfig.EncodeLevel = zapcore.CapitalColorLevelEncoder
-	}
-
-	// setup Logger to write to file if provided
-	if config.LogFile != "" {
-		fullpath, err := filepath.Abs(config.LogFile)
-		if err != nil {
-			return err
+	initOnce.Do(func() {
+		zcfg := zap.NewDevelopmentConfig()
+		if config.NoColor {
+			zcfg.EncoderConfig.EncodeLevel = zapcore.CapitalLevelEncoder
+		} else {
+			zcfg.EncoderConfig.EncodeLevel = zapcore.CapitalColorLevelEncoder
 		}
-		zcfg.OutputPaths = append(zcfg.OutputPaths, fullpath)
-	}
 
-	if config.Verbose {
-		zcfg.Level.SetLevel(zap.DebugLevel)
-	} else {
-		zcfg.Level.SetLevel(zap.InfoLevel)
-		// hide fields that will confuse users during simple user errors
-		zcfg.EncoderConfig.CallerKey = zapcore.OmitKey
-		zcfg.EncoderConfig.TimeKey = zapcore.OmitKey
-	}
+		if config.LogFile != "" {
+			fullpath, err := filepath.Abs(config.LogFile)
+			if err != nil {
+				panic(err) // Use panic here since sync.Once does not allow error return
+			}
+			zcfg.OutputPaths = append(zcfg.OutputPaths, fullpath)
+		}
 
-	if !config.Stacktrace {
-		zcfg.DisableStacktrace = true
-	}
+		if config.Verbose {
+			zcfg.Level.SetLevel(zap.DebugLevel)
+		} else {
+			zcfg.Level.SetLevel(zap.InfoLevel)
+			zcfg.EncoderConfig.CallerKey = zapcore.OmitKey
+			zcfg.EncoderConfig.TimeKey = zapcore.OmitKey
+		}
+		if !config.Stacktrace {
+			zcfg.DisableStacktrace = true
+		}
 
-	// use sugared logger
-	baseLogger, err := zcfg.Build()
-	if err != nil {
-		return err
-	}
-	logger = baseLogger.Sugar()
+		baseLogger, err := zcfg.Build()
+		if err != nil {
+			panic(err) // Use panic here since sync.Once does not allow error return
+		}
+		logger = baseLogger.Sugar()
+	})
 	return nil
 }

pkg/logging/logger_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright © 2023-present, Meta Platforms, Inc. and affiliates
+Copyright © 2024-present, Meta Platforms, Inc. and affiliates
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal
 in the Software without restriction, including without limitation the rights
@@ -21,6 +21,7 @@ package logging
 
 import (
 	"os"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -38,6 +39,7 @@ import (
 func TestInitLogStacktraceNoLogfile(t *testing.T) {
 	core, recordedLogs := observer.New(zapcore.InfoLevel)
 
+	initOnce = sync.Once{} // Reset the sync.Once for testing purposes
 	err := InitLog(Config{
 		Stacktrace: true,
 	})
@@ -101,7 +103,7 @@ func TestInitLog(t *testing.T) {
 			cfg := tc.config
 			cfg.LogFile = logFile
 
-			// initialize the logger for this test case
+			initOnce = sync.Once{} // Reset the sync.Once for testing purposes
 			err = InitLog(cfg)
 			require.NoError(t, err)