Skip to content

Add comprehensive test suite covering all commands and edge cases#6

Merged
vedssharma merged 1 commit intomainfrom
claude/add-test-suite-3ZG6t
Apr 1, 2026
Merged

Add comprehensive test suite covering all commands and edge cases#6
vedssharma merged 1 commit intomainfrom
claude/add-test-suite-3ZG6t

Conversation

@vedssharma
Copy link
Copy Markdown
Owner

  • internal/http: URL validation (SSRF, cloud metadata blocking, private IPs),
    all HTTP methods (GET/POST/PUT/PATCH/DELETE) via httptest, header handling
  • internal/format: sanitizeOutput (ANSI escapes, control chars, DEL),
    prettyJSON (valid/invalid), status color selection, all print function smoke tests
  • internal/storage: full CRUD coverage for history (add, load, limit enforcement,
    clear, get by ID), collections (create, add, order, delete, cascade), aliases
    (create/upsert, get, delete), and parseJSONHeaders edge cases
  • cmd/request: parseHeaders (malformed, multi-value, whitespace trimming),
    filterSensitiveHeaders (all sensitive header types, case-insensitive),
    warnIfSensitiveBody, resolveAlias (full URLs, alias resolution, trailing slash),
    readBodyFromFile (valid, path traversal blocked, symlinks, subdirectories)
  • cmd/export: isJSON, parsePostmanURL (path/query/protocol parsing),
    buildPostmanRequest (body detection, method casing), integration tests for
    buildPostmanFrom{Collection,History,AllCollections}
  • cmd/import: resolvePostmanURL (string, object, reconstruct from parts),
    flattenPostmanItems (nested folders, deep nesting, prefixes),
    convertPostmanRequest (headers, raw body, non-raw mode skip)

176 tests total, all passing.

https://claude.ai/code/session_012s4PVKiEUVA1Sf8zH921ij

- internal/http: URL validation (SSRF, cloud metadata blocking, private IPs),
  all HTTP methods (GET/POST/PUT/PATCH/DELETE) via httptest, header handling
- internal/format: sanitizeOutput (ANSI escapes, control chars, DEL),
  prettyJSON (valid/invalid), status color selection, all print function smoke tests
- internal/storage: full CRUD coverage for history (add, load, limit enforcement,
  clear, get by ID), collections (create, add, order, delete, cascade), aliases
  (create/upsert, get, delete), and parseJSONHeaders edge cases
- cmd/request: parseHeaders (malformed, multi-value, whitespace trimming),
  filterSensitiveHeaders (all sensitive header types, case-insensitive),
  warnIfSensitiveBody, resolveAlias (full URLs, alias resolution, trailing slash),
  readBodyFromFile (valid, path traversal blocked, symlinks, subdirectories)
- cmd/export: isJSON, parsePostmanURL (path/query/protocol parsing),
  buildPostmanRequest (body detection, method casing), integration tests for
  buildPostmanFrom{Collection,History,AllCollections}
- cmd/import: resolvePostmanURL (string, object, reconstruct from parts),
  flattenPostmanItems (nested folders, deep nesting, prefixes),
  convertPostmanRequest (headers, raw body, non-raw mode skip)

176 tests total, all passing.

https://claude.ai/code/session_012s4PVKiEUVA1Sf8zH921ij
@vedssharma vedssharma merged commit 521facc into main Apr 1, 2026
1 check passed
@vedssharma vedssharma deleted the claude/add-test-suite-3ZG6t branch April 1, 2026 01:24
@claude
Copy link
Copy Markdown

claude Bot commented Apr 1, 2026

PR Review: Comprehensive Test Suite

Great addition overall — 176 tests across 6 files with solid coverage of security-sensitive paths (SSRF, path traversal, header redaction). A few things worth addressing:


Issues

1. os.Chdir in setupWorkDir is not parallel-safe (cmd/request_test.go)

os.Chdir is process-wide. If any test using setupWorkDir ever gets t.Parallel() added, it will cause a data race. Consider passing the directory explicitly to functions under test instead, or using t.Chdir() (Go 1.24+) which is cleanup-safe and parallel-aware.

// Better: use t.Chdir (Go 1.24+) instead of rolling your own
func TestReadBodyFromFile_ValidFile(t *testing.T) {
    dir := t.TempDir()
    t.Chdir(dir)
    // ...
}

2. import_time violates Go naming conventions (cmd/export_test.go:~388)

Go uses camelCase for local variables — import_time should be req or histReq.

// Current
import_time := model.Request{...}
store.AddToHistory(import_time)

// Fix
req := model.Request{...}
store.AddToHistory(req)

3. warnIfSensitiveBody tests only assert no-panic, not behavior (cmd/request_test.go)

These tests pass even if the warning logic is accidentally removed:

func TestWarnIfSensitiveBody_ContainsPassword(t *testing.T) {
    warnIfSensitiveBody(`{"username":"alice","password":"s3cr3t"}`)
    // ← no assertion that a warning was actually emitted
}

If warnIfSensitiveBody writes to stderr or returns a boolean, capture and assert it. If it's a pure side-effect (prints a warning), consider refactoring to accept an io.Writer so tests can verify output. As-is, these tests guard against panics but not regressions in the actual warning behavior.

4. New transitive dependencies in go.mod are substantial

The modernc.org/* packages (libc, gc, mathutil, memory, strutil, token) are the CGo-free SQLite implementation — 7 new indirect dependencies totalling significant binary size. Confirm this was already a transitive dep of your SQLite driver and just wasn't pinned before, rather than a newly introduced dependency. If it's new, it should be explicit in the PR description.


Minor Notes

  • TestIsPrivateOrReservedHost doesn't cover IPv6 private ranges (::1, fd00::/8, fe80::/10). If validateURL accepts IPv6, these should be tested.
  • TestConvertPostmanRequest_BodyModeCaseInsensitive tests "RAW" (uppercase). Worth a quick check that the production code uses strings.EqualFold or similar rather than just == "raw" — the test title implies case-insensitivity is intentional behavior.
  • None of the tests use t.Parallel(). For a 176-test suite this is fine, but flagging it as a future optimization opportunity once the os.Chdir issue above is resolved.

What's Working Well

  • Security test coverage is thorough: SSRF (AWS/GCP/Alibaba/ECS metadata endpoints), path traversal (../../../etc/passwd, absolute paths), header redaction (case-insensitive, all sensitive header types)
  • t.Setenv("HOME", t.TempDir()) for storage isolation is correct and clean
  • Error message assertions (not just err != nil) make failures much more debuggable
  • Section comments with box-drawing characters make the test files very readable
  • TestBuildPostmanFromCollection_NotFound asserting "not found" in the error message is a good regression guard

Overall a solid test suite — address the warnIfSensitiveBody assertions and the naming nit before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants