- Notifications
You must be signed in to change notification settings - Fork959
Used typed tool handler forget_me tool#447
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
4 commits Select commitHold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # Testing | ||
| This project uses a combination of unit tests and end-to-end (e2e) tests to ensure correctness and stability. | ||
| ## Unit Testing Patterns | ||
| - Unit tests are located alongside implementation, with filenames ending in `_test.go`. | ||
| - Currently the preference is to use internal tests i.e. test files do not have `_test` package suffix. | ||
| - Tests use [testify](https://github.com/stretchr/testify) for assertions and require statements. Use `require` when continuing the test is not meaningful, for example it is almost never correct to continue after an error expectation. | ||
| - Mocking is performed using [go-github-mock](https://github.com/migueleliasweb/go-github-mock) or `githubv4mock` for simulating GitHub rest and GQL API responses. | ||
| - Each tool's schema is snapshotted and checked for changes using the `toolsnaps` utility (see below). | ||
| - Tests are designed to be explicit and verbose to aid maintainability and clarity. | ||
| - Handler unit tests should take the form of: | ||
| 1. Test tool snapshot | ||
| 1. Very important expectations against the schema (e.g. `ReadOnly` annotation) | ||
| 1. Behavioural tests in table-driven form | ||
| ## End-to-End (e2e) Tests | ||
| - E2E tests are located in the [`e2e/`](../e2e/) directory. See the [e2e/README.md](../e2e/README.md) for full details on running and debugging these tests. | ||
| ## toolsnaps: Tool Schema Snapshots | ||
| - The `toolsnaps` utility ensures that the JSON schema for each tool does not change unexpectedly. | ||
| - Snapshots are stored in `__toolsnaps__/*.snap` files , where `*` represents the name of the tool | ||
| - When running tests, the current tool schema is compared to the snapshot. If there is a difference, the test will fail and show a diff. | ||
| - If you intentionally change a tool's schema, update the snapshots by running tests with the environment variable: `UPDATE_TOOLSNAPS=true go test ./...` | ||
| - In CI (when `GITHUB_ACTIONS=true`), missing snapshots will cause a test failure to ensure snapshots are always | ||
| committed. | ||
| ## Notes | ||
| - Some tools that mutate global state (e.g., marking all notifications as read) are tested primarily with unit tests, not e2e, to avoid side effects. | ||
| - For more on the limitations and philosophy of the e2e suite, see the [e2e/README.md](../e2e/README.md). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| // Package toolsnaps provides test utilities for ensuring json schemas for tools | ||
| // have not changed unexpectedly. | ||
| package toolsnaps | ||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "github.com/josephburnett/jd/v2" | ||
| ) | ||
| // Test checks that the JSON schema for a tool has not changed unexpectedly. | ||
| // It compares the marshaled JSON of the provided tool against a stored snapshot file. | ||
| // If the UPDATE_TOOLSNAPS environment variable is set to "true", it updates the snapshot file instead. | ||
| // If the snapshot does not exist and not running in CI, it creates the snapshot file. | ||
| // If the snapshot does not exist and running in CI (GITHUB_ACTIONS="true"), it returns an error. | ||
| // If the snapshot exists, it compares the tool's JSON to the snapshot and returns an error if they differ. | ||
| // Returns an error if marshaling, reading, or comparing fails. | ||
| func Test(toolName string, tool any) error { | ||
| toolJSON, err := json.MarshalIndent(tool, "", " ") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal tool %s: %w", toolName, err) | ||
| } | ||
| snapPath := fmt.Sprintf("__toolsnaps__/%s.snap", toolName) | ||
| // If UPDATE_TOOLSNAPS is set, then we write the tool JSON to the snapshot file and exit | ||
| if os.Getenv("UPDATE_TOOLSNAPS") == "true" { | ||
| return writeSnap(snapPath, toolJSON) | ||
| } | ||
| snapJSON, err := os.ReadFile(snapPath) //nolint:gosec // filepaths are controlled by the test suite, so this is safe. | ||
| // If the snapshot file does not exist, this must be the first time this test is run. | ||
| // We write the tool JSON to the snapshot file and exit. | ||
| if os.IsNotExist(err) { | ||
| // If we're running in CI, we will error if there is not snapshot because it's important that snapshots | ||
| // are committed alongside the tests, rather than just being constructed and not committed during a CI run. | ||
| if os.Getenv("GITHUB_ACTIONS") == "true" { | ||
| return fmt.Errorf("tool snapshot does not exist for %s. Please run the tests with UPDATE_TOOLSNAPS=true to create it", toolName) | ||
| } | ||
| return writeSnap(snapPath, toolJSON) | ||
| } | ||
| // Otherwise we will compare the tool JSON to the snapshot JSON | ||
| toolNode, err := jd.ReadJsonString(string(toolJSON)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse tool JSON for %s: %w", toolName, err) | ||
| } | ||
| snapNode, err := jd.ReadJsonString(string(snapJSON)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse snapshot JSON for %s: %w", toolName, err) | ||
| } | ||
| // jd.Set allows arrays to be compared without order sensitivity, | ||
| // which is useful because we don't really care about this when exposing tool schemas. | ||
| diff := toolNode.Diff(snapNode, jd.SET).Render() | ||
| if diff != "" { | ||
| // If there is a difference, we return an error with the diff | ||
| return fmt.Errorf("tool schema for %s has changed unexpectedly:\n%s", toolName, diff) | ||
| } | ||
| return nil | ||
| } | ||
| func writeSnap(snapPath string, contents []byte) error { | ||
| // Ensure the directory exists | ||
| if err := os.MkdirAll(filepath.Dir(snapPath), 0700); err != nil { | ||
| return fmt.Errorf("failed to create snapshot directory: %w", err) | ||
| } | ||
| // Write the snapshot file | ||
| if err := os.WriteFile(snapPath, contents, 0600); err != nil { | ||
| return fmt.Errorf("failed to write snapshot file: %w", err) | ||
| } | ||
| return nil | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| package toolsnaps | ||
| import ( | ||
| "encoding/json" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
| type dummyTool struct { | ||
| Name string `json:"name"` | ||
| Value int `json:"value"` | ||
| } | ||
| // withIsolatedWorkingDir creates a temp dir, changes to it, and restores the original working dir after the test. | ||
| func withIsolatedWorkingDir(t *testing.T) { | ||
| dir := t.TempDir() | ||
| origDir, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { require.NoError(t, os.Chdir(origDir)) }) | ||
| require.NoError(t, os.Chdir(dir)) | ||
| } | ||
| func TestSnapshotDoesNotExistNotInCI(t *testing.T) { | ||
| withIsolatedWorkingDir(t) | ||
| // Given we are not running in CI | ||
| t.Setenv("GITHUB_ACTIONS", "false") // This REALLY is required because the tests run in CI | ||
| tool := dummyTool{"foo", 42} | ||
| // When we test the snapshot | ||
| err := Test("dummy", tool) | ||
| // Then it should succeed and write the snapshot file | ||
| require.NoError(t, err) | ||
| path := filepath.Join("__toolsnaps__", "dummy.snap") | ||
| _, statErr := os.Stat(path) | ||
| assert.NoError(t, statErr, "expected snapshot file to be written") | ||
| } | ||
| func TestSnapshotDoesNotExistInCI(t *testing.T) { | ||
| withIsolatedWorkingDir(t) | ||
| // Given we are running in CI | ||
| t.Setenv("GITHUB_ACTIONS", "true") | ||
| tool := dummyTool{"foo", 42} | ||
| // When we test the snapshot | ||
| err := Test("dummy", tool) | ||
| // Then it should error about missing snapshot in CI | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "tool snapshot does not exist", "expected error about missing snapshot in CI") | ||
| } | ||
| func TestSnapshotExistsMatch(t *testing.T) { | ||
| withIsolatedWorkingDir(t) | ||
| // Given a matching snapshot file exists | ||
| tool := dummyTool{"foo", 42} | ||
| b, _ := json.MarshalIndent(tool, "", " ") | ||
| require.NoError(t, os.MkdirAll("__toolsnaps__", 0700)) | ||
| require.NoError(t, os.WriteFile(filepath.Join("__toolsnaps__", "dummy.snap"), b, 0600)) | ||
| // When we test the snapshot | ||
| err := Test("dummy", tool) | ||
| // Then it should succeed (no error) | ||
| require.NoError(t, err) | ||
| } | ||
| func TestSnapshotExistsDiff(t *testing.T) { | ||
| withIsolatedWorkingDir(t) | ||
| // Given a non-matching snapshot file exists | ||
| require.NoError(t, os.MkdirAll("__toolsnaps__", 0700)) | ||
| require.NoError(t, os.WriteFile(filepath.Join("__toolsnaps__", "dummy.snap"), []byte(`{"name":"foo","value":1}`), 0600)) | ||
| tool := dummyTool{"foo", 2} | ||
| // When we test the snapshot | ||
| err := Test("dummy", tool) | ||
| // Then it should error about the schema diff | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "tool schema for dummy has changed unexpectedly", "expected error about diff") | ||
| } | ||
| func TestUpdateToolsnaps(t *testing.T) { | ||
| withIsolatedWorkingDir(t) | ||
| // Given UPDATE_TOOLSNAPS is set, regardless of whether a matching snapshot file exists | ||
| t.Setenv("UPDATE_TOOLSNAPS", "true") | ||
| require.NoError(t, os.MkdirAll("__toolsnaps__", 0700)) | ||
| require.NoError(t, os.WriteFile(filepath.Join("__toolsnaps__", "dummy.snap"), []byte(`{"name":"foo","value":1}`), 0600)) | ||
| tool := dummyTool{"foo", 42} | ||
| // When we test the snapshot | ||
| err := Test("dummy", tool) | ||
| // Then it should succeed and write the snapshot file | ||
| require.NoError(t, err) | ||
| path := filepath.Join("__toolsnaps__", "dummy.snap") | ||
| _, statErr := os.Stat(path) | ||
| assert.NoError(t, statErr, "expected snapshot file to be written") | ||
| } | ||
| func TestMalformedSnapshotJSON(t *testing.T) { | ||
| withIsolatedWorkingDir(t) | ||
| // Given a malformed snapshot file exists | ||
| require.NoError(t, os.MkdirAll("__toolsnaps__", 0700)) | ||
| require.NoError(t, os.WriteFile(filepath.Join("__toolsnaps__", "dummy.snap"), []byte(`not-json`), 0600)) | ||
| tool := dummyTool{"foo", 42} | ||
| // When we test the snapshot | ||
| err := Test("dummy", tool) | ||
| // Then it should error about malformed snapshot JSON | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "failed to parse snapshot JSON for dummy", "expected error about malformed snapshot JSON") | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||||
| { | ||||||||
| "annotations": { | ||||||||
| "title": "Get my user profile", | ||||||||
| "readOnlyHint": true | ||||||||
| }, | ||||||||
| "description": "Get details of the authenticated GitHub user. Use this when a request includes \"me\", \"my\". The output will not change unless the user changes their profile, so only call this once.", | ||||||||
| "inputSchema": { | ||||||||
| "properties": { | ||||||||
| "reason": { | ||||||||
| "description": "Optional: the reason for requesting the user information", | ||||||||
| "type": "string" | ||||||||
| } | ||||||||
| }, | ||||||||
| "type": "object" | ||||||||
| }, | ||||||||
| "name": "get_me" | ||||||||
| } | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Suggested change
It doesn't really matter, but I do love EOF newlines being added by automation rather than omitted if it's possible. CollaboratorAuthor
| ||||||||
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.