From f9402a63180741e8f85271181568004015f950fc Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 22 Feb 2022 12:43:13 -0600 Subject: [PATCH] fix: state collision (#4881) * Add helper for navigating the quick picker This has problems similar to the menu except instead of closing it gets re-created which interrupts the hover call and causes the test to fail. Now it will keep trying just like the menu. * Add a test for opening a file * Add test for colliding state * Update VS Code This contains the colliding state fix. --- test/e2e/codeServer.test.ts | 30 ++++++ test/e2e/models/CodeServer.ts | 191 ++++++++++++++++++++++++++-------- vendor/package.json | 2 +- vendor/yarn.lock | 4 +- 4 files changed, 183 insertions(+), 44 deletions(-) diff --git a/test/e2e/codeServer.test.ts b/test/e2e/codeServer.test.ts index 06eaaea0d..1f712f913 100644 --- a/test/e2e/codeServer.test.ts +++ b/test/e2e/codeServer.test.ts @@ -1,3 +1,5 @@ +import { promises as fs } from "fs" +import * as path from "path" import { describe, test, expect } from "./baseFixture" describe("CodeServer", true, [], () => { @@ -24,4 +26,32 @@ describe("CodeServer", true, [], () => { await codeServerPage.focusTerminal() expect(await codeServerPage.page.isVisible("#terminal")).toBe(true) }) + + test("should open a file", async ({ codeServerPage }) => { + const dir = await codeServerPage.workspaceDir + const file = path.join(dir, "foo") + await fs.writeFile(file, "bar") + await codeServerPage.openFile(file) + }) + + test("should not share state with other paths", async ({ codeServerPage }) => { + const dir = await codeServerPage.workspaceDir + const file = path.join(dir, "foo") + await fs.writeFile(file, "bar") + + await codeServerPage.openFile(file) + + // If we reload now VS Code will be unable to save the state changes so wait + // until those have been written to the database. It flushes every five + // seconds so we need to wait at least that long. + await codeServerPage.page.waitForTimeout(5500) + + // The tab should re-open on refresh. + await codeServerPage.page.reload() + await codeServerPage.waitForTab(file) + + // The tab should not re-open on a different path. + await codeServerPage.setup(true, "/vscode") + expect(await codeServerPage.tabIsVisible(file)).toBe(false) + }) }) diff --git a/test/e2e/models/CodeServer.ts b/test/e2e/models/CodeServer.ts index 62d0218e3..e8fff0716 100644 --- a/test/e2e/models/CodeServer.ts +++ b/test/e2e/models/CodeServer.ts @@ -3,7 +3,7 @@ import * as cp from "child_process" import { promises as fs } from "fs" import * as path from "path" import { Page } from "playwright" -import { logError } from "../../../src/common/util" +import { logError, plural } from "../../../src/common/util" import { onLine } from "../../../src/node/util" import { PASSWORD, workspaceDir } from "../../utils/constants" import { idleTimer, tmpdir } from "../../utils/helpers" @@ -13,14 +13,21 @@ interface CodeServerProcess { address: string } -class CancelToken { +class Context { private _canceled = false + private _done = false public canceled(): boolean { return this._canceled } + public done(): void { + this._done = true + } public cancel(): void { this._canceled = true } + public finish(): boolean { + return this._done + } } /** @@ -30,6 +37,7 @@ export class CodeServer { private process: Promise | undefined public readonly logger: Logger private closed = false + private _workspaceDir: Promise | undefined constructor(name: string, private readonly codeServerArgs: string[]) { this.logger = logger.named(name) @@ -47,11 +55,21 @@ export class CodeServer { return address } + /** + * The workspace directory code-server opens with. + */ + get workspaceDir(): Promise { + if (!this._workspaceDir) { + this._workspaceDir = tmpdir(workspaceDir) + } + return this._workspaceDir + } + /** * Create a random workspace and seed it with settings. */ private async createWorkspace(): Promise { - const dir = await tmpdir(workspaceDir) + const dir = await this.workspaceDir await fs.mkdir(path.join(dir, "User")) await fs.writeFile( path.join(dir, "User/settings.json"), @@ -184,11 +202,18 @@ export class CodeServerPage { } /** - * Navigate to code-server. + * The workspace directory code-server opens with. */ - async navigate() { - const address = await this.codeServer.address() - await this.page.goto(address, { waitUntil: "networkidle" }) + get workspaceDir() { + return this.codeServer.workspaceDir + } + + /** + * Navigate to a code-server endpoint. By default go to the root. + */ + async navigate(endpoint = "/") { + const to = new URL(endpoint, await this.codeServer.address()) + await this.page.goto(to.toString(), { waitUntil: "networkidle" }) } /** @@ -273,6 +298,29 @@ export class CodeServerPage { await this.page.waitForSelector("textarea.xterm-helper-textarea") } + /** + * Open a file by using menus. + */ + async openFile(file: string) { + await this.navigateMenus(["File", "Open File"]) + await this.navigateQuickInput([path.basename(file)]) + await this.waitForTab(file) + } + + /** + * Wait for a tab to open for the specified file. + */ + async waitForTab(file: string): Promise { + return this.page.waitForSelector(`.tab :text("${path.basename(file)}")`) + } + + /** + * See if the specified tab is open. + */ + async tabIsVisible(file: string): Promise { + return this.page.isVisible(`.tab :text("${path.basename(file)}")`) + } + /** * Navigate to the command palette via menus then execute a command by typing * it then clicking the match from the results. @@ -287,13 +335,45 @@ export class CodeServerPage { } /** - * Navigate through the specified set of menus. If it fails it will keep - * trying. + * Navigate through the items in the selector. `open` is a function that will + * open the menu/popup containing the items through which to navigation. */ - async navigateMenus(menus: string[]) { - const navigate = async (cancelToken: CancelToken) => { - const steps: Array<() => Promise> = [() => this.page.waitForSelector(`${menuSelector}:focus-within`)] - for (const menu of menus) { + async navigateItems(items: string[], selector: string, open?: (selector: string) => void): Promise { + const logger = this.codeServer.logger.named(selector) + + /** + * If the selector loses focus or gets removed this will resolve with false, + * signaling we need to try again. + */ + const openThenWaitClose = async (ctx: Context) => { + if (open) { + await open(selector) + } + this.codeServer.logger.debug(`watching ${selector}`) + try { + await this.page.waitForSelector(`${selector}:not(:focus-within)`) + } catch (error) { + if (!ctx.done()) { + this.codeServer.logger.debug(`${selector} navigation: ${error.message || error}`) + } + } + return false + } + + /** + * This will step through each item, aborting and returning false if + * canceled or if any navigation step has an error which signals we need to + * try again. + */ + const navigate = async (ctx: Context) => { + const steps: Array<{ fn: () => Promise; name: string }> = [ + { + fn: () => this.page.waitForSelector(`${selector}:focus-within`), + name: "focus", + }, + ] + + for (const item of items) { // Normally these will wait for the item to be visible and then execute // the action. The problem is that if the menu closes these will still // be waiting and continue to execute once the menu is visible again, @@ -301,43 +381,72 @@ export class CodeServerPage { // if the old promise clicks logout before the new one can). By // splitting them into two steps each we can cancel before running the // action. - steps.push(() => this.page.hover(`text=${menu}`, { trial: true })) - steps.push(() => this.page.hover(`text=${menu}`, { force: true })) - steps.push(() => this.page.click(`text=${menu}`, { trial: true })) - steps.push(() => this.page.click(`text=${menu}`, { force: true })) + steps.push({ + fn: () => this.page.hover(`${selector} :text("${item}")`, { trial: true }), + name: `${item}:hover:trial`, + }) + steps.push({ + fn: () => this.page.hover(`${selector} :text("${item}")`, { force: true }), + name: `${item}:hover:force`, + }) + steps.push({ + fn: () => this.page.click(`${selector} :text("${item}")`, { trial: true }), + name: `${item}:click:trial`, + }) + steps.push({ + fn: () => this.page.click(`${selector} :text("${item}")`, { force: true }), + name: `${item}:click:force`, + }) } + for (const step of steps) { - await step() - if (cancelToken.canceled()) { - this.codeServer.logger.debug("menu navigation canceled") + try { + logger.debug(`navigation step: ${step.name}`) + await step.fn() + if (ctx.canceled()) { + logger.debug("navigation canceled") + return false + } + } catch (error) { + logger.debug(`navigation: ${error.message || error}`) return false } } return true } - const menuSelector = '[aria-label="Application Menu"]' - const open = async () => { - await this.page.click(menuSelector) - await this.page.waitForSelector(`${menuSelector}:not(:focus-within)`) - return false + // We are seeing the menu closing after opening if we open it too soon and + // the picker getting recreated in the middle of trying to select an item. + // To counter this we will keep trying to navigate through the items every + // time we lose focus or there is an error. + let attempts = 1 + let context = new Context() + while (!(await Promise.race([openThenWaitClose(), navigate(context)]))) { + ++attempts + logger.debug("closed, retrying (${attempt}/∞)") + context.cancel() + context = new Context() } - // TODO: Starting in 1.57 something closes the menu after opening it if we - // open it too soon. To counter that we'll watch for when the menu loses - // focus and when/if it does we'll try again. - // I tried using the classic menu but it doesn't show up at all for some - // reason. I also tried toggle but the menu disappears after toggling. - let retryCount = 0 - let cancelToken = new CancelToken() - while (!(await Promise.race([open(), navigate(cancelToken)]))) { - this.codeServer.logger.debug("menu was closed, retrying") - ++retryCount - cancelToken.cancel() - cancelToken = new CancelToken() - } + context.finish() + logger.debug(`navigation took ${attempts} ${plural(attempts, "attempt")}`) + } - this.codeServer.logger.debug(`menu navigation retries: ${retryCount}`) + /** + * Navigate through a currently opened "quick input" widget, retrying on + * failure. + */ + async navigateQuickInput(items: string[]): Promise { + await this.navigateItems(items, ".quick-input-widget") + } + + /** + * Navigate through the menu, retrying on failure. + */ + async navigateMenus(menus: string[]): Promise { + await this.navigateItems(menus, '[aria-label="Application Menu"]', async (selector) => { + await this.page.click(selector) + }) } /** @@ -345,8 +454,8 @@ export class CodeServerPage { * * It is recommended to run setup before using this model in any tests. */ - async setup(authenticated: boolean) { - await this.navigate() + async setup(authenticated: boolean, endpoint = "/") { + await this.navigate(endpoint) // If we aren't authenticated we'll see a login page so we can't wait until // the editor is ready. if (authenticated) { diff --git a/vendor/package.json b/vendor/package.json index c1414d98f..cb8e1098b 100644 --- a/vendor/package.json +++ b/vendor/package.json @@ -7,6 +7,6 @@ "postinstall": "./postinstall.sh" }, "devDependencies": { - "code-oss-dev": "coder/vscode#96e241330d9c44b64897c1e5031e00aa894103db" + "code-oss-dev": "coder/vscode#6337ee490d16b7dfd8854d22c998f58d6cd21ef5" } } diff --git a/vendor/yarn.lock b/vendor/yarn.lock index 98921a1b6..7ad95d880 100644 --- a/vendor/yarn.lock +++ b/vendor/yarn.lock @@ -274,9 +274,9 @@ clone-response@^1.0.2: dependencies: mimic-response "^1.0.0" -code-oss-dev@coder/vscode#96e241330d9c44b64897c1e5031e00aa894103db: +code-oss-dev@coder/vscode#6337ee490d16b7dfd8854d22c998f58d6cd21ef5: version "1.63.0" - resolved "https://codeload.github.com/coder/vscode/tar.gz/96e241330d9c44b64897c1e5031e00aa894103db" + resolved "https://codeload.github.com/coder/vscode/tar.gz/6337ee490d16b7dfd8854d22c998f58d6cd21ef5" dependencies: "@microsoft/applicationinsights-web" "^2.6.4" "@parcel/watcher" "2.0.3"