-
Notifications
You must be signed in to change notification settings - Fork 746
fix(amazonq): Inline test after function addition, writeToTextEditor fix, and package.json pathing fix #7800
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
fix(amazonq): Inline test after function addition, writeToTextEditor fix, and package.json pathing fix #7800
Conversation
|
| // Switch back to Webview Iframe when dealing with external webviews from Amazon Q. | ||
| await editorView.closeAllEditors() | ||
| await webviewView.switchToFrame() | ||
| testContext.webviewView = webviewView |
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.
Is this line for setting the testContext to webview needed? In the sense that the webview isn't being update or changed so should the state be updated?
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.
Oh smart, yeah that isn't needed
| const input = new InputBox() | ||
| await input.sendKeys('Write a simple sentece') | ||
| await input.sendKeys('Generate the fibonacci sequence through iteration') | ||
| await input.sendKeys(Key.ENTER) |
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.
Does this enter now accept the generation?
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.
Unfortunately it does not, but it is required for us to get out of the suggestion decision messing up any following clean ups
| * @returns Promise<void> | ||
| */ | ||
| export async function writeToTextEditor(textEditor: TextEditor, text: string): Promise<void> { | ||
| await textEditor.typeTextAt(1, 1, ' ') |
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.
I know the reason why we need a space, but should we add a comment since its not obvious?
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.
Sure
Hweinstock
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.
Nice work! So with this PR can we run all the tests together?
| const driver = textEditor.getDriver() | ||
| await pressKey(driver, 'ENTER') | ||
| await pressKey(driver, 'TAB') | ||
| await sleep(8000) |
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.
what is this sleep for? 8 seconds sounds like a lot
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.
Good catch, the sleep is for waiting for the AmazonQ response to be generated. I realized its never always the same and can take sometime to either reason or just start coding right away. 8 seconds seems like the sweet spot for leeway for waiting for a response to be generated fully on other devices as well.
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.
I see. Is there a way we can wait dynamically with a cap? Like basically poll every 1 second until the response shows up the in the DOM. I think this general util will be helpful for other areas as well.
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.
Good point, will look into it!
|
|
||
| const textAfter = await textEditor.getText() | ||
| assert(textAfter.length > textBefore.length, 'Amazon Q should have generated code') | ||
| assert(textAfter.includes('fibonacci'), 'Generated code should contain fibonacci logic') |
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.
Could this be potentially flaky? Wondering if the model ever generates a solution where they use fib or hallucinates something.
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.
Good point, fixed!
Hweinstock
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.
I think we should avoid the sleep, and create some type of waiting utility, but otherwise this LGTM.
|
/retryBuilds |
…fix, and package.json pathing fix (aws#7800) ## Problem The inline test cannot be run consecutively with other tests in our suite since it switches its focus to the editorView and textEditorView. Each time the inline test is run, a new driver was launched with caused a new VSCode instance to instantiate. The `package.json` should only target the `.test.js` files. Right now it targets all .js files in the directory. The `writeToTextEditor` function is faulty since it is wrongly indexed due to its element not being accessed before counting and referencing its indices within code. ## Solution Implemented an `after` function which correctly switches back to our webviewView (AmazonQ). Added the true inline test into the suite and fixed driver problem. Changed the path referenced in the `package.json` to `.test.js` files. Fixed the `writeToTextEditor` function such that it includes a "dummy" space input into the textEditor and then counts the number of indices of lines. This then writes the desired text within the correct line without problems. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
…fix, and package.json pathing fix (aws#7800) ## Problem The inline test cannot be run consecutively with other tests in our suite since it switches its focus to the editorView and textEditorView. Each time the inline test is run, a new driver was launched with caused a new VSCode instance to instantiate. The `package.json` should only target the `.test.js` files. Right now it targets all .js files in the directory. The `writeToTextEditor` function is faulty since it is wrongly indexed due to its element not being accessed before counting and referencing its indices within code. ## Solution Implemented an `after` function which correctly switches back to our webviewView (AmazonQ). Added the true inline test into the suite and fixed driver problem. Changed the path referenced in the `package.json` to `.test.js` files. Fixed the `writeToTextEditor` function such that it includes a "dummy" space input into the textEditor and then counts the number of indices of lines. This then writes the desired text within the correct line without problems. --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Problem
The inline test cannot be run consecutively with other tests in our suite since it switches its focus to the editorView and textEditorView. Each time the inline test is run, a new driver was launched with caused a new VSCode instance to instantiate.
The
package.jsonshould only target the.test.jsfiles. Right now it targets all .js files in the directory.The
writeToTextEditorfunction is faulty since it is wrongly indexed due to its element not being accessed before counting and referencing its indices within code.Solution
Implemented an
afterfunction which correctly switches back to our webviewView (AmazonQ). Added the true inline test into the suite and fixed driver problem.Changed the path referenced in the
package.jsonto.test.jsfiles.Fixed the
writeToTextEditorfunction such that it includes a "dummy" space input into the textEditor and then counts the number of indices of lines. This then writes the desired text within the correct line without problems.feature/xbranches will not be squash-merged at release time.