vscode icon indicating copy to clipboard operation
vscode copied to clipboard
verified publisher icon microsoft

Review smoke test `sendKeybinding` usage

Open aiday-mar opened this issue 9 months ago • 3 comments

Note: in order to avoid unnecessarily breaking the build before the snow trip, it may be preferable to address this debt work starting from next week.

Last iteration, when the experimental edit context was turned on by default, it caused smoke test errors consistently. While debugging with Joao, it was found that the smoke test errors were caused by the dispatchKeybinding method which was dispatching the right arrow key press, but was not awaiting for the cursor to move right before continuing the computation. To mitigate such cases, the dispatchKeybinding was awaiting 100ms after sending the keybinding, in the hopes that the keybinding would be processed in this time frame. Since this can sometimes fail, we decided to add a callback to this method called accept which would end the dispatchKeybinding method execution when the accept method returns. In the following PR https://github.com/microsoft/vscode/pull/239902 I have added this callback and removed the 100ms timeout. I ran the VS Code CI pipeline on this PR 5 times, and it gave the following errors. Please note that the PR https://github.com/microsoft/vscode/pull/239902 is a draft closed PR and is not meant to be merged, but simply to demonstrate where the smoke tests fail.

The 5 runs were:

Run 1

https://dev.azure.com/monacotools/Monaco/_build/results?buildId=321945&view=results

linux - browser

1) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - undefined:
     Error: Timeout: get elements '.quick-input-widget .quick-input-list .monaco-list-row .quick-input-list-row > .monaco-icon-label .label-name' after 20 seconds.
      at Code.poll (/mnt/vss/_work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForElements (/mnt/vss/_work/1/s/test/automation/out/code.js:161:16)
      at async QuickInput.waitForQuickInputElements (/mnt/vss/_work/1/s/test/automation/out/quickinput.js:28:9)
      at async Task.assertTasks (/mnt/vss/_work/1/s/test/automation/out/task.js:29:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:38:17)

macos - browser

1) VSCode Smoke Tests (Electron)
       Terminal
         Terminal stickyScroll
           should support multi-line prompt:
     Error: Failed for command sticky scroll 2, exitcode 0, text content Prompt> sticky scroll 1 
      at checkCommandAndOutput (out/areas/terminal/terminal-stickyScroll.test.js:42:19)
      at async Context.<anonymous> (out/areas/terminal/terminal-stickyScroll.test.js:58:13)

  2) VSCode Smoke Tests (Electron)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - false:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:33:17)

  3) VSCode Smoke Tests (Electron)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - undefined:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:37:17)

  4) VSCode Smoke Tests (Electron)
       Task
         Task Quick Pick
           Tasks: Run Task
             icon - icon only:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:42:17)
Run 2

https://dev.azure.com/monacotools/Monaco/_build/results?buildId=322495&view=results

linux - browser

1) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - true:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/mnt/vss/_work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/mnt/vss/_work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/mnt/vss/_work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/mnt/vss/_work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:29:17)

  2) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - false:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/mnt/vss/_work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/mnt/vss/_work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/mnt/vss/_work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/mnt/vss/_work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:33:17)

macos - browser

1) VSCode Smoke Tests (Web)
       Terminal
         Terminal stickyScroll
           should support multi-line prompt:
     Error: Failed for command sticky scroll 1, exitcode 0, text content 
      at checkCommandAndOutput (out/areas/terminal/terminal-stickyScroll.test.js:42:19)
      at async Context.<anonymous> (out/areas/terminal/terminal-stickyScroll.test.js:56:13)

  2) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - false:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:33:17)

  3) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - undefined:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:37:17)

  4) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             icon - color only:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:47:17)

  5) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             icon - icon & color:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:52:17)
Run 3

https://dev.azure.com/monacotools/Monaco/_build/results?buildId=322537&view=results

linux - browser

  1) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - false:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/mnt/vss/_work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/mnt/vss/_work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/mnt/vss/_work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/mnt/vss/_work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:33:17)

  2) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - undefined:
     Error: Timeout: get elements '.quick-input-widget .quick-input-list .monaco-list-row .quick-input-list-row > .monaco-icon-label .label-name' after 20 seconds.
      at Code.poll (/mnt/vss/_work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForElements (/mnt/vss/_work/1/s/test/automation/out/code.js:161:16)
      at async QuickInput.waitForQuickInputElements (/mnt/vss/_work/1/s/test/automation/out/quickinput.js:28:9)
      at async Task.assertTasks (/mnt/vss/_work/1/s/test/automation/out/task.js:29:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:38:17)

macos - browser

 1) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - false:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:33:17)
