Skip to content

test: increase test coverage in cmd/ and internal/[api | config | deputil] #392

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.

Sign up for GitHub

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

Merged
mwbrooks merged 6 commits into from
Mar 12, 2026
Merged

test: increase test coverage in cmd/ and internal/[api | config | deputil] #392

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a155ba5
test: increase test coverage in cmd/ and internal/api|config|deputil
mwbrooks Mar 12, 2026
28f1bf2
test: rewrite api app tests as table-driven tests
mwbrooks Mar 12, 2026
58c9c3f
test: normalize test function names to use Test_ prefix convention
mwbrooks Mar 12, 2026
c2f5c7e
Merge branch 'main' into mwbrooks-test-coverage-simple-files
mwbrooks Mar 12, 2026
e2e0ed5
test: try to alphabetized app_test.go
mwbrooks Mar 12, 2026
c4dbb36
test: refactor InitializeGlobalFlags to table-driven test with flag d…
mwbrooks Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions cmd/datastore/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,3 +465,55 @@ func prepareExportMockData(cm *shared.ClientsMock, numberOfItems int, maxItemsTo
}
return data, nil
}

func Test_getExpressionPatterns(t *testing.T) {
tests := map[string]struct {
expression string
wantAttrs int
wantVals int
}{
"expression with attributes and values": {
expression: "#name = :name AND #status = :status",
wantAttrs: 2,
wantVals: 2,
},
"empty expression": {
expression: "",
wantAttrs: 0,
wantVals: 0,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
attrs, vals := getExpressionPatterns(tc.expression)
assert.Len(t, attrs, tc.wantAttrs)
assert.Len(t, vals, tc.wantVals)
})
}
}

func Test_mapAttributeFlag(t *testing.T) {
tests := map[string]struct {
flag string
wantErr bool
}{
"valid JSON": {
flag: `{"#name":"name"}`,
},
"invalid JSON": {
flag: `not json`,
wantErr: true,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
result, err := mapAttributeFlag(tc.flag)
if tc.wantErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.NotNil(t, result)
}
})
}
}
Comment on lines +495 to +519
Copy link
Copy Markdown
Member

@zimeg zimeg Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 thought: Unit tests matching each function is a kind pattern since IMHO it also encourages tests at the cmd level is preferred?

Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think we could actually reduce our overall tests and have the same coverage by testing through the command level. We've had success with some of the simpler, newer commands and hopefully we can bring that to the original, complex commands!

74 changes: 74 additions & 0 deletions internal/api/app_test.go
View file
Open in desktop
Copy link
Copy Markdown
Member

@zimeg zimeg Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐ praise: More praises for API tests! These are so good!

Original file line number Diff line number Diff line change
Expand Up @@ -519,3 +519,77 @@ func TestClient_DeveloperAppInstall_RequestAppApproval(t *testing.T) {
})
}
}

func Test_Client_GetAppStatus(t *testing.T) {
tests := map[string]struct {
httpResponseJSON string
wantErr bool
errMessage string
}{
"OK result": {
httpResponseJSON: `{"ok":true,"apps":[{"app_id":"A123","status":"installed"}]}`,
},
"Error result": {
httpResponseJSON: `{"ok":false,"error":"invalid_app"}`,
wantErr: true,
errMessage: "invalid_app",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appStatusMethod,
Response: tc.httpResponseJSON,
})
defer teardown()

result, err := c.GetAppStatus(ctx, "token", []string{"A123"}, "T123")
if tc.wantErr {
require.Error(t, err)
require.Contains(t, err.Error(), tc.errMessage)
} else {
require.NoError(t, err)
require.NotNil(t, result)
}
})
}
}

