test: fix tests to run in airplane mode (offline) #102

mwbrooks · 2025-05-21T03:38:22Z

Summary

This pull request updates our tests to not require a network connection. Now our tests run ⚡ slightly faster and 🔌 work offline.

A note about the HTTPClient:

The HTTPClientOptions defines a wide range of options available.
However, only a subset are implemented. For example, retry logic is not implement
This is because the implementation was borrowed from package api.
I decided to scope this PR to moving existing code around without adding new logic.
The hope is that this will allow us to use it across the code base in a less risky way.
Then we can improve the HTTPClient with the unsupported options.

A few opportunities for follow-up pull requests:

URLChecker(...) should be updated to use httpClient.Head instead of httpClient.Get
- This will improve the speed of the URLChecker(...) and prevent a double-download of a template from happening
- ✋🏻 I can do this after we merge this PR
generateGitZipFileURL(...) should be updated to call URLChecker(...) for custom branches as well
- This will improve the error message when a custom branch doesn't exist
- ✋🏻 I can do this after we merge this PR
package update should be updated to use the new HTTPClient interface
- The HTTPClient interface was borrowed from the package api implementation
- We should update both package api, package update, and package project to use the new HTTPClient interface
- ✋🏻 I can do this after we merge this PR
Use HTTPClient interface in other areas of the code base that use a custom http.Client
- De-dup implementation and use one HTTPClient implementation throughout the code base

Reviewers

Test Offline Mode

# Turn off your network
$ make test | grep FAIL: 
# → Confirm no failures (no output)

