test: isolates temp test fixtures - #1569
Conversation
|
@rdimitrov quick cleanup PR. Would love to help more here! No urgency! |
JosephDoUrden
left a comment
There was a problem hiding this comment.
Tested this one properly. Planted /tmp/non-existent-file.json on main and TestImportService_ErrorHandling really does fail there, the non-existent case "succeeds" and then nil derefs. Same planted file against this branch, immune, t.TempDir() is guaranteed empty. So the importer_test change is a real fix, not just cleanup. The commands ones were already unique via MkdirTemp so those are cleanup, but the autocleanup is still nicer. Also the chdir error is now actually checked instead of swallowed, good.
go test -race -count=2 on both packages green, golangci-lint v2.13.1 0 issues, branch is 0 behind main.
One small thing btw, the title got renamed to fix: but the change is test only and the single commit still says test:. Squash takes the PR title here so this would land in the changelog as a bug fix. I'd rename it back to test:.
Looks merge-ready apart from that.
|
@JosephDoUrden updated the PR title. Thank you for the review! |
|
Title looks right now and the branch is unchanged since my review, so nothing else from me. Still merge-ready. |
rdimitrov
left a comment
There was a problem hiding this comment.
@yowainwright @JosephDoUrden - Thank you both! 🚀 🙌
There was a problem hiding this comment.
🟢 Approval recommended
The changes are test-only, improve isolation/cleanup, and the updated temp-dir usage and error checks look correct and consistent across the touched tests.
Pull request overview
This PR improves test isolation by replacing shared/global temp paths with t.TempDir() so each test gets its own automatically cleaned-up temporary directory, reducing cross-test interference and leftover files.
Changes:
- Updated importer tests to write seed/invalid JSON files under
t.TempDir()instead of OS-level temp locations and manual cleanup. - Updated publisher command tests to use
t.TempDir()and to fail fast ifos.Chdirfails. - Simplified test utilities by using
t.TempDir()rather than manually creating/removing temp directories.
File summaries
| File | Description |
|---|---|
| internal/importer/importer_test.go | Moves temp file usage to t.TempDir() and simplifies error-case fixtures for missing/invalid files. |
| cmd/publisher/commands/validate_test.go | Uses t.TempDir() for per-test working directories and checks os.Chdir errors. |
| cmd/publisher/commands/testutil_test.go | Uses t.TempDir() for server.json fixtures and relies on Go’s built-in temp cleanup. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Motivation and Context
Replace shared temp paths with `t.TempDir()`` so each test gets its own cleanup-scoped directory.
How Has This Been Tested?
GitHub CI passed.
Breaking Changes
No breaking changes; just tests.
Types of changes
Checklist
Additional context
Small test-only cleanup. No production code paths are changed.