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 1 commit
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!

48 changes: 48 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,51 @@ func TestClient_DeveloperAppInstall_RequestAppApproval(t *testing.T) {
})
}
}

func TestClient_GetAppStatus_Ok(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appStatusMethod,
Response: `{"ok":true,"apps":[{"app_id":"A123","status":"installed"}]}`,
})
defer teardown()
result, err := c.GetAppStatus(ctx, "token", []string{"A123"}, "T123")
require.NoError(t, err)
require.NotNil(t, result)
}

func TestClient_GetAppStatus_Error(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appStatusMethod,
Response: `{"ok":false,"error":"invalid_app"}`,
})
defer teardown()
_, err := c.GetAppStatus(ctx, "token", []string{"A123"}, "T123")
require.Error(t, err)
require.Contains(t, err.Error(), "invalid_app")
}

func TestClient_ConnectionsOpen_Ok(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appConnectionsOpenMethod,
Response: `{"ok":true,"url":"wss://example.com/ws"}`,
})
defer teardown()
result, err := c.ConnectionsOpen(ctx, "token")
require.NoError(t, err)
require.Equal(t, "wss://example.com/ws", result.URL)
}

func TestClient_ConnectionsOpen_Error(t *testing.T) {
ctx := slackcontext.MockContext(t.Context())
c, teardown := NewFakeClient(t, FakeClientParams{
ExpectedMethod: appConnectionsOpenMethod,
Response: `{"ok":false,"error":"token_revoked"}`,
})
defer teardown()
_, err := c.ConnectionsOpen(ctx, "token")
require.Error(t, err)
require.Contains(t, err.Error(), "token_revoked")
}
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 TestClient_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 TestClient_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 TestClient_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 TestClient_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 TestSetFlags(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 TestInitializeGlobalFlags(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
24 changes: 24 additions & 0 deletions internal/deputil/url_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.

📠 thought: A slackhttp package might be easier to find this ongoing? TIL about these modules 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.

🧠 Makes sense that we can merge these two eventually! I'll add a note.

Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,30 @@ func Test_URLChecker(t *testing.T) {
httpClientMock.On("Head", mock.Anything).Return(nil, fmt.Errorf("HTTPClient error"))
},
},
"Returns an empty string for HTTP 500 Internal Server Error": {
url: "https://example.com/server-error",
expectedURL: "",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
res := slackhttp.MockHTTPResponse(http.StatusInternalServerError, "Internal Server Error")
httpClientMock.On("Head", mock.Anything).Return(res, nil)
},
},
"Returns an empty string for HTTP 301 redirect": {
url: "https://example.com/redirect",
expectedURL: "",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
res := slackhttp.MockHTTPResponse(http.StatusMovedPermanently, "Moved")
httpClientMock.On("Head", mock.Anything).Return(res, nil)
},
},
"Returns an empty string for HTTP 403 Forbidden": {
url: "https://example.com/forbidden",
expectedURL: "",
setupHTTPClientMock: func(httpClientMock *slackhttp.HTTPClientMock) {
res := slackhttp.MockHTTPResponse(http.StatusForbidden, "Forbidden")
httpClientMock.On("Head", mock.Anything).Return(res, nil)
},
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
Expand Down
Loading