Make changes to support testing individual commands, add a few tests #15

cheshire137 · 2024-10-11T17:18:18Z

For https://github.com/github/models/issues/330. This adds a few basic tests covering the happy path for the view, run, and list commands. It also adds a new Actions workflow for running tests.

To support testing without hitting the actual Azure APIs, I added a new command.Config type to bundle together an Azure client and output streams. I renamed azuremodels.Client to azuremodels.AzureClient and made a new azuremodels.Client that's an interface. That let me add an azuremodels.MockClient type for use in tests, allowing us to stub out functions like ListModels to avoid making real HTTP requests.

Current test output:

	github.com/github/gh-models		coverage: 0.0% of statements
	github.com/github/gh-models/cmd		coverage: 0.0% of statements
ok  	github.com/github/gh-models/cmd/list	1.014s	coverage: 17.4% of statements
ok  	github.com/github/gh-models/cmd/run	1.025s	coverage: 32.5% of statements
	github.com/github/gh-models/internal/azuremodels		coverage: 0.0% of statements
	github.com/github/gh-models/internal/sse		coverage: 0.0% of statements
	github.com/github/gh-models/internal/ux		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/cmd/view	1.101s	coverage: 26.9% of statements

Trying to make it an injected thing for easier testing.

https://docs.github.com/en/actions/use-cases-and-examples/building-and-testing/building-and-testing-go

synchronize = pushed to head, see https://docs.github.com/webhooks/webhook-events-and-payloads?actionType=synchronize#pull_request

cheshire137 · 2024-10-11T19:01:51Z

.github/workflows/lint.yml

@@ -2,17 +2,25 @@ name: "go-linter"

 on:
  pull_request:
+    types: [opened, synchronize, reopened]


Run the linter on fewer PR events, such as when the PR is labelled.

cheshire137 · 2024-10-11T19:02:02Z

.github/workflows/lint.yml

  merge_group:
  workflow_dispatch:
+  push:
+    branches:
+      - 'main'


Also lint what gets merged into the main branch.

cheshire137 · 2024-10-11T19:02:16Z

.github/workflows/lint.yml


 permissions:
  contents: read

+concurrency:
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
+  cancel-in-progress: true


Cancel an existing run of the linter for the same commit.

cheshire137 · 2024-10-11T19:02:26Z

.github/workflows/lint.yml

 jobs:
  lint:
    strategy:
      fail-fast: false
-    runs-on: ubuntu-latest-xl
+    runs-on: ubuntu-latest


Probably don't need the XL version.

Shorter lines, a little less reaching through.

cheshire137 · 2024-10-11T19:15:03Z

cmd/root.go

+	token, _ := auth.TokenForHost("github.com")
+	if token == "" {
+		util.WriteToOut(out, "No GitHub token found. Please run 'gh auth login' to authenticate.\n")
+		return nil
+	}


Each of the commands (run, view, list) was doing this, so I pulled out to this earlier point to DRY things up. It also lets me test the individual commands by passing in a mock client.

Does moving this affect the ability to list commands, get help from the cli unless you're signed in?

It did indeed affect things! Fixed with 1b48b0f. Now when I'm not authenticated and I try to run a command, I get output like:

% ./gh-models view gpt-4o-mini No GitHub token found. Please run 'gh auth login' to authenticate. Error: not authenticated Usage: gh models view [model] [flags] Flags: -h, --help help for view

If I just try to check usage info, I get output like:

% ./gh-models list --help No GitHub token found. Please run 'gh auth login' to authenticate. List available models Usage: gh models list [flags] Flags: -h, --help help for list

Less reaching-through at call sites.

cheshire137 · 2024-10-11T19:19:22Z