func Test_Client_ConnectionsOpen(t *testing.T) {
Copy link
Copy Markdown
Member

@zimeg zimeg Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧮 quibble(non-blocking): Forgive me for not searching too much up, but if tests have alphabetical order in this file?

👾 ramble: I'd be curious if separating API methods into different files gives us the most confidence toward focused tests. Not something to change now of course, but it'd be curious pattern for ongoing additions!

Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's not alphabetized. The file looks like it started off being alphabetical, but after line 200, it's a free-for-all. Commit e2e0ed5 attempts to move these functions where they should be.

A challenge with alphabetizing is that logically we want to group getters/setters but alphabetically they are separated. Something for us to think about.

Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👾 ramble: I'd be curious if separating API methods into different files gives us the most confidence toward focused tests. Not something to change now of course, but it'd be curious pattern for ongoing additions!

Personally, I prefer small files would be open to a single file for each API method. But Golang tends to prefer fewer, longer files. That's probably why we are taking the current approach.

tests := map[string]struct {
httpResponseJSON string
wantErr bool
errMessage string
expectedURL string
}{
"OK result": {
httpResponseJSON: `{"ok":true,"url":"wss://example.com/ws"}`,
expectedURL: "wss://example.com/ws",
},
"Error result": {
httpResponseJSON: `{"ok":false,"error":"token_revoked"}`,
wantErr: true,
errMessage: "token_revoked",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appConnectionsOpenMethod,
Response: tc.httpResponseJSON,
})
defer teardown()

result, err := c.ConnectionsOpen(ctx, "token")
if tc.wantErr {
require.Error(t, err)
require.Contains(t, err.Error(), tc.errMessage)
} else {
require.NoError(t, err)
require.Equal(t, tc.expectedURL, result.URL)
}
})
}
}
189 changes: 189 additions & 0 deletions internal/api/datastore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,3 +490,192 @@ func TestClient_AppsDatastoreGet(t *testing.T) {
})
}
}

func Test_Client_AppsDatastoreBulkPut(t *testing.T) {
tests := map[string]struct {
request types.AppDatastoreBulkPut
httpResponseJSON string
statusCode int
wantErr bool
errMessage string
}{
"success": {
request: types.AppDatastoreBulkPut{
Datastore: "my_ds",
App: "A1",
Items: []map[string]interface{}{{"id": "1", "name": "test"}},
},
httpResponseJSON: `{"ok":true,"datastore":"my_ds"}`,
},
"api_error": {
request: types.AppDatastoreBulkPut{
Datastore: "my_ds",
App: "A1",
Items: []map[string]interface{}{{"id": "1"}},
},
httpResponseJSON: `{"ok":false,"error":"datastore_error"}`,
wantErr: true,
errMessage: "datastore_error",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appDatastoreBulkPutMethod,
Response: tc.httpResponseJSON,
StatusCode: tc.statusCode,
})
defer teardown()
_, err := c.AppsDatastoreBulkPut(ctx, "token", tc.request)
if tc.wantErr {
require.Error(t, err)
require.Contains(t, err.Error(), tc.errMessage)
} else {
require.NoError(t, err)
}
})
}
}

func Test_Client_AppsDatastoreCount(t *testing.T) {
tests := map[string]struct {
request types.AppDatastoreCount
httpResponseJSON string
statusCode int
wantCount int
wantErr bool
errMessage string
}{
"success": {
request: types.AppDatastoreCount{
Datastore: "my_ds",
App: "A1",
},
httpResponseJSON: `{"ok":true,"datastore":"my_ds","count":42}`,
wantCount: 42,
},
"api_error": {
request: types.AppDatastoreCount{
Datastore: "my_ds",
App: "A1",
},
httpResponseJSON: `{"ok":false,"error":"datastore_error"}`,
wantErr: true,
errMessage: "datastore_error",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appDatastoreCountMethod,
Response: tc.httpResponseJSON,
StatusCode: tc.statusCode,
})
defer teardown()
got, err := c.AppsDatastoreCount(ctx, "token", tc.request)
if tc.wantErr {
require.Error(t, err)
require.Contains(t, err.Error(), tc.errMessage)
} else {
require.NoError(t, err)
require.Equal(t, tc.wantCount, got.Count)
}
})
}
}

