-
Notifications
You must be signed in to change notification settings - Fork 30
test: increase test coverage for iostreams #394
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
base: main
Are you sure you want to change the base?
Changes from all commits
5fee7f6
64dfb2c
6188979
fc34422
4e0ebd1
afa0729
5552c10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -383,6 +383,30 @@ The branch name can also be set by changing | |
| for the `build-lint-test-e2e-test` workflow in the `.circleci/config.yml` file, | ||
| but take care not to merge this change into `main`! | ||
|
|
||
| #### Test naming conventions | ||
|
|
||
| Test function names should use the format `Test_StructName_FunctionName` for methods | ||
| on a struct, or `Test_FunctionName` for package-level functions. The underscore after | ||
| `Test` separates the Go test prefix from the identifier being tested: | ||
|
|
||
| ```go | ||
| // Testing a method on a struct | ||
| func Test_Client_GetAppStatus(t *testing.T) { ... } | ||
|
|
||
| // Testing a package-level function | ||
| func Test_getKeyLength(t *testing.T) { ... } | ||
| ``` | ||
|
Comment on lines
+392
to
+398
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⭐ praise: Thanks for calling out the difference in method and function testing! |
||
|
|
||
| #### Test ordering conventions | ||
|
|
||
| Test functions should be ordered alphabetically within each file. When a file has | ||
| logical sections (separated by comments), tests should be alphabetical within each | ||
| section. | ||
|
|
||
| Getter and setter functions should be grouped together under the base name. Ignore | ||
| the `Get` or `Set` prefix when determining alphabetical order. For example, | ||
| `Test_AppName` and `Test_SetAppName` are both sorted under `A` for `AppName`. | ||
|
|
||
| #### Contributing tests | ||
|
|
||
| If you'd like to add tests, please review our | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,10 +20,41 @@ import ( | |
|
|
||
| "github.com/slackapi/slack-cli/internal/config" | ||
| "github.com/slackapi/slack-cli/internal/slackdeps" | ||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func Test_IOStreams_ExitCode(t *testing.T) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: Alphabetically ordered as best as we can. 🔤
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧮 praise: I'm often in favor for simple later additions! 💡 question: Would matching implementation order perhaps make this more clear too? I understand one function should have one test with perhaps multiple cases. I'd expect to find 👾 ramble: I'm unsure what's most common in codebase current but we might find orderings of implementation can be motivating in these changes? |
||
| tests := map[string]struct { | ||
| setCode ExitCode | ||
| expected ExitCode | ||
| }{ | ||
| "default is ExitOK": { | ||
| setCode: ExitOK, | ||
| expected: ExitOK, | ||
| }, | ||
| "set to ExitError": { | ||
| setCode: ExitError, | ||
| expected: ExitError, | ||
| }, | ||
| "set to ExitCancel": { | ||
| setCode: ExitCancel, | ||
| expected: ExitCancel, | ||
| }, | ||
| } | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| fsMock := slackdeps.NewFsMock() | ||
| osMock := slackdeps.NewOsMock() | ||
| cfg := config.NewConfig(fsMock, osMock) | ||
| io := NewIOStreams(cfg, fsMock, osMock) | ||
| io.SetExitCode(tc.setCode) | ||
| assert.Equal(t, tc.expected, io.GetExitCode()) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func Test_IOSteams_NewIOStreams(t *testing.T) { | ||
| var io *IOStreams | ||
| fsMock := slackdeps.NewFsMock() | ||
|
|
@@ -65,3 +96,15 @@ func Test_IOStreams_IsTTY(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func Test_IOStreams_SetCmdIO(t *testing.T) { | ||
| fsMock := slackdeps.NewFsMock() | ||
| osMock := slackdeps.NewOsMock() | ||
| cfg := config.NewConfig(fsMock, osMock) | ||
| io := NewIOStreams(cfg, fsMock, osMock) | ||
| cmd := &cobra.Command{Use: "test"} | ||
| io.SetCmdIO(cmd) | ||
| assert.NotNil(t, cmd.InOrStdin()) | ||
| assert.NotNil(t, cmd.OutOrStdout()) | ||
| assert.NotNil(t, cmd.ErrOrStderr()) | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😳 note: These are awesome tests to find! We made some changes to 👾 ramble: Since this PR has no functional changes I'd be curious in preferring the adjacent tests where we define interfaces more clear! |
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.
note: Helping Claude be better 💪🏻