Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 27, 2021
Merged

refactor(testing): fix flaky terminal test #3230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c727d9
refactor: make CodeServer methods more stable
jsjoeio Apr 26, 2021
449c6da
refactor: add timeout to terminal.test.ts
jsjoeio Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 27 additions & 30 deletions test/e2e/models/CodeServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { CODE_SERVER_ADDRESS } from "../../utils/constants"
// 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


constructor(page: Page) {
this.page = page
Expand All @@ -30,15 +31,18 @@ export class CodeServer {
// but usually a reload or two fixes it
// TODO@jsjoeio @oxy look into Firefox reconnection/timeout issues
while (!editorIsVisible) {
// When a reload happens, we want to wait for all resources to be
// loaded completely. Hence why we use that instead of DOMContentLoaded
// 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

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.

reloadCount += 1
if (await this.isEditorVisible()) {
console.log(` Editor became visible after ${reloadCount} reloads`)
break
}
// When a reload happens, we want to wait for all resources to be
// loaded completely. Hence why we use that instead of DOMContentLoaded
// Read more: https://thisthat.dev/dom-content-loaded-vs-load/
await this.page.reload({ waitUntil: "load" })
await this.page.reload()
}
}

Expand All @@ -49,46 +53,39 @@ export class CodeServer {
// Make sure the editor actually loaded
// If it's not visible after 5 seconds, something is wrong
await this.page.waitForLoadState("networkidle")
return await this.page.isVisible("div.monaco-workbench", { timeout: 5000 })
return await this.page.isVisible(this.editorSelector)
}

/**
* Focuses Integrated Terminal
* by going to the Application Menu
* and clicking View > Terminal
* by using "Terminal: Focus Terminal"
* from the Command Palette
*
* This should focus the terminal no matter
* if it already has focus and/or is or isn't
* visible already.
*/
async focusTerminal() {
// If the terminal is already visible
// then we can focus it by hitting the keyboard shortcut
const isTerminalVisible = await this.page.isVisible("#terminal")
if (isTerminalVisible) {
await this.page.keyboard.press(`Control+Backquote`)
// Wait for terminal to receive focus
await this.page.waitForSelector("div.terminal.xterm.focus")
// Sometimes the terminal reloads
// which is why we wait for it twice
await this.page.waitForSelector("div.terminal.xterm.focus")
return
}
// Open using the manu
// Click [aria-label="Application Menu"] div[role="none"]
await this.page.click('[aria-label="Application Menu"] div[role="none"]')

// Click text=View
await this.page.hover("text=View")
await this.page.click("text=View")

// Click text=Terminal
await this.page.hover("text=Terminal")
await this.page.click("text=Terminal")
// 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")
Comment on lines +76 to +85
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.


// Wait for terminal to receive focus
// Sometimes the terminal reloads once or twice
// which is why we wait for it to have the focus class
await this.page.waitForSelector("div.terminal.xterm.focus")
// Sometimes the terminal reloads
// 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

}

/**
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/terminal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ test.describe("Integrated Terminal", () => {
await page.waitForLoadState("load")
await page.keyboard.type(`echo '${testString}' > '${tmpFile}'`)
await page.keyboard.press("Enter")
// It may take a second to process
await page.waitForTimeout(1000)
Comment on lines +55 to +56
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.


const { stdout } = await output
expect(stdout).toMatch(testString)
Expand Down