refactor: replace t.Skip with t.Skipf throughout codebase (#1449)

* refactor: replace t.Skip with t.Skipf and improve test skipping

- Replace all t.Skip() calls with t.Skipf() providing descriptive reasons
- Update CLI test infrastructure to use skipReason instead of fatal exits
- Add golangci-lint rule to enforce t.Skipf usage
- Update CLAUDE.md with test skipping conventions
- Ensure TestMain always calls m.Run() for proper test reporting

This change improves test debugging by providing clear reasons when tests
are skipped, and ensures that missing/stale binaries result in properly
reported skipped tests rather than fatal exits.

Co-Authored-By: Claude <noreply@anthropic.com>

* chore: add tests.test to .gitignore

* [autofix.ci] apply automated fixes

* fix: re-add forbidigo to test file exclusions

The forbidigo linter needs to be excluded from test files because tests
legitimately need to use os.Getenv and os.Setenv for setting up test
environments. With 223 occurrences across 44 test files, it's not
practical to add nolint comments to each one.

The t.Skip check will still be enforced through code review and the
documented convention in CLAUDE.md.

* docs: fix CLAUDE.md to show t.Skipf in all examples

Updated the test skipping convention examples to consistently use t.Skipf()
instead of t.Skip(), ensuring the documentation matches the actual
implementation and reinforces the correct pattern.

* docs: clarify TestMain must call os.Exit(m.Run()) for proper exit codes

Updated CLAUDE.md to explicitly state that TestMain must call os.Exit(m.Run())
to propagate test exit codes, and clarified that skipReason should be set
before calling m.Run(). This ensures proper test process termination and
exit code propagation.

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

Author: 3 people

.gitignore

@@ -51,3 +51,4 @@ internal/exec/output.*
 # Coverage files
 coverage.out
 coverage.html
+tests.test

.golangci.yml

@@ -37,6 +37,8 @@ linters:
       forbid:
         - pattern: os\.Getenv
           msg: Use `viper.BindEnv` for new environment variables instead of `os.Getenv`
+        - pattern: '\.Skip\('
+          msg: Use `t.Skipf` with a descriptive reason instead of `t.Skip`
     funlen:
       lines: 60
       statements: 40

CLAUDE.md

@@ -154,6 +154,24 @@ viper.SetEnvPrefix("ATMOS")
 - Target >80% coverage, especially for `pkg/` and `internal/exec/`
 - **Comments must end with periods**: All comments should be complete sentences ending with a period (enforced by golangci-lint)
 
+### Test Skipping Conventions (MANDATORY)
+- **ALWAYS use `t.Skipf()` instead of `t.Skip()`** - Provide clear reasons for skipped tests
+- **NEVER use `t.Skipf()` without a reason**
+- Examples:
+  ```go
+  // WRONG: No reason provided
+  t.Skipf("Skipping test")
+  
+  // CORRECT: Clear reason with context
+  t.Skipf("Skipping symlink test on Windows: symlinks require special privileges")
+  t.Skipf("Skipping test: %s", dynamicReason)
+  ```
+- **For CLI tests that depend on rebuilt binaries**:
+  - Set package-level `skipReason` variable in `TestMain` before calling `m.Run()`
+  - Individual test functions check and skip with `t.Skipf()` if set
+  - TestMain MUST call `os.Exit(m.Run())` to propagate the test exit code
+  - Never use `log.Fatal()` for missing/stale binaries - set `skipReason` instead
+
 ### CLI Command Structure & Examples
 Atmos uses **embedded markdown files** for maintainable examples:
 

internal/exec/copy_glob_test.go

@@ -107,7 +107,7 @@ func TestShouldExcludePath(t *testing.T) {
 // Skipped on Windows.
 func TestShouldExcludePath_Directory(t *testing.T) {
 	if runtime.GOOS == "windows" {
-		t.Skip("Skipping directory exclusion test on Windows")
+		t.Skipf("Skipping directory exclusion test on Windows: path handling differs")
 	}
 	dir, err := os.MkdirTemp("", "dir-exclude")
 	if err != nil {
@@ -242,7 +242,7 @@ func TestCopyDirRecursive(t *testing.T) {
 // TestProcessDirEntry_Symlink ensures that symlink entries are skipped.
 func TestProcessDirEntry_Symlink(t *testing.T) {
 	if runtime.GOOS == "windows" {
-		t.Skip("Skipping symlink test on Windows")
+		t.Skipf("Skipping symlink test on Windows: symlinks require special privileges")
 	}
 	srcDir, err := os.MkdirTemp("", "symlink-src")
 	if err != nil {
@@ -260,7 +260,7 @@ func TestProcessDirEntry_Symlink(t *testing.T) {
 	}
 	linkPath := filepath.Join(srcDir, "link.txt")
 	if err := os.Symlink(targetFile, linkPath); err != nil {
-		t.Skip("Cannot create symlink on this system, skipping test.")
+		t.Skipf("Cannot create symlink on this system: insufficient privileges or unsupported filesystem")
 	}
 	entries, err := os.ReadDir(srcDir)
 	if err != nil {
@@ -352,7 +352,7 @@ func TestGetMatchesForPattern_InvalidPattern(t *testing.T) {
 // TestGetMatchesForPattern_ShallowNoMatch tests the shallow branch with no matches.
 func TestGetMatchesForPattern_ShallowNoMatch(t *testing.T) {
 	if runtime.GOOS == "windows" {
-		t.Skip("Skipping shallow no-match test on Windows")
+		t.Skipf("Skipping shallow no-match test on Windows: glob behavior differs")
 	}
 	oldFn := getGlobMatchesForTest
 	defer func() { getGlobMatchesForTest = oldFn }()
@@ -745,7 +745,7 @@ func TestCopyFile_FailChmod(t *testing.T) {
 	dstFile := filepath.Join(dstDir, "test.txt")
 	err = copyFile(srcFile, dstFile)
 	if err == nil {
-		t.Skip("os.Chmod patch not effective on this platform")
+		t.Skipf("Skipping test: os.Chmod not effective on this platform")
 	}
 	if !strings.Contains(err.Error(), "setting permissions") {
 		t.Errorf("Expected chmod error, got %v", err)

internal/exec/template_utils_test.go

@@ -11,7 +11,7 @@ import (
 // TestCreateTempDirectory verifies that a temporary directory is created with the expected permissions.
 func TestCreateTempDirectory(t *testing.T) {
 	if runtime.GOOS == "windows" {
-		t.Skip("Skipping permission check on Windows")
+		t.Skipf("Skipping permission check on Windows: Unix permissions not applicable")
 	}
 
 	dir, err := createTempDirectory()

pkg/downloader/custom_git_detector_test.go

@@ -164,7 +164,7 @@ func TestDetect_UnsupportedHost(t *testing.T) {
 
 func TestRemoveSymlinks(t *testing.T) {
 	if runtime.GOOS == "windows" {
-		t.Skip("Skipping symlink tests on Windows.")
+		t.Skipf("Skipping symlink tests on Windows: symlinks require special privileges")
 	}
 	tempDir, err := os.MkdirTemp("", "symlinktest")
 	if err != nil {

pkg/downloader/get_git_test.go

@@ -520,7 +520,7 @@ func TestCheckGitVersion_CommandError(t *testing.T) {
 
 func TestCheckGitVersion_WindowsSuffix_Stripped(t *testing.T) {
 	if runtime.GOOS != "windows" {
-		t.Skip("windows-specific behavior")
+		t.Skipf("Skipping test: Windows-specific git behavior differs from Unix systems")
 	}
 	// On Windows, version string may contain ".windows." and should be stripped
 	writeFakeGit(t, "git version 2.20.1.windows.1", 0)

pkg/downloader/git_getter_test.go

@@ -113,7 +113,7 @@ func TestRemoveSymlinks_NonexistentRoot_PropagatesError(t *testing.T) {
 func TestRemoveSymlinks_WalkError_Propagates(t *testing.T) {
 	// Permission-denied dir (Unix only; Windows semantics differ)
 	if runtime.GOOS == "windows" {
-		t.Skip("permission-based Walk error test skipped on Windows")
+		t.Skipf("Skipping permission-based Walk error test on Windows: permissions work differently")
 	}
 	root := t.TempDir()
 	denyDir := filepath.Join(root, "deny")
@@ -182,7 +182,7 @@ func TestCustomGitGetter_Get_RemoveSymlinkError(t *testing.T) {
 	// This test is similar to the success case, but we'll use a read-only directory
 	// to force a permission error during symlink removal
 	if runtime.GOOS == "windows" {
-		t.Skip("Skipping read-only directory test on Windows")
+		t.Skipf("Skipping read-only directory test on Windows: read-only semantics differ")
 	}
 
 	// Create a read-only directory

pkg/downloader/gogetter_downloader_test.go

@@ -147,7 +147,7 @@ func TestDownloadDetectFormatAndParseFile(t *testing.T) {
 
 func TestGoGetterGet_File(t *testing.T) {
 	if runtime.GOOS == "windows" {
-		t.Skip("Skipping file copying test on Windows due to potential file system differences.")
+		t.Skipf("Skipping file copying test on Windows: file system differences may cause issues")
 	}
 	srcDir, err := os.MkdirTemp("", "src")
 	if err != nil {

pkg/store/google_secret_manager_store_test.go

@@ -497,7 +497,7 @@ func TestNewGSMStore(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			if tt.skipMessage != "" && os.Getenv("GOOGLE_APPLICATION_CREDENTIALS") == "" {
-				t.Skip(tt.skipMessage)
+				t.Skipf("%s", tt.skipMessage)
 			}
 			store, err := NewGSMStore(tt.options)
 			if tt.expectError {