Add MaskedString and other utilities - #91
Conversation
WalkthroughDependency update promotes yaml.v3 to a direct requirement. Introduces MaskValue and a new MaskedString type with marshaling semantics; refactors MaskArguments. Adds sys utilities: generic Ptr, Result[T], and enhanced IsLocalhost parsing/logic with tests. Minor string pointer refactor. Expands unit tests and logging test helper. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant MaskedString
participant JSON as encoding/json
participant YAML as gopkg.in/yaml.v3
Note over App,MaskedString: Construct and use MaskedString
App->>MaskedString: NewMaskedString("secret")
App->>MaskedString: String()
MaskedString-->>App: masked value
App->>JSON: Marshal(MaskedString)
JSON->>MaskedString: MarshalJSON()
MaskedString-->>JSON: unmasked JSON string
JSON-->>App: bytes
App->>YAML: Marshal(MaskedString)
YAML->>MaskedString: MarshalYAML()
MaskedString-->>YAML: unmasked YAML scalar
YAML-->>App: bytes
sequenceDiagram
autonumber
participant Caller
participant Net as sys.IsLocalhost
participant URL as net/url
participant NetPkg as net
Caller->>Net: IsLocalhost(input)
alt parseable URL
Net->>URL: Parse(input)
URL-->>Net: URL
Net->>URL: Hostname()
URL-->>Net: host
else host[:port] or literal
Net->>NetPkg: SplitHostPort(input)?
NetPkg-->>Net: host or error
Note over Net: if IPv6 [::1] trim brackets
end
Net->>NetPkg: ParseIP(host)
alt localhost/loopback/unspecified
Net-->>Caller: true
else
Net-->>Caller: false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
string/mask.go (3)
74-90: Flatten branching in MaskValue for readability.Same behavior, less nesting.
Apply:
func MaskValue(arg string) string { - if isURL.MatchString(arg) { - u, err := MaskURL(arg) - if err == nil { - return u - } else { - return Mask(arg) - } - } else if isEmail.MatchString(arg) { - return MaskEmail(arg) - } else if isJWT.MatchString(arg) { - return Mask(arg) - } else { - return arg - } + if isURL.MatchString(arg) { + if u, err := MaskURL(arg); err == nil { + return u + } + return Mask(arg) + } + if isEmail.MatchString(arg) { + return MaskEmail(arg) + } + if isJWT.MatchString(arg) { + return Mask(arg) + } + return arg }Optional: consider broadening isURL to RFC 3986 scheme regex
^[A-Za-z][A-Za-z0-9+.-]*://to catch schemes with+,.,-.
101-140: Document semantics and ensure %#v prints masked.Add a clearer doc comment and GoStringer so %#v is also redacted.
Apply:
-// MaskedString is a custom string type that masks its value when formatted or text-marshaled. +// MaskedString holds a sensitive string. +// Behavior: +// - String()/fmt %s,%v: masked +// - MarshalText: masked +// - MarshalJSON/YAML: unmasked +// - Text()/Bytes(): unmasked helpers (avoid in logs) type MaskedString string @@ func (ms MaskedString) String() string { if len(ms) == 0 { return "" } return Mask(string(ms)) } + +// GoString implements fmt.GoStringer so %#v also prints masked. +func (ms MaskedString) GoString() string { + return ms.String() +}
127-135: Unmasked JSON/YAML can leak secrets—confirm intended usage.If structs containing MaskedString are logged via JSON/YAML (structured logs, telemetry), this will emit the raw value. Verify call sites and consider logger integration that formats MaskedString via String()/Text() consciously (e.g., custom slog handler/zap encoder).
string/mask_test.go (4)
31-31: Rename test to canonical initialism.Use URL, not Url, for consistency with Go initialisms.
-func TestMaskUrl(t *testing.T) { +func TestMaskURL(t *testing.T) {
154-161: Avoid shadowing json package name.Rename local var to improve clarity.
- json, err := ms.MarshalJSON() + b, err := ms.MarshalJSON() assert.NoError(t, err) - assert.Equal(t, tt.want, string(json)) + assert.Equal(t, tt.want, string(b))
175-181: Avoid shadowing yaml import.Minor readability win.
- yaml, err := ms.MarshalYAML() + val, err := ms.MarshalYAML() assert.NoError(t, err) - assert.Equal(t, tt.want, yaml) + assert.Equal(t, tt.want, val)
233-237: Don’t ignore marshal error in comparison test.Check error before asserting equality.
- expectedJSON, _ := json.Marshal(input) - assert.Equal(t, expectedJSON, jsonBytes) + expectedJSON, err := json.Marshal(input) + assert.NoError(t, err) + assert.Equal(t, expectedJSON, jsonBytes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
go.mod(1 hunks)string/mask.go(2 hunks)string/mask_test.go(2 hunks)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
go.mod (1)
30-30: Direct yaml.v3 dependency—LGTM.Matches new YAML marshal/unmarshal usage in tests.
string/mask.go (2)
4-4: json import—LGTM.Required for MaskedString.MarshalJSON.
92-99: Delegating MaskArguments to MaskValue—LGTM.Removes duplication and keeps logic centralized.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
string/string.go (1)
5-8: Ptr returns address of a copy; clarify doc to avoid misusePtr[T] returns a pointer to a copy of v (not the caller’s variable). Tests assuming pointer identity with the caller’s variable will fail; update the doc to make this explicit.
-// Ptr returns a pointer to the given value. +// Ptr returns a pointer to a copy of v. The returned pointer escapes and is safe to use +// beyond the call. Note: it is not the address of the caller's variable. func Ptr[T any](v T) *T { return &v }string/mask.go (4)
74-90: Tighten URL/JWT detection and simplify control flowSuggest: (a) broaden URL scheme regex to RFC-compliant set; (b) early returns to reduce nesting.
func MaskValue(arg string) string { - if isURL.MatchString(arg) { - u, err := MaskURL(arg) - if err == nil { - return u - } else { - return Mask(arg) - } - } else if isEmail.MatchString(arg) { - return MaskEmail(arg) - } else if isJWT.MatchString(arg) { - return Mask(arg) - } else { - return arg - } + if isURL.MatchString(arg) { + if u, err := MaskURL(arg); err == nil { + return u + } + return Mask(arg) + } + if isEmail.MatchString(arg) { + return MaskEmail(arg) + } + if isJWT.MatchString(arg) { + return Mask(arg) + } + return arg }Additionally, consider updating the regex declarations (outside this hunk):
// More permissive per RFC 3986 scheme var isURL = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9+.-]*://`) // Base64url without padding is typical; allow optional '=' padding if desired var isJWT = regexp.MustCompile(`^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+=*$`)
101-104: Document the security semantics prominently
Call out that fmt/text are masked but JSON/YAML marshal unmasked, so consumers don’t assume “always masked.”-// MaskedString is a custom string type that masks its value when formatted or text-marshaled. +// MaskedString masks in fmt (%s/%v/%#v) and TextMarshaler outputs, but intentionally marshals +// the raw (unmasked) value to JSON and YAML. Do not log JSON/YAML output of this type. type MaskedString string
109-112: Minor: avoid extra call in Bytes()
Slightly simpler conversion.func (ms MaskedString) Bytes() []byte { - return []byte(ms.Text()) + return []byte(string(ms)) }
127-135: Unmasked JSON/YAML is intentional — validate usage paths
If these structs are ever JSON/YAML-logged, secrets will leak. Ensure logging paths use fmt/String() or MarshalText for masked output, or wrap log sinks.Do you want a guardrail utility (e.g., SafeJSON that replaces MaskedString with masked text for logs) and a repository-wide grep to confirm no logging of JSON/YAML using MaskedString-containing types?
sys/result.go (1)
8-13: Consider encapsulating fields to enforce invariant (optional).Exported Ok/Err allow external code to set both simultaneously. If you want stricter invariants, make fields unexported and expose accessors.
sys/result_test.go (2)
40-63: IsErr happy-path coverage is good. Add a case for nil target.Add a case like Okint.IsErr(nil) to ensure it returns false after the IsErr hardening.
Apply this diff to add coverage:
func TestResult_IsErr(t *testing.T) { @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert.Equal(t, tt.expected, tt.result.IsErr()) }) } + + t.Run("nil target ignored", func(t *testing.T) { + assert.False(t, Ok[int](1).IsErr(nil)) + assert.False(t, Err[int](errors.New("boom")).IsErr(nil)) + }) }
94-108: Add TestErrMatch_OkResult to cover Ok case of IsErrMatches
Ensure that calling IsErrMatches on an Ok result never panics and always returns false, including when no checks are passed.func TestErrMatchMultiple(t *testing.T) { @@ assert.Equal(t, err, result.Err) } +func TestErrMatch_OkResult(t *testing.T) { + ok := Ok("value") + assert.False(t, ok.IsErrMatches("value")) + assert.False(t, ok.IsErrMatches()) // no checks => false when no error +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
string/mask.go(2 hunks)string/mask_test.go(2 hunks)string/string.go(1 hunks)string/string_test.go(1 hunks)sys/net.go(1 hunks)sys/net_test.go(1 hunks)sys/result.go(1 hunks)sys/result_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- string/mask_test.go
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
sys/net.go (1)
24-24: Harden localhost detection
Parsing with url.Parse/SplitHostPort plus net.IP.IsLoopback now correctly covers 127/8 and ::1. Note net.IP.IsUnspecified returns true for both “0.0.0.0” and “::” (golangdoc.github.io); the checkip.To4() != nillimits unspecified support to IPv4. Remove that guard to treat “::” as local as well.string/mask.go (3)
4-4: Import json for custom marshaling — LGTM
Needed for MaskedString.MarshalJSON. No issues.
96-96: Good centralization via MaskValue
Delegating all per-arg masking through MaskValue decreases duplication and keeps rules consistent.
142-145: Constructor — LGTM
Simple and clear; keeps call sites readable.sys/result.go (2)
15-18: IsOk implementation looks good.
47-56: Constructors Ok/Err are correct and zero-value safe.sys/result_test.go (2)
10-38: Table tests for IsOk are solid.
65-71: General: tests are readable and comprehensive across types.Also applies to: 73-79, 81-92, 110-134
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
sys/result.go (1)
20-40: IsErr hardening looks good (nil fast-path + skip nil checks)Matches prior guidance: avoids errors.Is with nil and returns early on nil Err.
🧹 Nitpick comments (5)
sys/result.go (1)
42-57: Guard added; consider tiny naming nit in loopBehavior is correct and the nil guard prevents panics. Minor readability nit: avoid naming the loop var
errsince it’s a string, not an error.- for _, err := range checks { - if strings.Contains(val, err) { + for _, s := range checks { + if strings.Contains(val, s) { return true } }sys/net.go (1)
21-25: Align the comment with actual behavior.If you keep the broader detection, update the doc to mention loopback ranges (127/8), IPv6 ::1, and unspecified 0.0.0.0/::; also clarify it accepts either a full URL or host[:port].
string/mask_test.go (3)
147-166: Avoid shadowing the encoding/json import with a local variableUsing json as a local name reduces readability and can confuse future edits.
- json, err := ms.MarshalJSON() + b, err := ms.MarshalJSON() assert.NoError(t, err) - assert.Equal(t, tt.want, string(json)) + assert.Equal(t, tt.want, string(b))
168-187: Avoid shadowing the yaml import with a local variable- yaml, err := ms.MarshalYAML() + v, err := ms.MarshalYAML() assert.NoError(t, err) - assert.Equal(t, tt.want, yaml) + assert.Equal(t, tt.want, v)
236-246: Don’t ignore json.Marshal errors; also confirm intentional unmasked JSON/YAML semanticsMarshal on strings shouldn’t error, but it’s cheap to assert. Separately, these tests codify that JSON/YAML emit unmasked values; please confirm this is the intended contract for any code paths that might log or externalize these payloads.
- jsonBytes, err := ms.MarshalJSON() + jsonBytes, err := ms.MarshalJSON() assert.NoError(t, err) - expectedJSON, _ := json.Marshal(input) - assert.Equal(t, expectedJSON, jsonBytes) + expJSON, expErr := json.Marshal(input) + assert.NoError(t, expErr) + assert.Equal(t, expJSON, jsonBytes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
string/mask_test.go(2 hunks)string/string_test.go(1 hunks)sys/net.go(1 hunks)sys/net_test.go(1 hunks)sys/result.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sys/net_test.go
- string/string_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
string/mask_test.go (2)
string/mask.go (3)
NewMaskedString(143-145)Mask(13-23)MaskedString(102-102)logger/console.go (1)
NewConsoleLogger(245-252)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
sys/result.go (2)
15-18: IsOk implementation is clear and correctSimple, idiomatic check against nil error.
59-68: Constructors are ergonomic and correctOk initializes with value; Err zeroes T and carries error. API is clear.
string/mask_test.go (3)
189-212: LGTM: Text() returns the raw value as specifiedGood coverage across edge cases and Unicode.
267-299: LGTM: Formatting verbs behavior is consistent%v, %s, and %#v aligning with String() is clearly validated.
455-505: LGTM: JSON/YAML round-trips preserve underlying values and masking semanticsEnd-to-end behavior is well covered.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
sys/net.go (3)
24-35: Normalize whitespace and trailing dot for localhost FQDN.
Trim spaces and accept "localhost." as equivalent to "localhost".Apply:
host := url if u, err := neturl.Parse(url); err == nil && u.Host != "" { host = u.Hostname() // strips [] for IPv6 } else if h, _, err := net.SplitHostPort(url); err == nil { host = h } else { host = strings.Trim(host, "[]") } + host = strings.TrimSpace(host) + host = strings.TrimSuffix(host, ".") if strings.EqualFold(host, "localhost") { return true }
36-38: Also treat IPv4-mapped loopback (::ffff:127.0.0.1) as local.
Map IPv6-mapped addresses to v4 before checks.Apply:
- if ip := net.ParseIP(host); ip != nil { - return ip.IsLoopback() || ip.IsUnspecified() // 127/8, ::1, 0.0.0.0, :: - } + if ip := net.ParseIP(host); ip != nil { + if ip4 := ip.To4(); ip4 != nil { + ip = ip4 + } + return ip.IsLoopback() || ip.IsUnspecified() // 127/8, ::1, 0.0.0.0, :: + }
22-39: Add a couple of negative/edge tests.
To lock behavior:
- true: "localhost", "LOCALHOST", "localhost:80", "http://localhost", "127.0.0.2", "http://[::1]:443", "http://[::]"
- true (after refactor above): "http://[::ffff:127.0.0.1]"
- false: "localhost.evil.com", "http://localhost.evil.com", "127.0.0.1.nip.io", "http://[fe80::1%25en0]"
logger/console_test.go (4)
13-20: Make captureOutput race-safe for parallel testslog.SetOutput is global; concurrent captureOutput calls (or other tests using the std logger) can interleave. Guard the swap with a mutex.
func captureOutput(f func()) string { - prev := log.Writer() + // serialize global std logger redirection + testLogMu.Lock() + defer testLogMu.Unlock() + prev := log.Writer() var buf bytes.Buffer log.SetOutput(&buf) defer log.SetOutput(prev) f() return strings.TrimSpace(buf.String()) }Add once in this file (outside the function):
// at top-level in this _test.go file: var testLogMu sync.MutexAnd import:
import "sync"
14-17: Also restore std logger flags/prefix to fully isolate testsIf code under test tweaks log flags/prefix, restore them to avoid cross-test bleed.
- prev := log.Writer() + prev := log.Writer() + prevFlags := log.Flags() + prevPrefix := log.Prefix() var buf bytes.Buffer log.SetOutput(&buf) - defer log.SetOutput(prev) + defer func() { + log.SetOutput(prev) + log.SetFlags(prevFlags) + log.SetPrefix(prevPrefix) + }()
110-128: Prefer t.Setenv for test-scoped env varsSimpler and safer than manual Setenv/Unsetenv, especially on failures.
- os.Setenv("AGENTUITY_LOG_LEVEL", tt.level) + t.Setenv("AGENTUITY_LOG_LEVEL", tt.level) logger := NewConsoleLogger().(*consoleLogger) @@ - os.Unsetenv("AGENTUITY_LOG_LEVEL")
19-19: Normalize trailing newlines by using TrimRight
Optional refactor to avoid stripping other whitespace:- return strings.TrimSpace(buf.String()) + return strings.TrimRight(buf.String(), "\r\n")Tests don’t rely on trimming spaces beyond CR/LF.
string/string.go (1)
20-26: Minor tidy: drop else after returnFlatten for readability.
- if nv == "" { - return nil - } else { - return sys.Ptr(nv) - } + if nv == "" { + return nil + } + return sys.Ptr(nv)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
logger/console_test.go(1 hunks)string/mask_test.go(2 hunks)string/string.go(2 hunks)sys/net.go(2 hunks)sys/pointer.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- string/mask_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
string/string.go (1)
sys/pointer.go (1)
Ptr(10-12)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
sys/net.go (2)
5-5: Good import alias to avoid shadowing and keep intent clear.
Theneturlalias prevents confusion with theurlparam. All good.
22-39: Secure host parsing replaces brittle substring checks — resolved.
You’re correctly extracting the hostname and using net.IP semantics (loopback/unspecified). This addresses the prior vuln and covers 127/8, ::1, 0.0.0.0, ::.logger/console_test.go (1)
14-18: Nice: reliably restoring the previous log writerSaving prev and deferring restoration avoids leaking the test writer even on panic. Good improvement.
sys/pointer.go (1)
9-12: Ptr helper looks goodReturning &v is idiomatic; each call yields a distinct address and avoids the “address of range var” pitfall.
string/string.go (1)
3-7: Importing sys is fineDependency looks correct and local; no cycle with sys.
| if v != "" { | ||
| nv := strings.TrimSpace(v) | ||
| if nv == "" { | ||
| return nil | ||
| } else { | ||
| return &nv | ||
| } | ||
| return sys.Ptr(nv) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Whitespace-only inputs now return non-nil "" pointer (likely unintended)
This changes behavior and makes StringPointer inconsistent with ClearEmptyStringPointer. If you want empty/whitespace → nil, trim first and check length.
-func StringPointer(v string) *string {
- if v != "" {
- nv := strings.TrimSpace(v)
- return sys.Ptr(nv)
- }
- return nil
-}
+func StringPointer(v string) *string {
+ nv := strings.TrimSpace(v)
+ if nv == "" {
+ return nil
+ }
+ return sys.Ptr(nv)
+}Consider updating the comment to clarify “empty or whitespace-only returns nil; otherwise pointer to trimmed value.”
🏁 Script executed:
#!/bin/bash
set -eux
# Locate and display StringPointer implementation with context
rg -n -C3 "func StringPointer" .Length of output: 392
🏁 Script executed:
#!/bin/bash
set -eux
# Show full StringPointer function for clarity
rg -n -C3 "func StringPointer" string/string.go
# Search for ClearEmptyStringPointer and related functions in the repo
rg -n -C3 "ClearEmptyStringPointer" .
rg -n -C3 "EmptyStringPointer" .Length of output: 2001
Refactor StringPointer to return nil for empty or whitespace-only input
Whitespace-only inputs currently produce a non-nil pointer to ""—inconsistent with ClearEmptyStringPointer. Change implementation to trim first and return nil when nv=="".
func StringPointer(v string) *string {
- if v != "" {
- nv := strings.TrimSpace(v)
- return sys.Ptr(nv)
- }
- return nil
+ nv := strings.TrimSpace(v)
+ if nv == "" {
+ return nil
+ }
+ return sys.Ptr(nv)
}Also update the doc comment to:
StringPointer returns nil for empty or whitespace-only input; otherwise returns a pointer to the trimmed value.
🤖 Prompt for AI Agents
In string/string.go around lines 11 to 15, the function should trim input first
and return nil for empty or whitespace-only input; update the doc comment to:
"StringPointer returns nil for empty or whitespace-only input; otherwise returns
a pointer to the trimmed value." Modify the implementation to call
strings.TrimSpace(v) into nv, and if nv == "" return nil, otherwise return
sys.Ptr(nv).
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove test-only imports from production file
This drags testing/testify into the build. Move tests to a *_test.go file and drop these imports here.
-import (
- "testing"
-
- "github.com/stretchr/testify/assert"
-)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ( | |
| "testing" | |
| "github.com/stretchr/testify/assert" | |
| ) |
🤖 Prompt for AI Agents
In sys/pointer.go around lines 3 to 7, the production file currently imports
"testing" and "github.com/stretchr/testify/assert", which are test-only; move
any test code using these imports into a new *_test.go file (or existing test
file) and remove the test imports from sys/pointer.go so the production build no
longer depends on testing/testify. Ensure the moved tests import testing and
testify in the test file and that sys/pointer.go only contains production
imports.
| func TestPtr(t *testing.T) { | ||
| // Test with string | ||
| str := "hello" | ||
| strPtr := Ptr(str) | ||
| assert.NotNil(t, strPtr) | ||
| assert.Equal(t, str, *strPtr) | ||
|
|
||
| // Test with int | ||
| num := 42 | ||
| numPtr := Ptr(num) | ||
| assert.NotNil(t, numPtr) | ||
| assert.Equal(t, num, *numPtr) | ||
|
|
||
| // Test with bool | ||
| flag := true | ||
| flagPtr := Ptr(flag) | ||
| assert.NotNil(t, flagPtr) | ||
| assert.Equal(t, flag, *flagPtr) | ||
|
|
||
| // Test with struct | ||
| type testStruct struct { | ||
| Name string | ||
| Age int | ||
| } | ||
| s := testStruct{Name: "test", Age: 25} | ||
| sPtr := Ptr(s) | ||
| assert.NotNil(t, sPtr) | ||
| assert.Equal(t, s, *sPtr) | ||
|
|
||
| // Test with slice | ||
| slice := []int{1, 2, 3} | ||
| slicePtr := Ptr(slice) | ||
| assert.NotNil(t, slicePtr) | ||
| assert.Equal(t, slice, *slicePtr) | ||
|
|
||
| // Test with nil interface | ||
| var nilInterface interface{} | ||
| nilPtr := Ptr(nilInterface) | ||
| assert.NotNil(t, nilPtr) | ||
| assert.Equal(t, nilInterface, *nilPtr) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Move tests to sys/pointer_test.go
Test code in a non-test file bloats prod deps and won’t run under go test. Remove from this file and add a dedicated test file.
-func TestPtr(t *testing.T) {
- // Test with string
- str := "hello"
- strPtr := Ptr(str)
- assert.NotNil(t, strPtr)
- assert.Equal(t, str, *strPtr)
-
- // Test with int
- num := 42
- numPtr := Ptr(num)
- assert.NotNil(t, numPtr)
- assert.Equal(t, num, *numPtr)
-
- // Test with bool
- flag := true
- flagPtr := Ptr(flag)
- assert.NotNil(t, flagPtr)
- assert.Equal(t, flag, *flagPtr)
-
- // Test with struct
- type testStruct struct {
- Name string
- Age int
- }
- s := testStruct{Name: "test", Age: 25}
- sPtr := Ptr(s)
- assert.NotNil(t, sPtr)
- assert.Equal(t, s, *sPtr)
-
- // Test with slice
- slice := []int{1, 2, 3}
- slicePtr := Ptr(slice)
- assert.NotNil(t, slicePtr)
- assert.Equal(t, slice, *slicePtr)
-
- // Test with nil interface
- var nilInterface interface{}
- nilPtr := Ptr(nilInterface)
- assert.NotNil(t, nilPtr)
- assert.Equal(t, nilInterface, *nilPtr)
-}Add this new file:
// sys/pointer_test.go
package sys
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestPtr(t *testing.T) {
str := "hello"
strPtr := Ptr(str)
assert.NotNil(t, strPtr)
assert.Equal(t, str, *strPtr)
num := 42
numPtr := Ptr(num)
assert.NotNil(t, numPtr)
assert.Equal(t, num, *numPtr)
flag := true
flagPtr := Ptr(flag)
assert.NotNil(t, flagPtr)
assert.Equal(t, flag, *flagPtr)
type testStruct struct {
Name string
Age int
}
s := testStruct{Name: "test", Age: 25}
sPtr := Ptr(s)
assert.NotNil(t, sPtr)
assert.Equal(t, s, *sPtr)
slice := []int{1, 2, 3}
slicePtr := Ptr(slice)
assert.NotNil(t, slicePtr)
assert.Equal(t, slice, *slicePtr)
var nilInterface interface{}
nilPtr := Ptr(nilInterface)
assert.NotNil(t, nilPtr)
assert.Equal(t, nilInterface, *nilPtr)
}Run to ensure no prod file imports test deps:
🏁 Script executed:
#!/bin/bash
rg -nP --type=go -g '!**/*_test.go' -C2 '^\s*import\s+"testing"'
rg -nP --type=go -g '!**/*_test.go' -C2 'github.com/stretchr/testify/assert'Length of output: 281
Move TestPtr out of sys/pointer.go into sys/pointer_test.go
sys/pointer.go must not import testing or github.com/stretchr/testify/assert or contain any test code—tests belong in _test.go files. Remove the following from sys/pointer.go (lines 14–54):
-func TestPtr(t *testing.T) {
- // Test with string
- str := "hello"
- strPtr := Ptr(str)
- assert.NotNil(t, strPtr)
- assert.Equal(t, str, *strPtr)
-
- // Test with int
- num := 42
- numPtr := Ptr(num)
- assert.NotNil(t, numPtr)
- assert.Equal(t, num, *numPtr)
-
- // Test with bool
- flag := true
- flagPtr := Ptr(flag)
- assert.NotNil(t, flagPtr)
- assert.Equal(t, flag, *flagPtr)
-
- // Test with struct
- type testStruct struct {
- Name string
- Age int
- }
- s := testStruct{Name: "test", Age: 25}
- sPtr := Ptr(s)
- assert.NotNil(t, sPtr)
- assert.Equal(t, s, *sPtr)
-
- // Test with slice
- slice := []int{1, 2, 3}
- slicePtr := Ptr(slice)
- assert.NotNil(t, slicePtr)
- assert.Equal(t, slice, *slicePtr)
-
- // Test with nil interface
- var nilInterface interface{}
- nilPtr := Ptr(nilInterface)
- assert.NotNil(t, nilPtr)
- assert.Equal(t, nilInterface, *nilPtr)
-}And add a new file sys/pointer_test.go containing:
package sys
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestPtr(t *testing.T) {
str := "hello"
strPtr := Ptr(str)
assert.NotNil(t, strPtr)
assert.Equal(t, str, *strPtr)
num := 42
numPtr := Ptr(num)
assert.NotNil(t, numPtr)
assert.Equal(t, num, *numPtr)
flag := true
flagPtr := Ptr(flag)
assert.NotNil(t, flagPtr)
assert.Equal(t, flag, *flagPtr)
type testStruct struct {
Name string
Age int
}
s := testStruct{Name: "test", Age: 25}
sPtr := Ptr(s)
assert.NotNil(t, sPtr)
assert.Equal(t, s, *sPtr)
slice := []int{1, 2, 3}
slicePtr := Ptr(slice)
assert.NotNil(t, slicePtr)
assert.Equal(t, slice, *slicePtr)
var nilInterface interface{}
nilPtr := Ptr(nilInterface)
assert.NotNil(t, nilPtr)
assert.Equal(t, nilInterface, *nilPtr)
}🤖 Prompt for AI Agents
In sys/pointer.go around lines 14–54 you have test code (TestPtr) and imports
for testing/testify which must be removed; delete the entire TestPtr function
and any testing/testify imports from sys/pointer.go, then create a new file
sys/pointer_test.go with package sys that imports "testing" and
"github.com/stretchr/testify/assert" and contains the TestPtr function as shown
in the review comment so tests live in a _test.go file and production code no
longer imports testing.
1 participant
Summary by CodeRabbit