chore: remove stray test artifacts from the source tree#57
Conversation
pkg/certificate/der, /pem and /invalid are tracked, zero-byte files. They are leftovers from an older ExportCertificate test that passed the format string as the filename, so it wrote "pem", "der" and "invalid" into the package directory. The test has since moved to temp files, but the artifacts were committed and stayed. TestExportLogic still wrote test_export.pem into internal/model on every run. It is gitignored, so it never got committed, but it is the same mistake and it would eventually make the same mess. Point it at t.TempDir and assert the file actually lands there.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a CLAUDE.md file to guide AI development and refactors internal/model/logic_test.go to write test exports to a temporary directory instead of leaving artifacts in the package directory. The review feedback suggests shadowing the model variable locally within the subtest to prevent shared state mutation and adding an assertion to ensure the exported certificate file is not empty.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| m = m.handleExportCommand(target) | ||
| if m.viewMode != ViewPopup || m.popupType != PopupAlert { | ||
| t.Errorf("Expected PopupAlert after export") | ||
| } | ||
| if !strings.Contains(m.popupMessage, "successfully") { | ||
| t.Errorf("Expected success message, got %q", m.popupMessage) | ||
| } | ||
| if _, err := os.Stat(target); err != nil { | ||
| t.Errorf("expected the certificate to be written: %v", err) | ||
| } |
There was a problem hiding this comment.
In Go, subtests should be isolated and avoid mutating shared state from the outer scope. Reassigning m directly mutates the shared m variable, which can lead to fragile tests or side effects if more tests are added or if they are run in a different order. Using m := m.handleExportCommand(target) shadows m locally within the subtest, keeping it isolated.
Additionally, since the original issue was about empty files being left behind, we should assert that the written file is not empty (info.Size() > 0) to prevent regressions.
| m = m.handleExportCommand(target) | |
| if m.viewMode != ViewPopup || m.popupType != PopupAlert { | |
| t.Errorf("Expected PopupAlert after export") | |
| } | |
| if !strings.Contains(m.popupMessage, "successfully") { | |
| t.Errorf("Expected success message, got %q", m.popupMessage) | |
| } | |
| if _, err := os.Stat(target); err != nil { | |
| t.Errorf("expected the certificate to be written: %v", err) | |
| } | |
| m := m.handleExportCommand(target) | |
| if m.viewMode != ViewPopup || m.popupType != PopupAlert { | |
| t.Errorf("Expected PopupAlert after export") | |
| } | |
| if !strings.Contains(m.popupMessage, "successfully") { | |
| t.Errorf("Expected success message, got %q", m.popupMessage) | |
| } | |
| info, err := os.Stat(target) | |
| if err != nil { | |
| t.Errorf("expected the certificate to be written: %v", err) | |
| } else if info.Size() == 0 { | |
| t.Errorf("expected the written certificate file to be non-empty") | |
| } |
1 participant
Three empty files are tracked in the repository
They are leftovers from an older
ExportCertificatetest that passed the format string as the filename, so it wrotepem,derandinvalidinto the package directory. The test has since moved to temp files, but the artifacts were committed and never cleaned up. (They arrived, fittingly, in a commit titled "clean up README and test files".)The same mistake is still live
TestExportLogicwritestest_export.pemintointernal/model/on everygo testrun. It is gitignored, so it has not been committed — but it is the identical bug and would eventually make the identical mess.Point it at
t.TempDir(), and while there, actually assert the certificate lands on disk (the test only checked the popup message, so it would have passed even if the write silently failed).Verified
go test ./...now leaves the working tree clean.