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

refactor(codewhisperer): refactor UTG code path and add test coverage #5590

Merged
merged 13 commits into from
Sep 24, 2024

Conversation

Will-ShaoHua
Copy link
Contributor

@Will-ShaoHua Will-ShaoHua commented Sep 13, 2024

Problem

  • complicate regex being used which is not scalable and hard to understand
  • stale and complicate code which lacks readability
  • test coverage

Solution

java

java-example-1.mov
java-example-2.mov

no supported (kotlin)

kotlin-example.mov

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Will-ShaoHua Will-ShaoHua requested review from a team as code owners September 13, 2024 18:30
@Will-ShaoHua
Copy link
Contributor Author

Will-ShaoHua commented Sep 13, 2024

[done] will upload a functional feature test screen recording

@@ -24,10 +24,12 @@ import { getOpenFilesInWindow } from '../../../shared/utilities/editorUtilities'
import { getLogger } from '../../../shared/logger/logger'
import { CodeWhispererSupplementalContext, CodeWhispererSupplementalContextItem, UtgStrategy } from '../../models/model'

type UtgSupportedLanguage = keyof typeof utgLanguageConfigs
const UTG_SUPPORTEDLANGUAGES: vscode.TextDocument['languageId'][] = ['java', 'python']
Copy link
Member

Choose a reason for hiding this comment

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

Can we fetch the list of supported languages to BE. Because In order to extend the feature to additional languages we will rely on weekly release of IDEs, instead we should rely on Backend for this information so that we can make such decisions very dynamic.

Copy link
Contributor Author

@Will-ShaoHua Will-ShaoHua Sep 16, 2024

Choose a reason for hiding this comment

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

Yea that's a good callout and discussed offline with Vikash, I think we're limited by the infra we have at this point of time, our service doesn't have this kinda of dynamic configuration API yet except the one for A/B testing. So we can't do much on this for now.

This comment also apply to other Q features as well like "auto trigger classifier co-efficients" https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/codewhisperer/service/classifierTrigger.ts#L43 and other language level gated features. It would be definitely a good feature for us to track in the future

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

this test takes 10 seconds. ...

openATextEditorWithText opens an editor, which can be slow. Could we add a param to it which only calls vscode.workspace.openTextDocument and skips showTextDocument

That was implemented in the new assertIsTestFile function. I think it would be preferable to add that variant to testUtil.ts, but doesn't need to block this for now.

After that change, the total test time for the "validate by file name" tests is now 60 ms instead of 10 s. Nice!

@justinmk3 justinmk3 merged commit 1db0f97 into aws:master Sep 24, 2024
23 of 24 checks passed
@Will-ShaoHua Will-ShaoHua deleted the cwspr-utg-refactor branch September 24, 2024 21:09
@Will-ShaoHua
Copy link
Contributor Author

That was implemented in the new assertIsTestFile function. I think it would be preferable to add that variant to testUtil.ts, but doesn't need to block this for now.

sgtm, i will add this to testUtil

@justinmk3
Copy link
Contributor

justinmk3 commented Sep 24, 2024

sgtm, i will add this to testUtil

Oh, I didn't see this. I have a PR already for that #5660

@Will-ShaoHua
Copy link
Contributor Author

Will-ShaoHua commented Sep 24, 2024

Oh, I didn't see this. I have a PR already for that

gotcha, thanks Justin :)!

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.

5 participants