Run 4

https://dev.azure.com/monacotools/Monaco/_build/results?buildId=322579&view=results

macOS browser:

  1) VSCode Smoke Tests (Electron)
       Terminal
         Terminal stickyScroll
           should show sticky scroll when appropriate:
     Error: Failed for command sticky scroll 2, exitcode 1, text content Prompt> sticky scroll 1 
      at checkCommandAndOutput (out/areas/terminal/terminal-stickyScroll.test.js:42:19)
      at async Context.<anonymous> (out/areas/terminal/terminal-stickyScroll.test.js:52:13)
Run 5

https://dev.azure.com/monacotools/Monaco/_build/results?buildId=322773&view=results

linux browser:

  1) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - false:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/mnt/vss/_work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/mnt/vss/_work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/mnt/vss/_work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/mnt/vss/_work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:33:17)

  2) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - undefined:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/mnt/vss/_work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/mnt/vss/_work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/mnt/vss/_work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/mnt/vss/_work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:37:17)

macOS browser:

1) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             hide property - false:
     Error: Timeout: get elements '.quick-input-widget .quick-input-list .monaco-list-row .quick-input-list-row > .monaco-icon-label .label-name' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForElements (/Users/runner/work/1/s/test/automation/out/code.js:161:16)
      at async QuickInput.waitForQuickInputElements (/Users/runner/work/1/s/test/automation/out/quickinput.js:28:9)
      at async Task.assertTasks (/Users/runner/work/1/s/test/automation/out/task.js:29:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:34:17)

  2) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             icon - color only:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:47:17)

  3) VSCode Smoke Tests (Web)
       Task
         Task Quick Pick
           Tasks: Run Task
             icon - icon & color:
     Error: Timeout: get text content ' .monaco-editor[data-uri$="tasks.json"] .view-lines' after 20 seconds.
      at Code.poll (/Users/runner/work/1/s/test/automation/out/code.js:208:23)
      at async Code.waitForTextContent (/Users/runner/work/1/s/test/automation/out/code.js:152:16)
      at async Editor.waitForTypeInEditor (/Users/runner/work/1/s/test/automation/out/editor.js:79:9)
      at async Task.configureTask (/Users/runner/work/1/s/test/automation/out/task.js:68:13)
      at async Context.<anonymous> (out/areas/task/task-quick-pick.test.js:52:17)

After examining these results, I have merged a PR which adds an optional callback to the dispatch method and also removes the 100ms timeout from the dispatchKeybinding method. For the smoke tests that fail below, I added a timeout of 100ms after the dispatchKeybinding call. The dispatchKeybinding has also been renamed to sendKeybinding to make it clearer that it is merely sending the keybinding.

  • As part of this change, it would be good if the owners of the smoke tests that send keybindings could review or add callbacks where necessary. It would be preferable to have callbacks everywhere so as to avoid smoke test flakiness.
  • For the failing smoke tests, it would be good if the owners could remove the 100ms timeout and add a callback which would ensure the keybinding is correctly processed.

Lines where to add a callback and remove the 100ms timeout

@meganrogge

@Tyriar

Lines where to review/add a callback

@connor4312

@jrieken

  • [ ] test/automation/src/editor.ts, line 39
  • [ ] test/automation/src/peek.ts, line 42

@bpasero

@TylerLeonhardt

  • [ ] test/automation/src/quickinput.ts, line 37
  • [ ] test/automation/src/quickinput.ts, line 51 to 57

@lramos15

@ulugbekna

@roblourens

  • [ ] test/automation/src/notebook.ts, line 30
  • [ ] test/automation/src/notebook.ts, line 38

@sandy081

@lszomoru

  • [ ] test/automation/src/scm.ts, line 48

@osortega

  • [ ] test/automation/src/search.ts, line 43 to 48
  • [ ] test/automation/src/search.ts, line 71
  • [ ] test/automation/src/search.ts, line 75
  • [ ] test/automation/src/search.ts, line 80
  • [ ] test/smoke/src/areas/search/search.test.ts, line 26 to 32

@rzhao271

Please let me know if you feel the assignment is incorrect.

aiday-mar avatar Mar 04 '25 08:03 aiday-mar

@aiday-mar I am having a hard time to follow. You indicate:

Lines where to add a callback and remove the 100ms timeout ... test/automation/src/editors.ts line 15 to 17

When I open that line in main, I see:

https://github.com/microsoft/vscode/blob/2b3cb533550639a43a144968b07576998a880718/test/automation/src/editors.ts#L14-L18

And on your PR I see:

https://github.com/microsoft/vscode/blob/3dea3480e2ee6da94dd1cdae10e72acc0964d0ea/test/automation/src/editors.ts#L14-L18

In both cases, I see no evidence of a 100ms timeout. I also do not fully understand what the added callback () => { } means that you added, as it does seem to be a No-Op.

Can you please rephrase what the ask is here, I am misunderstanding something.

Also, since only 5 tests seems to fail, shouldn't that be the first thing for people to look into that own these tests?

bpasero avatar Mar 07 '25 12:03 bpasero

Reading this now for the third time, I think the ask is to get rid of the callback if its not needed and only have it otherwise for the tests that fail, while also better understand what the 100ms waiting time is needed.

I think the ideal goal here is that dispatchKeybinding has no callback whatsoever, even the failing tests should not require any artificial timeouts?

Update: I still not understand this accept method, why would it be part of the dispatchKeybinding if its being awaited at the end of that method and not just something we do outside, after we have called dispatchKeybinding 🤔

bpasero avatar Mar 07 '25 12:03 bpasero

Hi @bpasero there are two headers: Lines where to review/add a callback for which the items assigned to you are under and the header Lines where to add a callback and remove the 100ms timeout for which only @meganrogge and @Tyriar only are assigned.

The PR you looked at is a draft closed PR which I used for testing. It is not meant to be merged. It's only function is to show the code I used for testing the removal of the 100ms timeout.

Yes so for the 5 tests that fail, I asked the people under the header Lines where to add a callback and remove the 100ms timeout to have a look at them. For these tests I added a 100ms timeouts, temporarily. These need to be removed and a callback needs to be added there. This does not concern you, since the smoke tests assigned to you did not fail with the change I made.

So I have done the work of adding callbacks where I thought I could find a reasonable callback. The ask is to review the lines in the issue description under your name, and decide whether the callback, if it exists, is a good callback, or whether it should be changed. Maybe no change is required. If there is no callback, it would be good to add one. I believe ideally we want to always have a callback which would ensure the keybinding is processed because the dispatchKeybinding method no longer has a 100ms timeout so there could be smoke test failures due to race conditions in the future.

Yes if we all wrote acceptance condition after the call to dispatchKeybinding, this would also fix the problem. I have added this parameter to serve as a signal to smoke test authors that they should check that the keybinding has resolved correctly. We use the same pattern for other methods in the smoke tests. Here are some signatures:

async waitForElements(selector: string, recursive: boolean, accept: (result: IElement[]) => boolean = result => result.length > 0): Promise<IElement[]>;

async waitForEditorSelection(selector: string, accept: (selection: { selectionStart: number; selectionEnd: number }) => boolean): Promise<void>;

Essentially we are mirroring the same pattern.

Please also note I have renamed the method dispatchKeybinding to sendKeybinding, so even though I refer to dispatchKeybinding in this issue, it is in fact now sendKeybinding.

aiday-mar avatar Mar 07 '25 12:03 aiday-mar

@sandy081 assigned you peek and rename which seem to have been added with https://github.com/microsoft/vscode/commit/d9b889ff6de60c1213a674d0a15f6eff708ba414#diff-c624b0f902614d46a7fe3cd8a5b4987a831d12fcacf81632607802c1e8d3cf70

jrieken avatar Mar 21 '25 11:03 jrieken

✋ I would suggest to put this on hold, pending the outcome of https://github.com/microsoft/vscode/issues/246731

bpasero avatar Apr 16 '25 15:04 bpasero

@aiday-mar I moved the milestone given Ben's comment above, feel free to move back if I misinterpreted this.

meganrogge avatar Apr 24 '25 18:04 meganrogge

Moved to backlog since https://github.com/microsoft/vscode/issues/246731 is still open.

Tyriar avatar Jul 01 '25 12:07 Tyriar