Test Updates to `generateGitZipFileURL` and ``URLChecker`

# Create a regular project
$ lack create asdf/
# → Choose any project
# → Confirm no errors
$ rm -rf asdf/

# Create from a template
$ lack create asdf -t slack-samples/bolt-js-starter-template
# → Confirm no errors
$ rm -rf asdf/

# Create from a template and custom branch
$ lack create asdf -t slack-samples/bolt-js-starter-template --branch zimeg-feat-logger
# → Confirm no errors
$ cat asdf/package.json
# → Confirm "@slack/bolt": "^3.19.0",
# → Confirm "@slack/logger": "^4.0.0",
$ rm -rf asdf/

# Error when create from a template with non-existent branch
$ lack create asdf -t slack-samples/bolt-js-starter-template --branch master
# → Confirm error code "git_clone_error"
$ rm -rf asdf/

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.

mwbrooks

Sharing a few learnings and decisions that I made along this flight path ✈️

mwbrooks · 2025-05-21T03:39:57Z

internal/deputil/url.go

+
+// URLChecker returns url if its status code is 200, otherwise returns empty string
+func URLChecker(httpClient slackdeps.HTTPClient, url string) string {
+	resp, err := httpClient.Get(url)


note: A future PR should update this to .Head. This will require adding Head to our HTTPClient interface as well.

@mwbrooks Great thinking! For templates that vendor dependencies I imagine this being an expensive operation and I fear rate limits might've been blocking downloads in CI sometimes as well 😓

It's so good to know of this follow up! 🧠 ✨

I fear rate limits might've been blocking downloads in CI sometimes as well 😓

🧠 Hopefully this reduces some flakiness!

mwbrooks · 2025-05-21T03:41:13Z

internal/deputil/url_test.go

 func Test_URLChecker(t *testing.T) {
-	url := URLChecker("https://github.com/slack-samples/deno-starter-template")
-	assert.Equal(t, "https://github.com/slack-samples/deno-starter-template", url, "should return url when url is valid")
+	tests := map[string]struct {


note: Reimplemented the tests as a table test and extended the cover. This function now has 100% test coverage. 💯

mwbrooks · 2025-05-21T03:43:27Z

internal/pkg/create/create.go

+			httpClient := slackdeps.NewHTTPClient(slackdeps.HTTPClientOptions{})
+			zipFileURL := generateGitZipFileURL(httpClient, template.path, gitBranch)


note: This is the main code change. We now inject a httpClient and mock it in tests.

We could take this further by defining a clients.HTTPClient instance, but I wanted to keep these changes scoped smaller to allow us to discuss those trade-offs later.

@mwbrooks So disciplined but a solid call for now!

I think it makes sense to use a shared http client that might allow specific options in calls, but I agree that discussion can wait.

FWIW this will be so fast to find as reference I hope 🔍 ✨

mwbrooks · 2025-05-21T03:44:18Z

internal/pkg/create/create.go

+			zipURL = deputil.URLChecker(httpClient, masterURL)
 		}
 	} else {
 		zipURL = zipURL + gitBranch + ".zip"


note: Kinda funny that we never apply URLChecker to custom branches. Work for the future!

@mwbrooks I was wondering of possible reason for this, but the points around how we can improve error messages for missing URLs outweigh the missing checks here I hope 🪄

mwbrooks · 2025-05-21T03:44:34Z

internal/pkg/create/create_test.go

-
-	url = generateGitZipFileURL("https://github.com/slack-samples/deno-starter-template", "")
-	assert.Equal(t, "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/main.zip", url, "should return zip download link with main")
+	tests := map[string]struct {


note: Reimplemented the tests as a table test and extended the cover. This function now has 100% test coverage. 💯

@mwbrooks Huge unlock for test stabilities all around here 🔏 ✨

While it'd be so surprising to me, I can imagine missing branches might cause some of the past tests to fail 😉

mwbrooks · 2025-05-21T03:45:24Z

internal/slackdeps/http.go

+// HTTPClient interface for http.Client.
+type HTTPClient interface {
+	Do(req *http.Request) (*http.Response, error)
+	Get(url string) (*http.Response, error)
+}


note: I only added what we use today, but this will need to extend to Post and Head as we use it in other areas of the code base.

mwbrooks · 2025-05-21T03:47:02Z

internal/slackdeps/http.go

+// NewHTTPClient returns an http.Client configured with HTTPClientOptions.
+func NewHTTPClient(opts HTTPClientOptions) *http.Client {


note: This is taken from internal/api/httpclient.go so it's been well used and tested. I only updated the comments and style for declaring the pointer client := &http.Client{}.

I'm excited for the future refactors that bring the http logic out of api 🤖 ✨

Same same same same same!

mwbrooks · 2025-05-21T03:47:40Z

internal/slackdeps/http_mock.go

+// MockHTTPResponse is a helper that returns a mocked http.Response with the provided httpStatus and body.
+func MockHTTPResponse(httpStatus int, body string) *http.Response {


note: This little helper makes me really happy 😄

"net/http/httptest"

This is such an impressive package IMO! I'm glad we're making tests at the right level here 🎉

mwbrooks · 2025-05-21T03:49:04Z

internal/slackdeps/http_mock.go

+	resWriter := httptest.NewRecorder()
+
+	resWriter.WriteHeader(httpStatus)
+	_, _ = io.WriteString(resWriter, body)


tip: After some searching, I learned that fmt.Printf(resWriter, body) and io.WriteString(resWriter, body) are both acceptable. WriteString is preferred because it's more readable and some packages such as http have optimized implementations of it.

Right now, we use both throughout our code base.

codecov · 2025-05-21T03:56:34Z

Codecov Report

Attention: Patch coverage is 93.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.42%. Comparing base (56d8c20) to head (96adc92).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/create/create.go	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   63.37%   63.42%   +0.04%     
==========================================
  Files         211      212       +1     
  Lines       22284    22308      +24     
==========================================
+ Hits        14122    14148      +26     
  Misses       7071     7071              
+ Partials     1091     1089       -2

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

zimeg

@mwbrooks Hacking in the cloud can callout such interesting problems! 🤖 ☁️

All of these changes move the code to such a good place for enhanced tests and all related improvements to follow. I'm glad to find tests running a bit faster and more stable too, and the cases provided are working well during review at this moment 😉

zimeg · 2025-05-23T03:47:25Z

internal/deputil/url.go

+
+// URLChecker returns url if its status code is 200, otherwise returns empty string
+func URLChecker(httpClient slackdeps.HTTPClient, url string) string {
+	resp, err := httpClient.Get(url)


@mwbrooks Great thinking! For templates that vendor dependencies I imagine this being an expensive operation and I fear rate limits might've been blocking downloads in CI sometimes as well 😓

It's so good to know of this follow up! 🧠 ✨

zimeg · 2025-05-23T03:49:35Z

internal/deputil/url_test.go

+			// Create mocks
+			httpClientMock := &slackdeps.HTTPClientMock{}
+			tt.setupHTTPClientMock(httpClientMock)

-	url = URLChecker("fake_url")
-	assert.Equal(t, "", url, "should return empty string when url is invalid")
+			// Execute
+			url := URLChecker(httpClientMock, tt.url)


The table tests, mock setup, and injected dependencies are so good to see here! 🤩

The trio dream team!

zimeg · 2025-05-23T03:51:33Z

internal/deputil/url.go

-// URLChecker returns url if it's status code is 200, otherwise returns empty string
-func URLChecker(url string) string {
-	resp, err := http.Get(url)
+	"github.com/slackapi/slack-cli/internal/slackdeps"


📝 No blocker but I can imagine we move custom HTTP setups - including this file - and interface implementations into a separate internal package?

Suggested change

"github.com/slackapi/slack-cli/internal/slackdeps"

"github.com/slackapi/slack-cli/internal/slackhttp"

I was thinking the same thing. The use of *util or *deps usually smells of opportunity for improvement. This seems like a good step forward.

zimeg · 2025-05-23T03:55:28Z

internal/deputil/url.go

+	"github.com/slackapi/slack-cli/internal/slackdeps"
+)
+
+// URLChecker returns url if its status code is 200, otherwise returns empty string


🔍 I'm not sure if this was included in the followup plans, but we might also want to return an error here for continued improvements to error messages?

It's not clear to me if .Get returns separate errors for the specific error cases:

Internet broke

URL is missing

I had the same thought but let it slide. I agree, let's follow-up with returning an error from URLChecker 👍🏻

zimeg · 2025-05-23T03:58:08Z

internal/pkg/create/create.go

+			httpClient := slackdeps.NewHTTPClient(slackdeps.HTTPClientOptions{})
+			zipFileURL := generateGitZipFileURL(httpClient, template.path, gitBranch)


@mwbrooks So disciplined but a solid call for now!

I think it makes sense to use a shared http client that might allow specific options in calls, but I agree that discussion can wait.

FWIW this will be so fast to find as reference I hope 🔍 ✨

zimeg · 2025-05-23T04:02:23Z

internal/pkg/create/create_test.go

-
-	url = generateGitZipFileURL("https://github.com/slack-samples/deno-starter-template", "")
-	assert.Equal(t, "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/main.zip", url, "should return zip download link with main")
+	tests := map[string]struct {


@mwbrooks Huge unlock for test stabilities all around here 🔏 ✨

While it'd be so surprising to me, I can imagine missing branches might cause some of the past tests to fail 😉

zimeg · 2025-05-23T04:03:08Z

internal/pkg/create/create_test.go

+				res := slackdeps.MockHTTPResponse(http.StatusOK, "OK")
+				httpClientMock.On("Get", "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/main.zip").Return(nil, fmt.Errorf("HttpClient error"))
+				httpClientMock.On("Get", "https://github.com/slack-samples/deno-starter-template/archive/refs/heads/master.zip").Return(res, nil)


zimeg · 2025-05-23T04:05:27Z

internal/slackdeps/http.go

+// NewHTTPClient returns an http.Client configured with HTTPClientOptions.
+func NewHTTPClient(opts HTTPClientOptions) *http.Client {


I'm excited for the future refactors that bring the http logic out of api 🤖 ✨

zimeg · 2025-05-23T04:07:57Z

internal/slackdeps/http_mock.go

+// MockHTTPResponse is a helper that returns a mocked http.Response with the provided httpStatus and body.
+func MockHTTPResponse(httpStatus int, body string) *http.Response {


"net/http/httptest"

This is such an impressive package IMO! I'm glad we're making tests at the right level here 🎉

zimeg · 2025-05-23T04:09:09Z

internal/slackdeps/http.go

📝 Edit to an above comment!

I didn't realize these files were new, but we might take this PR as a chance to start a separate package:

internal/slackhttp/http.go

Good point! I was going to hold off for a new PR, but since these files are new, we might as well do it now. I'll start the refactor now...

Commit 96adc92 refactors this PR to use a separate packaged:

internal/slackhttp/http.go internal/slackhttp/http_mock.go internal/slackhttp/http_test.go

mwbrooks · 2025-05-27T00:16:39Z

Thanks for the great review @zimeg! 🙇🏻 And for suggesting that we refactor the package slackhttp now rather than later 👌🏻 This PR is looking good, so I'm going to merge it now and follow-up with a few of the additional tasks.

test: fix tests to run offline without a network connection

f9b6f0a

mwbrooks added this to the Next Release milestone May 21, 2025

mwbrooks self-assigned this May 21, 2025

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 May 21, 2025

mwbrooks commented May 21, 2025

View reviewed changes

mwbrooks added 2 commits May 20, 2025 20:51

Merge branch 'main' into mwbrooks-http-mock

53b143a

refactor: serves me right for resolving conflicts in the GitHub Editor

b72d1dd

mwbrooks added 2 commits May 21, 2025 06:24

test: add tests for HTTPClient

51a55a1

Merge branch 'main' into mwbrooks-http-mock

df932c0

mwbrooks marked this pull request as ready for review May 21, 2025 17:55

mwbrooks requested a review from a team as a code owner May 21, 2025 17:55

zimeg approved these changes May 23, 2025

View reviewed changes

mwbrooks added 2 commits May 26, 2025 16:48

Merge branch 'main' into mwbrooks-http-mock

33d628f

refactor: slackdeps/http.go -> slackhttp/http.go

96adc92

mwbrooks merged commit 4406cb0 into main May 27, 2025
6 checks passed

mwbrooks deleted the mwbrooks-http-mock branch May 27, 2025 00:16

		httpClient := slackdeps.NewHTTPClient(slackdeps.HTTPClientOptions{})
		zipFileURL := generateGitZipFileURL(httpClient, template.path, gitBranch)

		// NewHTTPClient returns an http.Client configured with HTTPClientOptions.
		func NewHTTPClient(opts HTTPClientOptions) *http.Client {

		// MockHTTPResponse is a helper that returns a mocked http.Response with the provided httpStatus and body.
		func MockHTTPResponse(httpStatus int, body string) *http.Response {

	"github.com/slackapi/slack-cli/internal/slackdeps"
	"github.com/slackapi/slack-cli/internal/slackhttp"

test: fix tests to run in airplane mode (offline) #102

test: fix tests to run in airplane mode (offline) #102

Uh oh!

Conversation

mwbrooks commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Reviewers

Test Offline Mode

Test Updates to generateGitZipFileURL and ``URLChecker`

Requirements

Uh oh!

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

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!

codecov bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented May 21, 2025 •

edited

Loading

Test Updates to `generateGitZipFileURL` and ``URLChecker`

codecov bot commented May 21, 2025 •

edited

Loading