Skip to content
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: add generated unit test to files automatically #2646

Merged
merged 19 commits into from
Jan 16, 2024
Merged

Conversation

abeatrix
Copy link
Contributor

@abeatrix abeatrix commented Jan 9, 2024

Part of #1475

  • Add new command for generating unit test in file so that it'd work with the edit command (will replace the old /test command once it's out of experimental)
  • Set it behind a config cody.internal.unstable
  • Add new EditMode file for inserting response to a different file (only used by /test currently)
  • Generates new unsaved test files based on existing test files in codebase if LLM didn't provide us with a name suggestion
  • Adds the generated test to the unsaved test file for users to review
  • Some additional codebase cleanup for commands

Note

This will not affect the /test command used by agent. In a follow up PR I will add a param that allows all commands to be run in chat mode only

Test plan

  1. Enable cody.internal.unstable
  2. Run the /unit command in the selected code to generate unit tests
  3. If there is a test file exists for the file that contains your selected code, the new unit tests will be added below the import statements in the file
  • if the user is not happy with the generated tests, simply close the file with the unsaved change
  1. If a test file does not exist for the selected file, the new unit tests will be added to a new unsaved file with the proposed file name for the new test file
  • user can then review the file before hitting save

In addition, confirm the following works as expected:

  • All existing tests are passing
  • Other commands should not be affected by this change

Demo

demo_auto_test.mov

Follow-ups

To-dos in follow-up PRs:

  • Add code lenses in the new test file with undo button
  • Update prompt to eliminate duplicated imports in new tests

@abeatrix abeatrix requested a review from umpox January 9, 2024 23:50
Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels really nice! Left some comments.

Mainly just wary of how we link Commands and Edits... It'd be awesome if we can abstract this slightly more - and might mean we can reuse this code easier in the future for any non-Command requirements

vscode/src/commands/CommandRunner.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupTask.ts Outdated Show resolved Hide resolved
vscode/src/edit/execute.ts Outdated Show resolved Hide resolved
@abeatrix
Copy link
Contributor Author

Feels really nice! Left some comments.

Mainly just wary of how we link Commands and Edits... It'd be awesome if we can abstract this slightly more - and might mean we can reuse this code easier in the future for any non-Command requirements

Thank you so much for your input @umpox! What you suggested makes sense and I've made some new changes in my local branch to reflect some of them.
I have some questions for you regarding the context fetching step, will discuss more during our 1:1 tomorrow. Thanks again for the review Tom!

@abeatrix abeatrix self-assigned this Jan 12, 2024
@abeatrix abeatrix requested review from umpox and a team January 12, 2024 01:15
@umpox
Copy link
Contributor

umpox commented Jan 12, 2024

image

This notification shows 🤔 It should usually only show if the file is not visible but I think it should be.

This is the code in streamTask:

            if (!visibleEditor) {
                await this.notifyTaskComplete(task)
            }

Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really good, left some comments!

I noticed that it can duplicate some stuff when there's already an existing test file with tests in it. I wonder if there's a nice way to provide the precedingText if the test file already exists 🤔 We use precedingText for the add intent and it works well to avoid duplication. Maybe one for a follow up PR though

vscode/src/editor/utils/editor-context.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Show resolved Hide resolved
vscode/src/non-stop/codelenses.ts Outdated Show resolved Hide resolved
vscode/src/edit/provider.ts Outdated Show resolved Hide resolved
@abeatrix abeatrix requested review from toolmantim, umpox and a team January 12, 2024 19:06
@abeatrix
Copy link
Contributor Author

@umpox it's now behind an experimental configuration.

Regarding I noticed that it can duplicate some stuff when there's already an existing test file with tests in it. it's an existing issue that I will look into in a follow up PR since it's more of a prompting issue.

vscode/package.json Outdated Show resolved Hide resolved
vscode/src/main.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupTask.ts Outdated Show resolved Hide resolved
Co-authored-by: Tom Ross <tom@umpox.com>
@abeatrix abeatrix merged commit bcd9790 into main Jan 16, 2024
14 of 15 checks passed
@abeatrix abeatrix deleted the bee/auto-unit-test branch January 16, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants