test: assert cmd.ExecuteContext flushes exit code to logstash#64
Conversation
mwbrooks
added
code health
| // Assertions | ||
| // TODO: Assert that the event tracker was called with the correct exit code | ||
| require.Equal(t, tt.expectedExitCode, clients.IO.GetExitCode()) | ||
| clientsMock.EventTracker.AssertCalled(t, "FlushToLogstash", mock.Anything, mock.Anything, mock.Anything, tt.expectedExitCode) |
There was a problem hiding this comment.
note: This is the most important part of the PR. We now assert that the correct exit code is flushed to Logstash.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #64 +/- ##
==========================================
- Coverage 63.12% 63.10% -0.02%
==========================================
Files 210 210
Lines 22159 22159
==========================================
- Hits 13987 13984 -3
- Misses 7086 7087 +1
- Partials 1086 1088 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@mwbrooks LGTM! 🧪 ✨
I didn't mean for additional refactor needed to test exit codes here, but it's nice to see the improvements to the testing suite.
Commit e35caa8 removed perhaps past flushes from iostreams instead of the event tracker since I don't believe that's used now, but please feel free to revert that as needed. I could not suggest it so quick otherwise 😅
I'm also bummed to see coverage drop from happenchance of the actual test run - IIRC ordering random lists is difficult to make deterministic - but the actual tests are improved so much here! 🚢 💨
| @@ -28,13 +28,32 @@ type EventTrackerMock struct { | |||
|
|
|||
| func (m *EventTrackerMock) AddDefaultMocks() { | |||
| m.On("FlushToLogstash", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) | |||
There was a problem hiding this comment.
| m.On("FlushToLogstash", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) |
🪓 Ignore this suggestion but I'm now wondering if we can be confident that we're setting up all mock implementations within tests for this method, so a default is no longer needed?
There was a problem hiding this comment.
Sure, I'm okay with that. We should think about how we want to approach the .DefaultMocks(). I think it saves us a lot of scaffolded code, but sometimes comes off as magical and leads to brittle tests that dependent on the default mocks. 🤔
However, since FlushToLogstash is only used on root.go, I think it's a safe one to always mock explicitly.
Commit 5128747 removes the default mock and declares it explicitly in the root_test.go table test.
|
|
||
| func (m *IOStreamsMock) FlushToLogstash(ctx context.Context) error { return nil } |
There was a problem hiding this comment.
🪓 Removed this implementations since this method no longer exists within iostreams!
There was a problem hiding this comment.
Ah great catch! I noticed that while refactoring, but it slipped from my mind.
Ha! No worries, this is a healthy refactor, so I'm glad jumped on the opportunity 🤾🏻
Totally cool and this is a very good catch! Thanks for pushing up the commit!
Yea, it's unfortunate, but overall we're inching toward that 70% mark. There's still some low hanging fruit 🍒 that we can use to increase the coverage more as well! |
2 participants
Summary
This pull request is a follow-up for #61 (comment) that asserts
cmd.ExecuteContextflushes the correct exit code to Logstash.In order for this to work, we needed to fix the implementation of the
EventTrackerMockby adding a few missing functions, writing it up toClientsMock, and adding default mocks to allow our existing tests to pass.I think we're at the point where we can test Logstash flushes throughout the code! 🎉
Requirements