refactor: replace t.Skip with t.Skipf throughout codebase#1449
Conversation
- 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>
|
Warning Rate limit exceeded@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRepository updates test skipping policies and behavior: adds a .gitignore rule, extends golangci-lint forbidigo to forbid plain t.Skip, inserts mandatory "Test Skipping Conventions" in CLAUDE.md, replaces many t.Skip calls with t.Skipf(...) including more descriptive messages, and centralizes CLI test skipping via a package-level skipReason in TestMain. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Go as "go test"
participant TM as "TestMain"
participant Env as "Env / Binary Checks"
participant M as "m.Run()"
participant T as "Individual Tests"
Go->>TM: start
TM->>Env: validate Atmos binary (existence/location/freshness)
alt problem found
Env-->>TM: set skipReason (non-empty)
TM-->>Go: log skip notice (no fatal)
else ok
Env-->>TM: skipReason empty
TM-->>Go: log Atmos binary info
end
TM->>M: invoke test suite
par tests
M->>T: run test
alt skipReason set
T-->>M: t.Skipf(skipReason)
else proceed
T-->>M: execute test body
end
end
M-->>TM: return exit code
TM-->>Go: os.Exit(code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/cli_test.go (1)
482-499: Don’t log full PATH; also avoid new os.Getenv usage in tests.
Including PATH in the skip reason is noisy and runs afoul of the “avoid os.Getenv” guidance. The message is clear without PATH.- skipReason = fmt.Sprintf("Atmos binary not found in PATH: %s. Run 'make build' to build the binary.", os.Getenv("PATH")) + skipReason = "Atmos binary not found in PATH. Run 'make build' to build the binary."
🧹 Nitpick comments (5)
.gitignore (1)
54-54: Scope the ignore to the repo root to avoid accidental matches.Anchoring prevents ignoring any nested file named
tests.testin subdirectories.- tests.test + /tests.testpkg/downloader/custom_git_detector_test.go (1)
166-168: Run the Windows symlink test when privileges are available.Instead of unconditionally skipping on Windows, probe symlink capability and skip only if creation fails. This improves coverage on privileged CI agents.
- if runtime.GOOS == "windows" { - t.Skipf("Skipping symlink tests on Windows: symlinks require special privileges") - } + if runtime.GOOS == "windows" { + // Probe symlink capability; skip only if creation is not permitted. + tmp, err := os.MkdirTemp("", "symlinkprobe") + if err != nil { + t.Fatalf("temp dir: %v", err) + } + defer os.RemoveAll(tmp) + target := filepath.Join(tmp, "a") + link := filepath.Join(tmp, "l") + _ = os.WriteFile(target, []byte("x"), 0o600) + if err := os.Symlink(target, link); err != nil { + t.Skipf("Skipping symlink tests on Windows: symlinks require special privileges (err=%v).", err) + } + _ = os.Remove(link) + }internal/exec/copy_glob_test.go (1)
111-111: Consistent, descriptive t.Skipf usage — nice; consider a tiny helper to de-dupe.Optional: introduce a helper to reduce repetition across tests without losing specificity.
// testhelpers/skip.go package testhelpers import ( "runtime" "testing" ) func SkipOnWindows(t *testing.T, reason string) { t.Helper() if runtime.GOOS == "windows" { t.Skipf("Skipping on Windows: %s", reason) } }Then replace the in-line guards with
testhelpers.SkipOnWindows(t, "<reason>").Also applies to: 246-246, 263-263, 356-356, 749-749
CLAUDE.md (1)
173-174: Add a trailing period for consistency.Maintain punctuation consistency in mandatory lists.
- - Never use `log.Fatal()` for missing/stale binaries + - Never use `log.Fatal()` for missing/stale binaries.tests/cli_test.go (1)
44-44: Add trailing period to comment.
Keeps comments consistent with the repo convention.- skipReason string // Package-level variable to track why tests should be skipped + skipReason string // Package-level variable to track why tests should be skipped.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.gitignore(1 hunks).golangci.yml(1 hunks)CLAUDE.md(1 hunks)internal/exec/copy_glob_test.go(5 hunks)internal/exec/template_utils_test.go(1 hunks)pkg/downloader/custom_git_detector_test.go(1 hunks)pkg/downloader/get_git_test.go(1 hunks)pkg/downloader/git_getter_test.go(2 hunks)pkg/downloader/gogetter_downloader_test.go(1 hunks)pkg/store/google_secret_manager_store_test.go(1 hunks)tests/cli_terraform_test.go(1 hunks)tests/cli_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
.golangci.yml
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
Configure golangci-lint in .golangci.yml with the specified linters enabled
Files:
.golangci.yml
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaultsFormat Go code with
gofumpt -w .andgoimports -w .
**/*.go: All comments must end with periods (complete sentences).
Always wrap errors with a static error first using fmt.Errorf and %w to preserve the error chain.
Target >80% coverage on new/changed lines and include comprehensive unit tests for new features.
Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Use logging only for system/debug info; structured logging without string interpolation; follow logging hierarchy per docs/logging.md.
Most text UI must go to stderr; only data/results to stdout for piping compatibility.
Prefer SDKs over shelling out to binaries; use filepath.Join, os.PathSeparator/ListSeparator; gate OS-specific logic with runtime.GOOS when unavoidable.
For non-standard execution paths, capture telemetry via pkg/telemetry (CaptureCmd or CaptureCmdString) without collecting user data.
Files:
pkg/downloader/custom_git_detector_test.gotests/cli_terraform_test.gointernal/exec/template_utils_test.gopkg/store/google_secret_manager_store_test.gopkg/downloader/gogetter_downloader_test.gopkg/downloader/get_git_test.gopkg/downloader/git_getter_test.gotests/cli_test.gointernal/exec/copy_glob_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)
**/*_test.go: Every new feature must include comprehensive unit tests
Test both happy paths and error conditions
Use table-driven tests for testing multiple scenarios
Include integration tests for command flows
Test CLI end-to-end when possible
Use test fixtures for complex inputs
Consider using testify/mock for creating mock implementations
**/*_test.go: Use table-driven unit tests for pure functions.
Co-locate tests alongside implementation files (use *_test.go).
Files:
pkg/downloader/custom_git_detector_test.gotests/cli_terraform_test.gointernal/exec/template_utils_test.gopkg/store/google_secret_manager_store_test.gopkg/downloader/gogetter_downloader_test.gopkg/downloader/get_git_test.gopkg/downloader/git_getter_test.gotests/cli_test.gointernal/exec/copy_glob_test.go
pkg/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests for packages under pkg with *_test.go naming.
Files:
pkg/downloader/custom_git_detector_test.gopkg/store/google_secret_manager_store_test.gopkg/downloader/gogetter_downloader_test.gopkg/downloader/get_git_test.gopkg/downloader/git_getter_test.go
tests/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Place integration tests under tests with fixtures in tests/test-cases/.
Files:
tests/cli_terraform_test.gotests/cli_test.go
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Provide integration tests for CLI commands using fixtures under tests/test-cases/.
Files:
tests/cli_terraform_test.gotests/cli_test.go
pkg/store/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
One implementation per file for interfaces in pkg/store (e.g., aws_ssm_store.go, azure_keyvault_store.go).
Files:
pkg/store/google_secret_manager_store_test.go
pkg/store/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement store providers behind interfaces and generate mocks (use //go:generate mockgen).
Files:
pkg/store/google_secret_manager_store_test.go
🧠 Learnings (32)
📓 Common learnings
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: CI should run unit tests, integration tests, golangci-lint, and coverage reporting
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: All code must pass golangci-lint checks
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: All code must pass golangci-lint checks
Applied to files:
.golangci.ymlCLAUDE.md
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow Go's documentation conventions
Applied to files:
.golangci.ymlCLAUDE.md
📚 Learning: 2025-04-23T15:02:50.246Z
Learnt from: osterman
PR: cloudposse/atmos#1202
File: pkg/utils/yaml_func_exec.go:104-104
Timestamp: 2025-04-23T15:02:50.246Z
Learning: In the Atmos codebase, direct calls to `os.Getenv` should be avoided. Instead, use `viper.BindEnv` for environment variable access. This provides a consistent approach to configuration management across the codebase.
Applied to files:
.golangci.ymltests/cli_test.go
📚 Learning: 2025-03-25T12:23:42.649Z
Learnt from: Listener430
PR: cloudposse/atmos#1149
File: internal/exec/go_getter_utils.go:104-104
Timestamp: 2025-03-25T12:23:42.649Z
Learning: Listener430 plans to add a test for verifying that token injection is skipped for unsupported hosts in a future review or refactoring iteration. This relates to the CustomGitDetector.Detect method in internal/exec/go_getter_utils.go.
Applied to files:
pkg/downloader/custom_git_detector_test.go
📚 Learning: 2025-03-21T17:35:56.827Z
Learnt from: Listener430
PR: cloudposse/atmos#984
File: internal/exec/go_getter_utils.go:234-248
Timestamp: 2025-03-21T17:35:56.827Z
Learning: When removing symlinks in Go, using os.Remove(path) is sufficient as it removes the symlink reference itself without affecting the target. filepath.Walk doesn't follow symlinks by default, so there's no need for special handling of nested symlink structures.
Applied to files:
pkg/downloader/custom_git_detector_test.gopkg/downloader/git_getter_test.go
📚 Learning: 2025-08-15T14:43:41.030Z
Learnt from: aknysh
PR: cloudposse/atmos#1352
File: pkg/store/artifactory_store_test.go:108-113
Timestamp: 2025-08-15T14:43:41.030Z
Learning: In test files for the atmos project, it's acceptable to ignore errors from os.Setenv/Unsetenv operations during test environment setup and teardown, as these are controlled test scenarios.
Applied to files:
tests/cli_terraform_test.gotests/cli_test.go
📚 Learning: 2024-11-12T03:16:02.910Z
Learnt from: aknysh
PR: cloudposse/atmos#775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for `componentFunc` in `internal/exec/template_funcs_component.go`, the function `cleanTerraformWorkspace` does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for `cleanTerraformWorkspace` is not needed.
Applied to files:
tests/cli_terraform_test.go
📚 Learning: 2024-10-27T04:41:49.199Z
Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.
Applied to files:
tests/cli_terraform_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: The atmos codebase has a custom extension to *testing.T that provides a Chdir method, allowing test functions to call t.Chdir() to change working directories during tests. This is used consistently across test files in the codebase.
Applied to files:
internal/exec/template_utils_test.gopkg/downloader/gogetter_downloader_test.gotests/cli_test.gointernal/exec/copy_glob_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions
Applied to files:
pkg/downloader/gogetter_downloader_test.gotests/cli_test.goCLAUDE.mdinternal/exec/copy_glob_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests. This is implemented through custom testing framework extensions and is used consistently throughout the test suite.
Applied to files:
pkg/downloader/gogetter_downloader_test.gotests/cli_test.gointernal/exec/copy_glob_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method call on *testing.T objects and works correctly for changing directories in tests.
Applied to files:
pkg/downloader/gogetter_downloader_test.gotests/cli_test.gointernal/exec/copy_glob_test.go
📚 Learning: 2025-05-23T19:51:47.091Z
Learnt from: samtholiya
PR: cloudposse/atmos#1255
File: cmd/describe_affected_test.go:15-15
Timestamp: 2025-05-23T19:51:47.091Z
Learning: In the atmos codebase, t.Chdir() is a valid method that can be called on *testing.T objects. This functionality is implemented through custom testing framework extensions and is used consistently throughout the test suite for changing working directories during tests.
Applied to files:
pkg/downloader/gogetter_downloader_test.gotests/cli_test.gointernal/exec/copy_glob_test.go
📚 Learning: 2024-10-23T21:36:40.262Z
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
Applied to files:
tests/cli_test.go
📚 Learning: 2025-08-29T20:57:35.423Z
Learnt from: osterman
PR: cloudposse/atmos#1433
File: cmd/theme_list.go:33-36
Timestamp: 2025-08-29T20:57:35.423Z
Learning: In the Atmos codebase, avoid using viper.SetEnvPrefix("ATMOS") with viper.AutomaticEnv() because canonical environment variable names are not exclusive to Atmos and could cause conflicts. Instead, use selective environment variable binding through the setEnv function in pkg/config/load.go with bindEnv(v, "config.key", "ENV_VAR_NAME") for specific environment variables.
Applied to files:
tests/cli_test.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Always bind environment variables with viper.BindEnv and ensure an ATMOS_ alternative exists for every env var.
Applied to files:
tests/cli_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use Viper for managing configuration, environment variables, and flags
Applied to files:
tests/cli_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use Viper for configuration management
Applied to files:
tests/cli_test.go
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Use snake_case for environment variables
Applied to files:
tests/cli_test.go
📚 Learning: 2025-04-10T20:48:22.687Z
Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: pkg/config/load.go:0-0
Timestamp: 2025-04-10T20:48:22.687Z
Learning: In the `bindEnv` function in `pkg/config/load.go`, panic is used deliberately instead of returning errors because errors from `BindEnv` would only occur due to developer mistakes. Using panic helps with early detection of these developer errors during initialization.
Applied to files:
tests/cli_test.go
📚 Learning: 2025-02-20T18:33:16.567Z
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:47-68
Timestamp: 2025-02-20T18:33:16.567Z
Learning: Go doesn't have a built-in secure string type. For sensitive data like tokens, using environment variables (which are protected by the OS) is a common and secure practice.
Applied to files:
tests/cli_test.go
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to cmd/**/*_test.go : Add command tests under cmd with *_test.go naming.
Applied to files:
CLAUDE.md
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Include integration tests for command flows
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Target >80% coverage on new/changed lines and include comprehensive unit tests for new features.
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-02T19:59:52.093Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to cmd/**/*.go : For CLI commands, use //go:embed to load *_usage.md examples and utils.PrintfMarkdown() to render them.
Applied to files:
CLAUDE.md
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests
Applied to files:
CLAUDE.md
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document complex logic with inline comments
Applied to files:
CLAUDE.md
📚 Learning: 2025-07-01T02:22:25.901Z
Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to cmd/*.go : Include troubleshooting hints when appropriate
Applied to files:
CLAUDE.md
📚 Learning: 2024-12-25T20:28:47.526Z
Learnt from: osterman
PR: cloudposse/atmos#887
File: internal/exec/stack_processor_utils.go:380-380
Timestamp: 2024-12-25T20:28:47.526Z
Learning: Windows path handling often requires `filepath.Join` to ensure correct separators and comparisons. Insufficient tests can break cross-platform compatibility, so migrating from `path.Join` to `filepath.Join` needs thorough testing on Windows before merging.
Applied to files:
internal/exec/copy_glob_test.go
📚 Learning: 2025-04-10T21:33:06.447Z
Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: pkg/filematch/filematch.go:32-32
Timestamp: 2025-04-10T21:33:06.447Z
Learning: The replacement of "*/*" with an empty string in recursive glob patterns (containing "**") is intentional behavior in the filematch package, as confirmed by the developer. This is necessary for proper handling of recursive directory matching when using the gobwas/glob library.
Applied to files:
internal/exec/copy_glob_test.go
📚 Learning: 2025-09-02T14:44:18.360Z
Learnt from: CR
PR: cloudposse/atmos#0
File: AGENTS.md:0-0
Timestamp: 2025-09-02T14:44:18.360Z
Learning: Applies to tests/{test-cases,testdata}/** : Do not modify files under `tests/test-cases/` or `tests/testdata/` unless explicitly instructed
Applied to files:
.gitignore
🧬 Code graph analysis (1)
tests/cli_test.go (1)
errors/error_funcs.go (1)
Exit(75-77)
🪛 LanguageTool
CLAUDE.md
[grammar] ~169-~169: There might be a mistake here.
Context: ...tests that depend on rebuilt binaries**: - Package-level skipReason variable is s...
(QB_NEW_EN)
[grammar] ~173-~173: Please add a punctuation mark at the end of paragraph.
Context: ...ver use log.Fatal() for missing/stale binaries ### CLI Command Structure & Examples A...
(PUNCTUATION_PARAGRAPH_END)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Build (ubuntu-latest, linux)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (12)
pkg/store/google_secret_manager_store_test.go (1)
500-501: LGTM: descriptive Skipf is consistent with the new lint rule.
t.Skipf("%s", tt.skipMessage)preserves the message and satisfies forbidigo.internal/exec/template_utils_test.go (1)
13-15: LGTM: clearer Windows skip reason via Skipf.Good descriptive message; aligned with repo policy.
.golangci.yml (1)
40-41: Tighten forbid patterns for Skip and SkipNow
Verified not.Skip(ort.SkipNow(calls remain in Go code; occurrences in CLAUDE.md are documentation examples and not linted.pkg/downloader/get_git_test.go (1)
523-523: Descriptive t.Skipf on non-Windows — LGTM.Clear reason provided; aligns with the new lint rule.
pkg/downloader/gogetter_downloader_test.go (1)
150-150: Windows guard now uses t.Skipf with context — LGTM.Message is explicit and actionable.
pkg/downloader/git_getter_test.go (2)
116-116: Use of t.Skipf for Windows permission semantics — LGTM.Reason is specific to Walk/permissions behavior.
185-185: Use of t.Skipf for Windows read-only semantics — LGTM.Clear rationale; consistent with repo policy.
CLAUDE.md (2)
157-174: New “Test Skipping Conventions” section — solid and specific.Captures the policy and good examples.
157-174: Conventions and tests checks passed
All scans found not.Skip()calls, TestMain intests/cli_test.goinvokesm.Run(), and there’s only one “Test Skipping Conventions (MANDATORY)” section in CLAUDE.md.tests/cli_test.go (2)
501-503: LGTM: conditional logging behind skip guard.
Only logs binary details when tests will actually run.
747-749: LGTM: early, descriptive skip with t.Skipf.
Matches the new convention and keeps failures from masking real test runs.tests/cli_terraform_test.go (1)
16-18: LGTM: centralized skip guard adopted.
Uses t.Skipf with the shared reason; consistent with TestMain flow.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CLAUDE.md (1)
157-174: Tighten punctuation and code formatting for consistency.Add periods and code formatting for identifiers; aligns with your style rules and fixes the grammar nits.
-### 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** +### 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:
- Package-level
skipReasonvariable is set inTestMain
- Individual test functions check and skip with
t.Skipf()if set
+- For CLI tests that depend on rebuilt binaries:
- Package-level
skipReasonvariable is set inTestMain.
- Individual test functions check and skip with
t.Skipf()if set.- TestMain MUST always call
m.Run()to allow proper test skipping
- Never use
log.Fatal()for missing/stale binaries
- Never use
log.Fatal()for missing/stale binaries.</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 820b52779706685bc8f0f605a70b80aa7a3270d9 and 30c674020c7c3c5bfb16095142e3b8fdf5a21867. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `CLAUDE.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧠 Learnings (11)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: CI should run unit tests, integration tests, golangci-lint, and coverage reportingLearnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: All code must pass golangci-lint checks</details> <details> <summary>📚 Learning: 2025-09-02T19:59:52.093Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to cmd/**/*_test.go : Add command tests under cmd with *_test.go naming.**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-07-01T02:22:25.901Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Include integration tests for command flows**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-09-02T19:59:52.093Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to **/*.go : Target >80% coverage on new/changed lines and include comprehensive unit tests for new features.**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-09-02T19:59:52.093Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-02T19:59:52.093Z
Learning: Applies to cmd/**/*.go : For CLI commands, use //go:embed to load *_usage.md examples and utils.PrintfMarkdown() to render them.**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-07-01T02:22:25.901Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Every new feature must include comprehensive unit tests**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-07-01T02:22:25.901Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Follow Go's documentation conventions**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-07-01T02:22:25.901Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: All code must pass golangci-lint checks**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-07-01T02:22:25.901Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*.go : Document complex logic with inline comments**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-07-01T02:22:25.901Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to cmd/*.go : Include troubleshooting hints when appropriate**Applied to files:** - `CLAUDE.md` </details> <details> <summary>📚 Learning: 2025-07-01T02:22:25.901Z</summary>Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions**Applied to files:** - `CLAUDE.md` </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>CLAUDE.md</summary> [grammar] ~169-~169: There might be a mistake here. Context: ...tests that depend on rebuilt binaries**: - Package-level `skipReason` variable is s... (QB_NEW_EN) --- [grammar] ~173-~173: Please add a punctuation mark at the end of paragraph. Context: ...ver use `log.Fatal()` for missing/stale binaries ### CLI Command Structure & Examples A... (PUNCTUATION_PARAGRAPH_END) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)</summary> * GitHub Check: Build (ubuntu-latest, linux) * GitHub Check: Build (windows-latest, windows) * GitHub Check: Lint (golangci) * GitHub Check: Analyze (go) * GitHub Check: Summary </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
==========================================
+ Coverage 55.94% 55.99% +0.04%
==========================================
Files 274 274
Lines 28936 28936
==========================================
+ Hits 16189 16202 +13
+ Misses 10955 10941 -14
- Partials 1792 1793 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
These changes were released in v1.190.0-test.0. |
- Resolve conflict in .gitignore by combining both test artifact entries - Includes latest main branch changes: - Update go-getter to latest version (#1441) - Make inline atmos config override config from imports (#1447) - Replace t.Skip with t.Skipf throughout codebase (#1449) - Maintains all gotcha-related changes from feature branch
* 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>
what
t.Skip()calls witht.Skipf()providing descriptive reasonsskipReasonvariable instead of fatal exitst.Skipfusagewhy
t.Skip()without a reason makes debugging difficult when tests are skippedt.Skipf()with descriptive messages helps developers understand why tests didn't runt.Skip()without reasonsChanges made
Test files updated (12 instances)
CLI test infrastructure
skipReasonvariable set in TestMainlogger.Fatal()calls for binary issues with skipReason settingm.Run()to allow proper test reportingLinting configuration
t.Skip(usageDocumentation
Testing
t.Skip()calls in codebaseSummary by CodeRabbit
Documentation
Tests
Chores