cmd/view/model_printer.go

 }

 func (p *modelPrinter) render() error {
 	modelSummary := p.modelSummary
 	if modelSummary != nil {
 		p.printLabelledLine("Display name:", modelSummary.FriendlyName)
-		p.printLabelledLine("Summary name:", modelSummary.Name)
+		p.printLabelledLine("Model name:", modelSummary.Name)


Writing the 'view' command test showed me this in the output produced, making me realize we were labelling this field wrong. Yay for tests! 🎉

cheshire137 · 2024-10-11T19:20:51Z

internal/azuremodels/types.go

@@ -36,7 +36,8 @@ type ChatCompletionOptions struct {
 	TopP        *float64      `json:"top_p,omitempty"`
 }

-type chatChoiceMessage struct {
+// ChatChoiceMessage is a message from a choice in a chat conversation.
+type ChatChoiceMessage struct {


Made public for use in tests.

cschleiden · 2024-10-11T19:46:54Z

.github/workflows/test.yml

+      - name: Configure Go private module access
+        run: |
+          echo "machine goproxy.githubapp.com login nobody password ${{ secrets.GOPROXY_TOKEN }}" >> $HOME/.netrc


could we use - uses: github/[email protected]?

Quite possibly! Let me try that out, this was copy-pasta from another github-org Go app.

Dropped from both our test and lint workflows in 3ede78e, we'll see if anything goes awry.

Nope, both builds are fine! 🎉

cschleiden · 2024-10-11T19:49:19Z

cmd/root.go

+	token, _ := auth.TokenForHost("github.com")
+	if token == "" {
+		util.WriteToOut(out, "No GitHub token found. Please run 'gh auth login' to authenticate.\n")
+		return nil
+	}


Does moving this affect the ability to list commands, get help from the cli unless you're signed in?

cschleiden · 2024-10-11T19:52:43Z

internal/azuremodels/client.go

+	// GetChatCompletionStream returns a stream of chat completions using the given options.
+	GetChatCompletionStream(context.Context, ChatCompletionOptions) (*ChatCompletionResponse, error)
+	// GetModelDetails returns the details of the specified model in a particular registry.
+	GetModelDetails(context.Context, string, string, string) (*ModelDetails, error)


nit, use names for the parameters to understand which 3 strings this takes?

Good idea! Fixed in 09cd4ae.

Related: #15 (comment)

e.g., ./gh-models run gpt-4o-mini "hello" No GitHub token found. Please run 'gh auth login' to authenticate. panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xba48c6a] goroutine 1 [running]: github.com/github/gh-models/internal/azuremodels.(*AzureClient).ListModels(0x0, {0xd098300, 0xd5ca4a0}) gh-models/internal/azuremodels/azure_client.go:194 +0x20a github.com/github/gh-models/cmd/run.(*runCommandHandler).loadModels(0x4?) gh-models/cmd/run/run.go:382 +0x2a github.com/github/gh-models/cmd/run.NewRunCommand.func1(0xc00021a308, {0xc0002d82e0, 0x2, 0x2}) gh-models/cmd/run/run.go:203 +0x139 github.com/spf13/cobra.(*Command).execute(0xc00021a308, {0xc0002d82a0, 0x2, 0x2}) go/pkg/mod/github.com/spf13/[email protected]/command.go:985 +0xaca github.com/spf13/cobra.(*Command).ExecuteC(0xc0001fd808) go/pkg/mod/github.com/spf13/[email protected]/command.go:1117 +0x3ff github.com/spf13/cobra.(*Command).ExecuteContextC(0xb6c5305?, {0xd098300?, 0xd5ca4a0?}) go/pkg/mod/github.com/spf13/[email protected]/command.go:1050 +0x47 main.mainRun() gh-models/main.go:29 +0x26 main.main() gh-models/main.go:19 +0x13

https: //github.com//pull/15#discussion_r1797368843 Co-Authored-By: Christopher Schleiden <[email protected]>

sgoedecke

Thank you for refactoring this to make it testable. One nit around changed token validation behavior, not a showstopper.

sgoedecke · 2024-10-13T21:41:43Z

cmd/root.go

+
+	var client azuremodels.Client
+
+	if token == "" {


For gh models run, previously the code would have immediately bailed out of the command if you had no token set, but now it looks like it's going to try and keep going with a "" token (and eventually run into some 403 error).

I really like that this PR still lets you do some commands without a token, but do you think it's worth trying to preserve the old behavior for run (printing a human-readable error and immediately exiting)?

do you think it's worth trying to preserve the old behavior for run (printing a human-readable error and immediately exiting)?

So the effect feels the same to users. When I try to run as an unauthenticated user right now on this branch, here's what I get:

% ./gh-models run gpt-4o-mini "hello world" No GitHub token found. Please run 'gh auth login' to authenticate. Error: not authenticated Usage: gh models run [model] [prompt] [flags] Flags: -h, --help help for run --max-tokens string Limit the maximum tokens for the model response. --system-prompt string Prompt the system. --temperature string Controls randomness in the response, use lower to be more deterministic. --top-p string Controls text diversity by selecting the most probable words until a set probability is reached.

Are you worried about us trying to make an HTTP call with an invalid (blank) token?

I don't really mind (actually making the HTTP call is not a problem), I was just a little worried that the error would be ugly. Given that it isn't, I have no qualms here - no need to follow up in another branch.

cheshire137 · 2024-10-14T15:35:43Z

I landed the PR so we can start having some test coverage and help avoid regressions/breakages, Sean, but reply back above regarding the blank token and I can iterate further in a follow-up branch!

cheshire137 added 9 commits October 11, 2024 11:11

Pull client earlier, pass to each command

57b9ca1

Trying to make it an injected thing for easier testing.

Rename Client to AzureClient

dd20108

Add Client interface

c1bb492

Add command.Config type

ef468eb

Docs

ec0883b

Add MockClient for testing

8076365

Add a basic test for the list command

0e91a6e

Probably don't need XL for lint

3122947

Add test workflow

6781863

https://docs.github.com/en/actions/use-cases-and-examples/building-and-testing/building-and-testing-go

cheshire137 self-assigned this Oct 11, 2024

cheshire137 added 12 commits October 11, 2024 12:19

Cancel existing runs when starting a new one

f247ca2

Don't need dual push + PR triggers

f1d30e0

Linter fixes

644fb0d

Add NewConfigWithTerminal

d236e8f

Fix label for model name in view output

9a307d6

Add basic test for view command

5d95d27

Return 'not implemented' error by default for mocks

13dc795

Export ChatChoiceMessage type from azuremodels

57b6183

Add basic run command test

a804f54

Don't need to rerun CI if a label changes, for example

bd59113

synchronize = pushed to head, see https://docs.github.com/webhooks/webhook-events-and-payloads?actionType=synchronize#pull_request

Run CI when pushing to main branch

f93fc5e

Output code coverage percentages

64bd851

cheshire137 commented Oct 11, 2024

View reviewed changes

cheshire137 added 3 commits October 11, 2024 14:08

Add NewTablePrinter

d69e1ae

Add Config WriteToOut

3738e1f

Add runCommandHandler writeToOut

099849a

Shorter lines, a little less reaching through.

cheshire137 commented Oct 11, 2024

View reviewed changes

Include client prop on runCommandHandler

0c00848

Less reaching-through at call sites.

cheshire137 commented Oct 11, 2024

View reviewed changes

cheshire137 marked this pull request as ready for review October 11, 2024 19:21

cheshire137 requested a review from a team as a code owner October 11, 2024 19:21

cheshire137 changed the title ~~Make changes to support testing individual commands, and add a test~~ Make changes to support testing individual commands, add a few tests Oct 11, 2024

cheshire137 requested a review from garman October 11, 2024 19:34

cschleiden reviewed Oct 11, 2024

View reviewed changes

cheshire137 and others added 3 commits October 11, 2024 15:30

Do we need private module proxy stuff?

3ede78e

Related: #15 (comment)

List param names

09cd4ae

https: //github.com//pull/15#discussion_r1797368843 Co-Authored-By: Christopher Schleiden <[email protected]>

sgoedecke approved these changes Oct 13, 2024

View reviewed changes

Merge branch 'main' into add-a-test

a746559

cheshire137 merged commit 27157bc into main Oct 14, 2024
5 checks passed

cheshire137 deleted the add-a-test branch October 14, 2024 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make changes to support testing individual commands, add a few tests #15

Make changes to support testing individual commands, add a few tests #15

cheshire137 commented Oct 11, 2024 •

edited

Loading

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cschleiden Oct 11, 2024

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cschleiden Oct 11, 2024

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cheshire137 Oct 11, 2024

cschleiden Oct 11, 2024

cschleiden Oct 11, 2024

cheshire137 Oct 11, 2024

sgoedecke left a comment

sgoedecke Oct 13, 2024

cheshire137 Oct 14, 2024 •

edited

Loading

sgoedecke Oct 14, 2024

cheshire137 commented Oct 14, 2024

Make changes to support testing individual commands, add a few tests #15

Make changes to support testing individual commands, add a few tests #15

Conversation

cheshire137 commented Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgoedecke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheshire137 Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheshire137 commented Oct 14, 2024

cheshire137 commented Oct 11, 2024 •

edited

Loading

cheshire137 Oct 14, 2024 •

edited

Loading