test: increase test coverage in cmd/ and internal/[api | config | deputil] by mwbrooks · Pull Request #392 · slackapi/slack-cli

mwbrooks · 2026-03-12T04:04:25Z

Changelog

N/A

Summary

This pull request increases our test coverage +0.8% by adding tests to untested functions in:

cmd/
internal/api
internal/config
internal/deputil

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2026-03-12T04:08:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.85%. Comparing base (4d5f768) to head (c4dbb36).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
+ Coverage   67.01%   67.85%   +0.84%     
==========================================
  Files         218      218              
  Lines       18090    18090              
==========================================
+ Hits        12123    12275     +152     
+ Misses       4833     4656     -177     
- Partials     1134     1159      +25

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Consolidate GetAppStatus and ConnectionsOpen tests from separate ok/error functions into table-driven tests using the map pattern.

mwbrooks · 2026-03-12T04:09:25Z

internal/config/flags_test.go

+	// Verify that key persistent flags were registered
+	flagNames := []string{
+		"apihost", "app", "config-dir", "experiment",
+		"force", "no-color", "skip-update", "slackdev",
+		"runtime", "team", "token", "verbose",
+	}
+	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)


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.

zimeg

@mwbrooks Things gets better every day with changes like this. Thanks so much 🔋 ✨

I'm leaving not important thoughts and a few comments related, but please feel free to merge this whenever.

zimeg · 2026-03-12T16:08:23Z

cmd/datastore/query_test.go

+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)
+			}
+		})
+	}
+}


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

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!

zimeg · 2026-03-12T16:11:54Z

internal/api/app_test.go

+	}
+}
+
+func Test_Client_ConnectionsOpen(t *testing.T) {


🧮 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!

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.

👾 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.

zimeg · 2026-03-12T16:12:26Z

internal/api/app_test.go

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

zimeg · 2026-03-12T16:16:10Z

internal/config/flags_test.go

+	flagNames := []string{
+		"apihost", "app", "config-dir", "experiment",
+		"force", "no-color", "skip-update", "slackdev",
+		"runtime", "team", "token", "verbose",
+	}


🧮 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!

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

zimeg · 2026-03-12T16:17:52Z

internal/deputil/url_test.go

📠 thought: A slackhttp package might be easier to find this ongoing? TIL about these modules too-

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

…etails

mwbrooks · 2026-03-12T19:28:59Z

@zimeg Thanks for the thorough review! I'll try to keep things alphabetized and maybe we can find a common standard for how to order getter/settings (e.g. Getters don't include "Get" and Setter always follows the Getter → AppName() and SetAppName()). 🚀 rockets away, onto the next PR!

test: increase test coverage in cmd/ and internal/api|config|deputil

a155ba5

mwbrooks added this to the Next Release milestone Mar 12, 2026

mwbrooks self-assigned this Mar 12, 2026

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Mar 12, 2026

mwbrooks added 2 commits March 11, 2026 21:21

test: rewrite api app tests as table-driven tests

28f1bf2

Consolidate GetAppStatus and ConnectionsOpen tests from separate ok/error functions into table-driven tests using the map pattern.

test: normalize test function names to use Test_ prefix convention

58c9c3f

mwbrooks commented Mar 12, 2026

View reviewed changes

mwbrooks marked this pull request as ready for review March 12, 2026 04:34

mwbrooks requested a review from a team as a code owner March 12, 2026 04:34

zimeg approved these changes Mar 12, 2026

View reviewed changes

mwbrooks added 3 commits March 12, 2026 10:19

Merge branch 'main' into mwbrooks-test-coverage-simple-files

c2f5c7e

test: try to alphabetized app_test.go

e2e0ed5

test: refactor InitializeGlobalFlags to table-driven test with flag d…

c4dbb36

…etails

mwbrooks merged commit 8e4c0aa into main Mar 12, 2026
8 checks passed

mwbrooks deleted the mwbrooks-test-coverage-simple-files branch March 12, 2026 19:29

Conversation

mwbrooks commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Summary

Requirements

Uh oh!

codecov bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwbrooks commented Mar 12, 2026 •

edited

Loading

codecov bot commented Mar 12, 2026 •

edited

Loading