refactor(testing): fix flaky terminal test #3230

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

Jump to bottom

Merged

repo-ranger merged 2 commits into main from jsjoeio/fix-e2e-tests

Apr 27, 2021

Merged

refactor(testing): fix flaky terminal test #3230

repo-ranger merged 2 commits into main from jsjoeio/fix-e2e-tests

Apr 27, 2021

Conversation

jsjoeio

Copy link

Contributor

@jsjoeio jsjoeio commented Apr 26, 2021 •

edited

Loading

This PR fixes a flaky implementation of the focusTerminal method on the CodeServer model.

This was caught after merging into #3169 main and the e2e tests failed here 😅

Changes:

refactor reloadUntilEditorIsVisible method. It wasn't waiting for code-server to fully load before reloading. before "Editor became visible after 11 reloads -> "Editor became visible after 1 reload"
refactor focusTerminal method.

Before

The should show the Integrated Terminal only passed intermittently in Firefox:

 ✓ CodeServer should show the Integrated Terminal (20s)
 x 3) CodeServer should show the Integrated Terminal (47s)
 x 1) CodeServer should show the Integrated Terminal (47s)
 x 2) CodeServer should show the Integrated Terminal (47s)
 x 4) CodeServer should show the Integrated Terminal (48s)
 1 passed (52s)
 4 failed
 codeServer.test.ts:41:8 › [firefox] CodeServer should show the Integrated Terminal =============
 codeServer.test.ts:41:8 › [firefox] CodeServer should show the Integrated Terminal =============
 codeServer.test.ts:41:8 › [firefox] CodeServer should show the Integrated Terminal =============
 codeServer.test.ts:41:8 › [firefox] CodeServer should show the Integrated Terminal =============
 1) codeServer.test.ts:41:8 › [firefox] CodeServer should show the Integrated Terminal ============

e4bebc41-637c-4949-9b13-ddf10f64a6cc.mp4

What's happening: it's waiting for the selector div.terminal.xterm.focus to be visible, which doesn't happen for some reason and the test times-out.

image

After

image

@jsjoeio jsjoeio self-assigned this

Apr 26, 2021

@jsjoeio jsjoeio added the testing label

Apr 26, 2021

@codecov

Copy link

codecov bot commented Apr 26, 2021 •

edited

Loading

Codecov Report

Merging #3230 (45df7b5) into main (1e683f3) will not change coverage.
The diff coverage is n/a.

❗ Current head 45df7b5 differs from pull request most recent head 449c6da. Consider uploading reports for the commit 449c6da to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@
## main #3230 +/- ##
=======================================
 Coverage 46.90% 46.90% 
=======================================
 Files 23 23 
 Lines 1196 1196 
 Branches 237 237 
=======================================
 Hits 561 561 
 Misses 451 451 
 Partials 184 184

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e683f3...449c6da. Read the comment docs.

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-e2e-tests branch from 45df7b5 to 7529975 Compare

April 27, 2021 21:26

jsjoeio added 2 commits

April 27, 2021 14:35

@jsjoeio


 refactor: make CodeServer methods more stable

8c727d9

@jsjoeio


 refactor: add timeout to terminal.test.ts

449c6da

@jsjoeio jsjoeio force-pushed the jsjoeio/fix-e2e-tests branch from 7529975 to 449c6da Compare

April 27, 2021 21:35

@jsjoeio jsjoeio added this to the v3.9.4 milestone

Apr 27, 2021

@jsjoeio jsjoeio marked this pull request as ready for review

April 27, 2021 21:36

@jsjoeio jsjoeio requested a review from a team as a code owner

April 27, 2021 21:36

jawnsy

jawnsy approved these changes

Apr 27, 2021

View reviewed changes

test/e2e/models/CodeServer.ts

// Read more: https://thisthat.dev/dom-content-loaded-vs-load/

await this.page.waitForLoadState("load")

// Give it an extra second just in case it's feeling extra slow

await this.page.waitForTimeout(1000)

Copy link

Choose a reason for hiding this comment

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

ahah these things make me sad, totally fair though

Copy link

Contributor Author

Choose a reason for hiding this comment

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

lol i know. sadly, this timeout was the only thing that made it consistently pass.

jsjoeio

jsjoeio commented

Apr 27, 2021

View reviewed changes

test/e2e/models/CodeServer.ts

// See Playwright docs: https://playwright.dev/docs/pom/

export class CodeServer {

page: Page

editorSelector = "div.monaco-workbench"

Copy link

Contributor Author

Choose a reason for hiding this comment

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

🛠️ I realized how many places I was using this in the model so refactored it into a property of the class

test/e2e/models/CodeServer.ts

// Read more: https://thisthat.dev/dom-content-loaded-vs-load/

await this.page.waitForLoadState("load")

// Give it an extra second just in case it's feeling extra slow

await this.page.waitForTimeout(1000)

Copy link

Contributor Author

Choose a reason for hiding this comment

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

lol i know. sadly, this timeout was the only thing that made it consistently pass.

test/e2e/models/CodeServer.ts

Comment on lines +76 to +85

// Click text=Command Palette

await this.page.hover("text=Command Palette")

await this.page.click("text=Command Palette")

// Type Terminal: Focus Terminal

await this.page.keyboard.type("Terminal: Focus Terminal")

// Click Terminal: Focus Terminal

await this.page.hover("text=Terminal: Focus Terminal")

await this.page.click("text=Terminal: Focus Terminal")

Copy link

Contributor Author

Choose a reason for hiding this comment

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

This approach produces consistent results — no matter if the terminal is already focused, visible/not visible.

test/e2e/models/CodeServer.ts

// which is why we wait for it twice

await this.page.waitForSelector("div.terminal.xterm.focus")

// Wait for terminal textarea to show up

await this.page.waitForSelector("textarea.xterm-helper-textarea")

Copy link

Contributor Author

Choose a reason for hiding this comment

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

This also seems to be better than looking for the div.terminal.xterm.focus selector, which may or may not appear.

H/T to @kylecarbs for the suggestion 🎉
xterm source

test/e2e/terminal.test.ts

Comment on lines +55 to +56

// It may take a second to process

await page.waitForTimeout(1000)

Copy link

Contributor Author

Choose a reason for hiding this comment

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

There is no great way of knowing that the terminal received both the input and the enter and processed it (because it's a <canvas> element so we can't assert the text on screen) so having this timeout accounts for any slowness that might occur.

@jsjoeio jsjoeio added the merge when passing label

Apr 27, 2021

@repo-ranger repo-ranger bot merged commit bc459e6 into main

Apr 27, 2021

@repo-ranger repo-ranger bot deleted the jsjoeio/fix-e2e-tests branch

April 27, 2021 21:52

Labels

testing

2 participants

@jsjoeio @jawnsy

refactor(testing): fix flaky terminal test #3230

refactor(testing): fix flaky terminal test #3230

Uh oh!

Conversation

@jsjoeio jsjoeio commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

codecov bot commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

@jawnsy jawnsy Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

@jsjoeio jsjoeio Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@jsjoeio jsjoeio commented Apr 26, 2021 •

edited

Loading

codecov bot commented Apr 26, 2021 •

edited

Loading