Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nathannaveen
Copy link
Contributor

  • Included tests for internal/signalio/json.go
  • Removed un-necessary error check

Signed-off-by: nathannaveen [email protected]

- Included tests for internal/signalio/json.go
- Removed un-necessary error check

Signed-off-by: nathannaveen <[email protected]>
@nathannaveen nathannaveen force-pushed the nathan/feat/signalio/jsonTest branch from ab4fffb to d523bf1 Compare January 11, 2023 18:09
@nathannaveen
Copy link
Contributor Author

I have removed:

nsData, ok := d.(map[string]any)
if !ok {
return fmt.Errorf("failed to get map for namespace: %s", ns)
}

I have replaced it with: nsData := d.(map[string]any).

I replaced this part because d is always set to a map[string] any before the check, so it will never be !ok.

Comment on lines +34 to +38
type mockWriterJSON struct{}

func (m *mockWriterJSON) Write(p []byte) (n int, err error) {
return 0, nil
}
Copy link
Contributor

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 {
Copy link
Contributor

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:

  1. 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 := "{...}").
  1. A test that checks a writer error with a io.Writer implementation that always returns an error (like iotest.ErrReader, but an equivalent for io.Writer doesn't exist).
  2. (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
Copy link
Contributor

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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants