Add more test coverage, stop swallowing client error #18
Conversation
| @@ -28,7 +28,7 @@ jobs: | |||
| - uses: actions/checkout@v4 | |||
| - uses: actions/setup-go@v5 | |||
| with: | |||
| go-version: ${{ vars.GOVERSION }} | |||
| go-version: ">=1.22" | |||
There was a problem hiding this comment.
Specifying the same Go version here as we do in our dev docs.
| @@ -43,4 +43,6 @@ jobs: | |||
| run: go build -v ./... | |||
|
|
|||
| - name: Run tests | |||
| run: go test -race -cover ./... | |||
| run: | | |||
| go version | |||
There was a problem hiding this comment.
Outputting the version for visibility when looking at test results, in case the version affects the results.
There was a problem hiding this comment.
Nice improvement! We could include this in a script/test as well, for consistency with other repos.
| @@ -32,7 +32,12 @@ func NewRootCommand() *cobra.Command { | |||
| util.WriteToOut(out, "No GitHub token found. Please run 'gh auth login' to authenticate.\n") | |||
| client = azuremodels.NewUnauthenticatedClient() | |||
| } else { | |||
| client = azuremodels.NewAzureClient(token) | |||
| var err error | |||
| client, err = azuremodels.NewDefaultAzureClient(token) | |||
There was a problem hiding this comment.
Putting together an AzureClient requires getting an http.Client, which could error. We're no longer silencing that error if it occurs, which helped me figure out a panic occurring on CI when running tests.
| @@ -98,37 +98,43 @@ func (m *ModelSummary) HasName(name string) bool { | |||
| return strings.EqualFold(m.FriendlyName, name) || strings.EqualFold(m.Name, name) | |||
| } | |||
|
|
|||
| type modelCatalogTextLimits struct { | |||
There was a problem hiding this comment.
I pulled out these nested types for ease of reference in testing.
|
|
||
| // NewAzureClient returns a new Azure client using the given auth token. | ||
| func NewAzureClient(authToken string) *AzureClient { | ||
| httpClient, _ := api.DefaultHTTPClient() |
There was a problem hiding this comment.
This is the error we were swallowing before.
cheshire137
changed the title
cmd/list/list_test.go
Outdated
|
|
||
| require.NoError(t, err) | ||
| require.Contains(t, outBuf.String(), "List available models") | ||
| require.Equal(t, "", errBuf.String()) |
There was a problem hiding this comment.
| require.Equal(t, "", errBuf.String()) | |
| require.Empty(t, errBuf.String()) |
Co-Authored-By: Christopher Schleiden <[email protected]>
|
Forgot to mention, but this is the test coverage with this branch's changes: % go test -race -cover ./...
github.com/github/gh-models coverage: 0.0% of statements
ok github.com/github/gh-models/cmd (cached) coverage: 78.9% of statements
ok github.com/github/gh-models/cmd/list (cached) coverage: 93.1% of statements
ok github.com/github/gh-models/cmd/run (cached) coverage: 47.2% of statements
ok github.com/github/gh-models/cmd/view (cached) coverage: 80.0% of statements
ok github.com/github/gh-models/internal/azuremodels (cached) coverage: 27.5% of statements
github.com/github/gh-models/internal/sse coverage: 0.0% of statements
github.com/github/gh-models/pkg/command coverage: 0.0% of statements
github.com/github/gh-models/pkg/util coverage: 0.0% of statements
ok github.com/github/gh-models/internal/ux (cached) coverage: 72.7% of statements |
This adds tests for the
--helpoutput of commands as well as forazuremodels.AzureClient'sGetModelDetails.To make
azuremodels.AzureClientmore testable, I switched it from using constants for API URLs to instead taking a config object that provides the URLs to use. That lets us in tests start a test server and use that server as a stand-in for third-party APIs.