-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Included tests for internal/signalio/json.go #306
base: main
Are you sure you want to change the base?
Conversation
- Included tests for internal/signalio/json.go - Removed un-necessary error check Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
ab4fffb
to
d523bf1
Compare
I have removed: criticality_score/internal/signalio/json.go Lines 54 to 57 in b148d93
I have replaced it with: I replaced this part because |
type mockWriterJSON struct{} | ||
|
||
func (m *mockWriterJSON) Write(p []byte) (n int, err error) { | ||
return 0, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a bytes.Buffer
to avoid needing to implement the io.Writer
interface.
It also allows you to inspect what was written.
signals []signal.Set | ||
extra []Field | ||
} | ||
test := struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I suggest restructuring these tests into the following the test functions as it isn't really clear what this function is trying to test:
- An end-to-end style test that:
- creates the writer with a
bytes.Buffer
. - passes in test data for all the parameters.
- checks the output matches a JSON string (e.g.
want := "{...}"
).
- A test that checks a writer error with a
io.Writer
implementation that always returns an error (likeiotest.ErrReader
, but an equivalent forio.Writer
doesn't exist). - (Optional) a test that exercises encoding errors.
These should ideally not contain the test struct building at the start as it isn't really necessary.
return fmt.Errorf("failed to get map for namespace: %s", ns) | ||
} | ||
nsData := d.(map[string]any) | ||
// we don't need to check for an error here (the above line) because d is always a map[string]any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth extending the comment to say that "if this fails in the future the assignment to nsData below will fail"
Signed-off-by: nathannaveen 42319948+nathannaveen@users.noreply.github.com