-
Notifications
You must be signed in to change notification settings - Fork 209
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
feat: test (edit) command & add tests code lenses #2959
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.
Woo!
- Why the rename to
/unit
? I don't see a strong argument for renaming — I'd recommend we keep a single/test
command (the edit version) and replace the old/test
(chat version) with this one - I couldn't get this to work well with Python or Typescript… it seems to replace the code with the tests (see videos below)
- We might need to add some kind of progress that isn't just a spinner, because tests can take quite a while to generate. Maybe something like
\
Generating tests… (${n} lines)``? - Is the "unit-test-case" command — it's an interesting idea, but I don't fully understand what problem it's trying to solve. Could you expand more on how it fits into the overall flow?
CleanShot.2024-01-31.at.16.38.16.mp4
CleanShot.2024-01-31.at.16.31.13.mp4
Because jetBrains is using the old one so if I was worried switching it would cause issue for them, but I think I can find a workaround for that. |
That's for adding more tests to an existing test suite inside the test file, vs create a new test suite with tests using other test context as reference, adding more tests to an existing test suite should be easier for the llm and the result should have a higher quality from what I've tested so far. |
Let me try to reproduce it. Can you try it on non-react typescript code in the meantime? |
That's interesting about higher quality — is that higher quality output for the same user problem? (i.e. same source and test file, but running the command from the test file first rather than the source file first). I still don't quite understand what "Add Tests" quite means, or how it fits into their overall flow though. When would someone know to hit and not hit the "Add Tests" button? |
@toolmantim Can you share the link to the repo you tested with to see if I can reproduce the issue? I can't reproduce the issue when testing it on the langchain python repository. python.mov |
|
Noticed one thing, it seems to often not stream the text into the new file for me. It's a bit off putting because it feels like nothing is working, but actually the file seems to be just created at the end. I got a good video of it doing this: TestStreaming.movIt looks like the file name isn't properly returned by the LLM. WDYT about maybe firing two LLM requests for this command:
We had something similar for the intent detector: https://sourcegraph.com/github.com/sourcegraph/cody@bee/new-commands/-/blob/lib/shared/src/intent-detector/client.ts?L66%3A1-125%3A6 We could do the same 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.
Left some minor comments. Looking really good!
const symbols = | ||
allSymbols?.filter( | ||
symbol => | ||
symbol.kind === vscode.SymbolKind.Function || | ||
symbol.kind === vscode.SymbolKind.Class || | ||
symbol.kind === vscode.SymbolKind.Method || | ||
symbol.kind === vscode.SymbolKind.Constructor | ||
) ?? [] | ||
|
||
// Add code lenses for each symbol | ||
if (symbols) { | ||
// For test files, adds code lenses for each symbol | ||
if (isTest) { | ||
for (const symbol of symbols) { |
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.
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.
Yea we should update the code lenses logic in a follow up PR!
Co-authored-by: Tom Ross <tom@umpox.com>
Co-authored-by: Tom Ross <tom@umpox.com>
@toolmantim @umpox i've updated the prompt so that it will always generate the file name. Before this change, if cody doesn't reply with a file name suggestion, we would wait for the test to finish before adding the code, causing a delay in showing the response. |
@toolmantim also put the Add Tests code lense behind the unstable flag so the new /test command won't get blocked by it. |
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.
Works great with the examples I found now! Let's just go with a single /test
command (edit) for now though.
CLOSE #1475 & #1761
PR to introduce
/unit
command that is a smarter version of the/test
command./unit command
/unit
command generates unit tests in chat (old/test
)/test
command generates unit tests in lineAdd Tests code lenses
add-tests
is a command that can be executed via the Cody Command CodeLenses (when enabled) in test files. It is not available via the command menu or context menu.Behind internal unstable flag.
Summary
This PR includes the following change:
executeUnitCaseCommand
to execute unit test casesdestination file
in the executeEditArguments interface in the edit/execute.ts file.destination file
in the executeEdit method in the edit/manager.ts file.NewFixupFileMap
import and added support for specifying adestination file
in the EditProvider class in the edit/provider.ts file.NewFixupFileMap
import in the FixupController class in the non-stop/FixupController.ts file.Test plan
Added e2e test.
Test
Run the
Test
command to confirm the newly generated tests are added to a new / exisiting test file automatically for you.Add Tests
Enable Code Lenses in your Cody Setting
Locate the
Add Tests
code lenses in a test file to check if new test cases have been inserted in line for youFollow-up