-
Notifications
You must be signed in to change notification settings - Fork 682
fix(amazonq): Addition of wait function for Amazon Q inline chat UI E2E Tests and Inline Keybind Shortcut Test #7840
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
base: feature/ui-e2e-tests
Are you sure you want to change the base?
Conversation
|
it('Inline Test', async () => { | ||
await writeToTextEditor(textEditor, 'Select Me') | ||
after(async function () { | ||
// Switch back to Webview Iframe when dealing with external webviews from Amazon Q. |
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.
nit: should be corrected to iframe
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.
Gotcha!
* @returns Promise<void> | ||
* @throws Error if timeout is exceeded | ||
*/ | ||
export async function waitForEditorStabilization(editor: TextEditor, timeout = 15000): Promise<void> { |
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 there a better name for this function? edtior stabilization sounds a bit unclear to me, maybe something like waitForTextGeneration is better?
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 idea!
|
||
if (currentLines === previousLines) { | ||
stableCount++ | ||
if (stableCount >= 2) { |
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.
whats the logic for breaking when stableCount is >= 2? does that mean that as soon as we see more then 2 lines we break?
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, this essentially will check for if the number of lines has not changed yet. If this number of lines being the same occurs twice (requiring another sleep), then we can say that the model has stopped generating code or thinking. Although it might be more useful to say this is just greater than 2, rather than equal to 2.
407de00
to
2ecfebc
Compare
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.
One small doc comment, otherwise lgtm.
const text = await textEditor.getText() | ||
assert.equal(text, 'Select Me') | ||
assert.equal(text, 'def factorial(n): ') |
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.
maybe a silly question, but why does this pass with the space at the end, if the writeToTextEditor
above doesn't add a space at the end? Or does it automatically?
Is this related to the dummy space below?
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 dummy space actually creates a space at the end of the word, and then it types from before that space so the space ends up at the end of whatever text we generate. I actually removed this logic since it is no longer needed in the test (the assert equal text)
} | ||
|
||
/** | ||
* Waits for Inline Generation by Amazon Q by checking if line count stops changing |
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, I like the logic you decided on here. Might be worth summarizing this logic in the doc-string since it would not be obvious to me without reading your PR description. (Nice PR description btw!)
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.
Thanks! Will do.
Looks like a lint error:
|
Fix lint issue with writeToTextEditor
eslint
Lint changes
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!
Problem
Currently we use a
sleep(8000)
function which causes us to wait an arbitrary 8 seconds for Amazon Q to implement the fibonacci sequence. This can cause flakiness and is generally not a good practice. We also have not fully updated our inline test and our writeToTextEditor function does not work well. We also have not implemented an inline test which uses keybinds to start an inline chat suggestion.Solution
I have implemented a function which constantly checks for a "stable" state in which we check for any new number of lines generated by Amazon Q. If this stable state is repeated more than 2 times, meaning the number of lines has stayed the same for 3 seconds (1 second wait in between), we will confirm that AmazonQ has finished generating its response. We have a maximum generate time of 15 seconds, as a fibonacci sequence should not take more than that, or something is clearly wrong with the model and we will error out. We have implemented the inline tests and the writeToTextEditor function fix.
feature/x
branches will not be squash-merged at release time.