- Notifications
You must be signed in to change notification settings - Fork1.4k
Add toolsnaps for every tool#543
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
Backfill toolsnaps invocations into all GitHub tool tests and generate corresponding snapshot files to catch unintended schema changes.
- Insert
require.NoError(t, toolsnaps.Test(...))in every tool test. - Add a snapshot file under
pkg/github/__toolsnaps__/for each tool. - Update the toolsnaps library’s error message to include instructions for updating snapshots.
Reviewed Changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/toolsnaps/toolsnaps.go | Augment error message to suggestUPDATE_TOOLSNAPS=true. |
| pkg/github/*_test.go | Invoketoolsnaps.Test in each tool’s test. |
| pkg/github/toolsnaps/*.snap | New snapshot files for every tool schema. |
Comments suppressed due to low confidence (2)
pkg/github/search_test.go:21
- The call to
require.NoErroris unqualified because therequirepackage isn't imported. Addimport "github.com/stretchr/testify/require"to avoid compile errors.
require.NoError(t, toolsnaps.Test(tool.Name, tool))pkg/github/pullrequests_test.go:932
- Variable
toolis undefined in this scope (onlyhandlerwas assigned). Capture the tool return value before using it, e.g.tool, handler := UpdatePullRequestBranch(...).
require.NoError(t, toolsnaps.Test(tool.Name, tool))522b2b1 to40ab680Compare846fac6 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Description
Some time ago I introduced the idea of
toolsnapsto highlight unintended changes to our tool schemas. I also imagined that it might have the benefit of quickly validating conformance if/when the project swaps to the official Go supported SDK. This PR backfills the toolsnaps to all tools.Here's a reminder of what the error messages look like:
Future Thoughts
Like with the desire to produce README's automatically from tool schemas, it would be better if there were a single data structure representing our tools, and then we could iterate that rather than relying on remembering to add the toolsnap call in each test.