-
-
Notifications
You must be signed in to change notification settings - Fork 151
test: increase test coverage in pkg/flags, pkg/filesystem, pkg/http, and pkg/function #2173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 34 commits
79f7203
c2ec8e8
c95ed53
138797d
c305c40
cbc82f1
919e954
9d7bcf3
af1b9a6
cff8ea2
84f0c95
a059e07
de7c7cc
8f91e21
1659bf2
04f8d19
f2bf8e1
0f23667
525f416
8b9554c
04a4a0a
fe62aae
4826a8e
3d15b91
7c2e8c6
f111de4
c495d14
aa5d84a
b50e896
1053cdb
b2e83cb
8ca5417
69b1d82
bc5657f
7704902
b80a3a1
35885a2
4187e60
fa508cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| # Changelog | ||
|
|
||
| All notable changes to this project will be documented in this file. | ||
| See [Conventional Commits](https://www.conventionalcommits.org/) for commit message format. | ||
|
|
||
| --- | ||
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Changed | ||
|
|
||
| - **`pkg/filesystem.GetGlobMatches`**: always returns a non-nil `[]string{}` (never `nil`). | ||
| Callers must use `len(result) == 0` to detect no matches instead of `result == nil`. | ||
| The cache is now bounded and configurable via three environment variables: | ||
| - `ATMOS_FS_GLOB_CACHE_MAX_ENTRIES` (default `1024`, minimum `16`) — maximum number of cached glob patterns. | ||
| - `ATMOS_FS_GLOB_CACHE_TTL` (default `5m`, minimum `1s`) — time-to-live for each cache entry. | ||
| Values below the respective minimums are clamped up rather than rejected. | ||
| - `ATMOS_FS_GLOB_CACHE_EMPTY` (default `1`) — set to `0` to skip caching patterns that match no files. | ||
| - **`pkg/http.normalizeHost`**: now strips default ports (`:443`, `:80`) in addition to | ||
| lower-casing and removing trailing dots, so that `api.github.com:443` is treated | ||
| identically to `api.github.com` for allowlist matching. | ||
|
|
||
| ### Added | ||
|
|
||
| - **`pkg/filesystem`**: expvar observability counters (`atmos_glob_cache_hits`, | ||
| `atmos_glob_cache_misses`, `atmos_glob_cache_evictions`, `atmos_glob_cache_len`) published | ||
| via `RegisterGlobCacheExpvars()` and accessible at `/debug/vars` when the HTTP debug | ||
| server is enabled. | ||
| - **`pkg/http`**: host-matcher three-level precedence documented and tested: | ||
| 1. `WithGitHubHostMatcher` — custom predicate always wins. | ||
| 2. `GITHUB_API_URL` — GHES hostname added to allowlist when set. | ||
| 3. Built-in allowlist — `api.github.com`, `raw.githubusercontent.com`, `uploads.github.com`. | ||
| Authorization is only injected over HTTPS and stripped on cross-host redirects | ||
| (301 / 302 / 303 / 307 / 308) to prevent token leakage. | ||
|
|
||
| ### Security | ||
|
|
||
| - Cross-host HTTP redirects (all five status codes: 301, 302, 303, 307, 308) no longer | ||
| forward the `Authorization` header to the redirect target, preventing accidental token | ||
| leakage via open redirects. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,4 +124,11 @@ link-check: | |
| @command -v lychee >/dev/null 2>&1 || { echo "Install lychee: brew install lychee"; exit 1; } | ||
| lychee --config lychee.toml --root-dir "$(CURDIR)" '**/*.md' | ||
|
|
||
| .PHONY: readme lint lintroller gomodcheck build version build-linux build-windows build-macos deps version-linux version-windows version-macos testacc testacc-cover testacc-coverage test-short test-short-cover generate-mocks link-check | ||
| # Run quick tests with race detector and shuffled order. | ||
| # This target is recommended for CI to catch data races and order-dependent failures. | ||
| # Usage: make test-race | ||
| test-race: deps | ||
| @echo "Running tests with -race -shuffle=on" | ||
| go test -race -shuffle=on $(TEST) $(TESTARGS) -timeout 10m | ||
|
|
||
| .PHONY: readme lint lintroller gomodcheck build version build-linux build-windows build-macos deps version-linux version-windows version-macos testacc testacc-cover testacc-coverage test-short test-short-cover test-race generate-mocks link-check | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add missing phony targets to clear checkmake Line 134 still omits 🔧 Proposed patch+all: build
+
+test: testacc
+
+clean:
+ rm -rf build coverage coverage.out coverage.html
+
-.PHONY: readme lint lintroller gomodcheck build version build-linux build-windows build-macos deps version-linux version-windows version-macos testacc testacc-cover testacc-coverage test-short test-short-cover test-race generate-mocks link-check
+.PHONY: all clean test readme lint lintroller gomodcheck build version build-linux build-windows build-macos deps version-linux version-windows version-macos testacc testacc-cover testacc-coverage test-short test-short-cover test-race generate-mocks link-check🧰 Tools🪛 checkmake (0.2.2)[warning] 134-134: Missing required phony target "all" (minphony) [warning] 134-134: Missing required phony target "clean" (minphony) [warning] 134-134: Missing required phony target "test" (minphony) 🤖 Prompt for AI Agents |
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1102,7 +1102,10 @@ func processYAMLConfigFileWithContextInternal( | |||||||||||||
| err, | ||||||||||||||
| ) | ||||||||||||||
| return nil, nil, nil, nil, nil, nil, nil, nil, errors.New(errorMessage) | ||||||||||||||
| } else if importMatches == nil { | ||||||||||||||
| } else { | ||||||||||||||
| // err == nil but importMatches is an empty slice (not nil): pkg/filesystem.GetGlobMatches | ||||||||||||||
| // guarantees a non-nil result, so the old "else if importMatches == nil" check was dead | ||||||||||||||
| // code. We reach this branch when no files matched and the call returned ([]string{}, nil). | ||||||||||||||
|
Comment on lines
+1106
to
+1108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix incorrect package contract reference in the inline comment. Line 1106 references 📝 Proposed comment-only fix- // err == nil but importMatches is an empty slice (not nil): pkg/filesystem.GetGlobMatches
- // guarantees a non-nil result, so the old "else if importMatches == nil" check was dead
- // code. We reach this branch when no files matched and the call returned ([]string{}, nil).
+ // err == nil but importMatches is an empty slice (not nil): this branch guards
+ // against empty results explicitly, independent of the specific GetGlobMatches
+ // package contract.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| errorMessage := fmt.Sprintf("no matches found for the import '%s' in the file '%s'", | ||||||||||||||
| imp, | ||||||||||||||
| relativeFilePath, | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // Package filesystem provides file-system utilities for the Atmos CLI, including | ||
| // atomic file writes (POSIX rename and a Windows-compatible remove-before-rename | ||
| // variant) and glob-pattern matching with a bounded, time-limited LRU cache. | ||
| // | ||
| // # GetGlobMatches contract | ||
| // | ||
| // [GetGlobMatches] always returns a non-nil []string. An empty result set is | ||
| // returned as []string{}, never nil. Callers must check len(result) == 0, not | ||
| // result == nil. | ||
| // | ||
| // # Cache configuration | ||
| // | ||
| // The glob LRU cache is configurable at startup via environment variables: | ||
| // | ||
| // - ATMOS_FS_GLOB_CACHE_MAX_ENTRIES – maximum number of cached patterns | ||
| // (default: 1024, minimum: 16; values below 16 are clamped up). | ||
| // - ATMOS_FS_GLOB_CACHE_TTL – TTL per entry as a Go duration string, e.g. | ||
| // "10m" (default: 5m, minimum: 1s; values below 1s are clamped up). | ||
| // - ATMOS_FS_GLOB_CACHE_EMPTY – set to "0" or "false" to disable caching | ||
| // of empty (no-match) results (default: "1" = enabled). | ||
| // | ||
| // # Observability | ||
| // | ||
| // Three atomic int64 counters track cache activity: | ||
| // - hits, misses, evictions (accessible via [GlobCacheHits], [GlobCacheMisses], | ||
| // [GlobCacheEvictions] in tests). | ||
| // | ||
| // Call [RegisterGlobCacheExpvars] once at startup to expose these counters via | ||
| // the expvar /debug/vars HTTP endpoint: | ||
| // | ||
| // filesystem.RegisterGlobCacheExpvars() | ||
| package filesystem |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| package filesystem | ||
|
|
||
| import ( | ||
| "path/filepath" | ||
| "sync" | ||
| "sync/atomic" | ||
| "time" | ||
| ) | ||
|
|
||
| // ResetGlobMatchesCache clears the glob matches LRU cache and resets all counters. | ||
| // This is exported only for testing to avoid data races from direct struct assignment. | ||
| func ResetGlobMatchesCache() { | ||
| globMatchesLRUMu.Lock() | ||
| globMatchesLRU.Purge() | ||
| globMatchesLRUMu.Unlock() | ||
| atomic.StoreInt64(&globMatchesEvictions, 0) | ||
| atomic.StoreInt64(&globMatchesHits, 0) | ||
| atomic.StoreInt64(&globMatchesMisses, 0) | ||
| } | ||
|
|
||
| // ResetPathMatchCache clears the path match cache. | ||
| // This is exported only for testing to ensure consistent state between tests. | ||
| func ResetPathMatchCache() { | ||
| pathMatchCacheMu.Lock() | ||
| pathMatchCache = make(map[pathMatchKey]bool) | ||
| pathMatchCacheMu.Unlock() | ||
| } | ||
|
|
||
| // SetGlobCacheEntryExpired forcibly marks a cache entry as expired for testing TTL eviction. | ||
| // It re-adds the entry with an expiry in the past, simulating TTL expiry. | ||
| func SetGlobCacheEntryExpired(pattern string) { | ||
| normalizedPattern := filepath.ToSlash(pattern) | ||
| globMatchesLRUMu.Lock() | ||
| if entry, ok := globMatchesLRU.Get(normalizedPattern); ok { | ||
| entry.expiry = time.Time{} // zero time is in the past | ||
| globMatchesLRU.Add(normalizedPattern, entry) | ||
| } | ||
| globMatchesLRUMu.Unlock() | ||
| } | ||
|
|
||
| // GlobCacheLen returns the number of entries currently in the glob LRU cache. | ||
| func GlobCacheLen() int { | ||
| globMatchesLRUMu.RLock() | ||
| defer globMatchesLRUMu.RUnlock() | ||
| return globMatchesLRU.Len() | ||
| } | ||
|
|
||
| // GlobCacheEvictions returns the total number of LRU evictions since the last cache reset. | ||
| // This counter is incremented atomically by the LRU eviction callback. | ||
| func GlobCacheEvictions() int64 { | ||
| return atomic.LoadInt64(&globMatchesEvictions) | ||
| } | ||
|
|
||
| // GlobCacheHits returns the total number of cache hits since the last cache reset. | ||
| func GlobCacheHits() int64 { | ||
| return atomic.LoadInt64(&globMatchesHits) | ||
| } | ||
|
|
||
| // GlobCacheMisses returns the total number of cache misses since the last cache reset. | ||
| func GlobCacheMisses() int64 { | ||
| return atomic.LoadInt64(&globMatchesMisses) | ||
| } | ||
|
|
||
| // ApplyGlobCacheConfigForTest re-reads ATMOS_FS_GLOB_CACHE_* env vars and reinitializes | ||
| // the glob LRU cache. Tests should call this after setting env vars via t.Setenv. | ||
| // It also resets all counters so tests start from a clean baseline. | ||
| func ApplyGlobCacheConfigForTest() { | ||
| applyGlobCacheConfig() | ||
| atomic.StoreInt64(&globMatchesEvictions, 0) | ||
| atomic.StoreInt64(&globMatchesHits, 0) | ||
| atomic.StoreInt64(&globMatchesMisses, 0) | ||
| } | ||
|
|
||
| // GlobCacheEmptyEnabled returns the current empty-result caching setting. | ||
| func GlobCacheEmptyEnabled() bool { | ||
| globMatchesLRUMu.RLock() | ||
| defer globMatchesLRUMu.RUnlock() | ||
| return globCacheEmptyEnabled | ||
| } | ||
|
|
||
| // ResetGlobExpvarOnce resets the sync.Once guard so RegisterGlobCacheExpvars | ||
| // can be called again in the same test binary. Only for use in tests that | ||
| // need to verify expvar registration after a cache reset. | ||
| func ResetGlobExpvarOnce() { | ||
| globExpvarOnce = sync.Once{} | ||
| } | ||
|
Comment on lines
+81
to
+86
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Verification inconclusiveScript executed: #!/bin/bash
# Check if any other test or init code calls RegisterGlobCacheExpvars
rg -n "RegisterGlobCacheExpvars" --type go -g '!*_test.go'Repository: cloudposse/atmos Repository: cloudposse/atmos stdout: Script executed: cat -n pkg/filesystem/glob_metrics.goRepository: cloudposse/atmos Repository: cloudposse/atmos stdout: Script executed: rg -n "RegisterGlobCacheExpvars" --type go pkg/filesystem -A 5 -B 2Repository: cloudposse/atmos Repository: cloudposse/atmos stdout: Script executed: sed -n '500,550p' pkg/filesystem/glob_atomic_test.goRepository: cloudposse/atmos Repository: cloudposse/atmos stdout: Script executed: rg -n "ResetGlobExpvarOnce" --type goRepository: cloudposse/atmos Repository: cloudposse/atmos stdout: Script executed: rg -n "RegisterGlobCacheExpvars" pkg/filesystem -B 3 -A 1 | grep -E "(func Test|RegisterGlobCacheExpvars)"Repository: cloudposse/atmos Repository: cloudposse/atmos stdout: Resetting
Either guard the re-registration (e.g., check if already published) or accept that 🤖 Prompt for AI Agents |
||
|
|
||
| // GlobCacheTTL returns the currently active cache TTL for test introspection. | ||
| func GlobCacheTTL() time.Duration { | ||
| globMatchesLRUMu.RLock() | ||
| defer globMatchesLRUMu.RUnlock() | ||
| return globCacheTTL | ||
| } | ||
|
|
||
| // GlobCacheMaxEntries returns the currently configured LRU capacity for test introspection. | ||
| func GlobCacheMaxEntries() int { | ||
| globMatchesLRUMu.RLock() | ||
| defer globMatchesLRUMu.RUnlock() | ||
| return globCacheMaxEntries | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify how non-positive cache settings behave.
applyGlobCacheConfigonly clamps positive values below the floor.0and negative values currently fall back to defaults, so "values below the respective minimums are clamped up" reads broader than the shipped behavior.🤖 Prompt for AI Agents