-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Expand Edit Tool tests, fix auto-apply duplicate responses #5866
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Your cubic subscription is currently inactive. Please reactivate your subscription to receive AI reviews and use cubic. |
✅ Deploy Preview for continuedev canceled.
|
sestinj
left a comment
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.
seems like this fix will work, but not sure how we pin it down from breaking again if there aren't tests. While testing auto-apply seems like overkill given it's an experiment, do we have enough vitest gui tests for the edit file tool that we can be sure this doesn't break something else? I think worth adding one, even if simple
|
✨ No issues found! Your code is sparkling clean! ✨ |
RomneyDa
left a comment
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.
Fills out the GUI integration testing with better utils.
Fixes a bug where setInactive is triggered just before streamResponseAfterToolCall
Moves auto accept edit tool functionality from edit tool implementation to parallel listeners
Changes StyledMarkdownPreview to force streamId by toolCallId rather than generating a streamId and forcing streamId
gui/src/context/MockIdeMessenger.ts
Outdated
| ]; | ||
| ], | ||
| ]; | ||
| chatStreamDelay: number = 0; |
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.
Expands the abilities of MockIdeMessenger to be able to set the contents returned (similar to MockLLM) and mock delays
| configUpdates?: Partial<BrowserSerializedContinueConfig>; | ||
| } | ||
|
|
||
| export function triggerConfigUpdate({ |
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.
Utils for triggering config updates in redux for testing, as well as selecting model, etc.
| if (out.status === "error") { | ||
| throw new Error(out.error); | ||
| } | ||
| return { |
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.
This is the main fix, which prevents there being two simultaneous calls to the model which can result in malformed json
| generateTitle: true, | ||
| }), | ||
| ); | ||
| } |
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.
setActive and setInactive are moved to around the actual streams, rather than in the streamThunkWrapper, since the wrapper can wrap chains of thunks, which causes setInactive to be prematurely triggered
| globals: true, | ||
| environment: "jsdom", | ||
| setupFiles: "./src/util/test/setupTests.ts", | ||
| onConsoleLog(log, type) { |
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.
ignore redundant/unecessary logs for our setup
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.
Allows selection of main editor in integration test utils
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.
Expands abilities of MockIdeMessenger, and streamlines data mocking for messaging
| toolCallId: currentToolCallApplyState.toolCallId!, | ||
| }), | ||
| ); | ||
| if (state.status === "done" && autoAcceptEditToolDiffs) { |
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.
auto accept edit tool implementation moved from edit tool to here
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.
Chat test is unchanged other than using the new utils.
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.
Apply test is unchanged other than using the new utils
|
@sestinj tests have been added |
sestinj
left a comment
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.
Assuming this has been tested manually in debug mode, this is ready to merge
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Expands edit tool testing, adding full edit tool walkthrough integration tests for multiple scenarios
When auto apply edits was enabled, accept was triggered
Updated to only be in parallel listeners.
This required moving around
setActiveandsetInactivedispatches because with nested thunks the stream wrapper would complete and setInactive which would cause subsequent response after tool call to abort.Also changes StyledMarkdownPreview to force streamId by toolCallId rather than generating a streamId and forcing streamId