Uh oh!
There was an error while loading. Please reload this page.
test: add test coverage for untested files - #388
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #388 +/- ##
==========================================
+ Coverage 65.48% 66.96% +1.47%
==========================================
Files 218 218 Lines 18090 18090 ==========================================
+ Hits 11847 12114 +267 + Misses 5154 4839 -315 - Partials 1089 1137 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mwbrooks
left a comment
There was a problem hiding this comment.
Comments the kind ones 🤾🏻
| // TODO: Refactor to use afero.Fs once ExtractAndWriteFile accepts it. Currently uses t.TempDir() which is safe. | ||
| func Test_ExtractAndWriteFile(t *testing.T) { |
There was a problem hiding this comment.
note: I've decided to implement these tests now using the t.TempDir() approach, but I've added a comment to refactor the code being tested to inject afero.Fs so that we can change the tests to be our memory-base FS.
There was a problem hiding this comment.
@mwbrooks Amazing callout! Having these tests in place will make that change much more confident I think! 🧪 ✨
There was a problem hiding this comment.
Happy to hear that you agree. I was on the fence, since we are trying to avoid I/O during testing. But I'll be sure to follow-up and remove these in an upcoming PR.
| // TODO: Refactor to use afero.Fs once Copy accepts it. Currently uses t.TempDir() which is safe. | ||
| func Test_Copy(t *testing.T) { |
There was a problem hiding this comment.
note: More reminders to refactor this, but these tests are safe to run since they are in the temp directory (just slower).
| // TODO: Refactor to use afero.Fs once CopyDirectory accepts it. Currently uses t.TempDir() which is safe. | ||
| func Test_CopyDirectory(t *testing.T) { | ||
| t.Run("copies directory structure", func(t *testing.T) { |
There was a problem hiding this comment.
note: Not using Table Tests here because each test case needs unique setup. To find a balance, I'm using the t.Run("...") to still group the test cases together in the same spirit as table tests.
srtaalej
left a comment
There was a problem hiding this comment.
LGTM! thanks for increasing our coverage 🟢 🟢 🟢 🚀
| // TODO: Refactor to use afero.Fs once ExtractAndWriteFile accepts it. Currently uses t.TempDir() which is safe. | ||
| func Test_ExtractAndWriteFile(t *testing.T) { |
There was a problem hiding this comment.
@mwbrooks Amazing callout! Having these tests in place will make that change much more confident I think! 🧪 ✨
There was a problem hiding this comment.
⭐ praise: These tests aren't so complicated but that's amazing I think! It gives me confidence that existing API methods are gathering expected responses as we expect!
There was a problem hiding this comment.
🧪 thought: Increasing coverage is appreciated so much but I'm worried that it builds confidence in patterns we're wanting to move from. I don't think command specific prompts ought be separate from commands but let's not address that now!
There was a problem hiding this comment.
thought: I think it's entirely okay to delete old, unrelated tests. So I hope it doesn't discourage future refactors.
| assert.NotEmpty(t, hostname) | ||
| // The hostname should be hashed, not the raw hostname | ||
| // It should not be "unknown" on a normal system | ||
| assert.NotEqual(t, "", hostname) |
There was a problem hiding this comment.
🗣️ question(non-blocking): Is not empty the same as not equal to empty string?
Changelog
Summary
This pull request add unit test coverage for 10 previously untested files across internal packages:
internal/apiinternal/archiveutilinternal/cmdutilinternal/configinternal/goutilsinternal/imageinternal/iostreamsinternal/ioutilsinternal/promptsOnly test files were changed in this pull request.
Requirements