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.

Author: code-asher

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)
+  })
 })

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<CodeServerProcess> | undefined
   public readonly logger: Logger
   private closed = false
+  private _workspaceDir: Promise<string> | 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<string> {
+    if (!this._workspaceDir) {
+      this._workspaceDir = tmpdir(workspaceDir)
+    }
+    return this._workspaceDir
+  }
+
   /**
    * Create a random workspace and seed it with settings.
    */
   private async createWorkspace(): Promise<string> {
-    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.
+   */
+  get workspaceDir() {
+    return this.codeServer.workspaceDir
+  }
+
+  /**
+   * Navigate to a code-server endpoint.  By default go to the root.
    */
-  async navigate() {
-    const address = await this.codeServer.address()
-    await this.page.goto(address, { waitUntil: "networkidle" })
+  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<void> {
+    return this.page.waitForSelector(`.tab :text("${path.basename(file)}")`)
+  }
+
+  /**
+   * See if the specified tab is open.
+   */
+  async tabIsVisible(file: string): Promise<void> {
+    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,66 +335,127 @@ 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<unknown>> = [() => this.page.waitForSelector(`${menuSelector}:focus-within`)]
-      for (const menu of menus) {
+  async navigateItems(items: string[], selector: string, open?: (selector: string) => void): Promise<void> {
+    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<unknown>; 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,
         // potentially conflicting with the new set of navigations (for example
         // 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")}`)
+  }
+
+  /**
+   * Navigate through a currently opened "quick input" widget, retrying on
+   * failure.
+   */
+  async navigateQuickInput(items: string[]): Promise<void> {
+    await this.navigateItems(items, ".quick-input-widget")
+  }
 
-    this.codeServer.logger.debug(`menu navigation retries: ${retryCount}`)
+  /**
+   * Navigate through the menu, retrying on failure.
+   */
+  async navigateMenus(menus: string[]): Promise<void> {
+    await this.navigateItems(menus, '[aria-label="Application Menu"]', async (selector) => {
+      await this.page.click(selector)
+    })
   }
 
   /**
    * Navigates to code-server then reloads until the editor is ready.
    *
    * 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) {

vendor/package.json

@@ -7,6 +7,6 @@
     "postinstall": "./postinstall.sh"
   },
   "devDependencies": {
-    "code-oss-dev": "coder/vscode#96e241330d9c44b64897c1e5031e00aa894103db"
+    "code-oss-dev": "coder/vscode#6337ee490d16b7dfd8854d22c998f58d6cd21ef5"
   }
 }

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"