func Test_Client_AppsDatastoreBulkDelete(t *testing.T) {
tests := map[string]struct {
request types.AppDatastoreBulkDelete
httpResponseJSON string
statusCode int
wantErr bool
errMessage string
}{
"success": {
request: types.AppDatastoreBulkDelete{
Datastore: "my_ds",
App: "A1",
IDs: []string{"id1", "id2"},
},
httpResponseJSON: `{"ok":true}`,
},
"api_error": {
request: types.AppDatastoreBulkDelete{
Datastore: "my_ds",
App: "A1",
IDs: []string{"id1"},
},
httpResponseJSON: `{"ok":false,"error":"not_found"}`,
wantErr: true,
errMessage: "not_found",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appDatastoreBulkDeleteMethod,
Response: tc.httpResponseJSON,
StatusCode: tc.statusCode,
})
defer teardown()
_, err := c.AppsDatastoreBulkDelete(ctx, "token", tc.request)
if tc.wantErr {
require.Error(t, err)
require.Contains(t, err.Error(), tc.errMessage)
} else {
require.NoError(t, err)
}
})
}
}

func Test_Client_AppsDatastoreBulkGet(t *testing.T) {
tests := map[string]struct {
request types.AppDatastoreBulkGet
httpResponseJSON string
statusCode int
wantErr bool
errMessage string
}{
"success": {
request: types.AppDatastoreBulkGet{
Datastore: "my_ds",
App: "A1",
IDs: []string{"id1", "id2"},
},
httpResponseJSON: `{"ok":true,"datastore":"my_ds","items":[{"id":"id1","name":"test"}]}`,
},
"api_error": {
request: types.AppDatastoreBulkGet{
Datastore: "my_ds",
App: "A1",
IDs: []string{"id1"},
},
httpResponseJSON: `{"ok":false,"error":"not_found"}`,
wantErr: true,
errMessage: "not_found",
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appDatastoreBulkGetMethod,
Response: tc.httpResponseJSON,
StatusCode: tc.statusCode,
})
defer teardown()
_, err := c.AppsDatastoreBulkGet(ctx, "token", tc.request)
if tc.wantErr {
require.Error(t, err)
require.Contains(t, err.Error(), tc.errMessage)
} else {
require.NoError(t, err)
}
})
}
}
38 changes: 38 additions & 0 deletions internal/config/flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,44 @@ import (
"github.com/stretchr/testify/assert"
)

func Test_SetFlags(t *testing.T) {
fs := slackdeps.NewFsMock()
os := slackdeps.NewOsMock()
config := NewConfig(fs, os)
cmd := &cobra.Command{}
cmd.Flags().String("test-flag", "default", "a test flag")

config.SetFlags(cmd)
assert.NotNil(t, config.Flags)
f := config.Flags.Lookup("test-flag")
assert.NotNil(t, f)
assert.Equal(t, "default", f.DefValue)
}

func Test_InitializeGlobalFlags(t *testing.T) {
fs := slackdeps.NewFsMock()
os := slackdeps.NewOsMock()
config := NewConfig(fs, os)
cmd := &cobra.Command{}

config.InitializeGlobalFlags(cmd)

// Verify that key persistent flags were registered
flagNames := []string{
"apihost", "app", "config-dir", "experiment",
"force", "no-color", "skip-update", "slackdev",
"runtime", "team", "token", "verbose",
}
Copy link
Copy Markdown
Member

@zimeg zimeg Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧮 quibble: Separating these to newlines might be nice for fast reading. I assumed this was a map at first! Perhaps this can be a table test?

tests := map[string]struct{
    shorthand string
    longform  string
}{
    "apihost": {
        longform: "apihost",
    },
    "app": {
        longform:  "app",
        shortform: "a",
    },
}

IIRC? Forgive me if shortform is too much. I'm hopeful that we'll soon revisit some of these options like app and team too!

Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zimeg Great suggestion, I think a list with explicit variables for short and hidden make these tests more readable. Commit c4dbb36

for _, name := range flagNames {
f := cmd.PersistentFlags().Lookup(name)
assert.NotNil(t, f, "flag %s should be registered", name)
}

// Verify hidden flags
assert.True(t, cmd.PersistentFlags().Lookup("apihost").Hidden)
assert.True(t, cmd.PersistentFlags().Lookup("slackdev").Hidden)
Copy link
Copy Markdown
Member Author

@mwbrooks mwbrooks Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I went back-and-forth on whether to have this test, but it's nice to have a test that locks-in and confirms that our expected global flags and hidden flags haven't changed without intention.

}

func TestDeprecatedFlagSubstitutions(t *testing.T) {
tests := map[string]struct {
expectedWarnings []string
Expand Down
Loading
Loading