-
Notifications
You must be signed in to change notification settings - Fork 268
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
Chat: Add support for cody.chat.preInstruction in the new preamble #2255
Conversation
@beyang @dominiccooney @abeatrix before I go adding tests… does this approach look sound, or would you prefer it another way? |
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 looks good to me.
We have that "typed" configuration layer, tests for defaults, etc. but because this is simply "any string" I don't think we need to plumb through that here? @philipp-spiess do you have thoughts?
@dominiccooney I've just added some tests, if you could quickly give it a re-review |
@@ -0,0 +1,87 @@ | |||
import { afterEach, describe, expect, it, vi } from 'vitest' |
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.
Would have been nice to have an e2e test for this, but our e2e tests aren't very end-to-end and don't go through our prompt construction gear. I glanced at the integration tests, but they seem to be all marked as skipped because the getTranscript
function is broken atm. So a unit test it is!
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.
Wow, I am surprised that the e2e tests somehow avoid prompt construction.
Unit test seems perfect for this. We will be waiting an hour for test results if we have e2e tests for everything.
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.
LGTM! Agree that there's no need to pass cody.chat.preInstruction
through our typed getConfiguration
wrapper.
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.
Looks great. Thank you for adding a test.
This adds support for the
cody.chat.preInstruction
setting in simplified chat, especially important now we have a "Chat Settings" button in the top of the chat editor:Fix #2165
Test plan