test: docstring revisions [TESTENG-5] (#9478)

webui/react/src/e2e/README.md

@@ -1,37 +1,49 @@
 # E2E
 
+Our goals are to keep test cases realiable, stable, maintainable, and readable.
+
 ## Framework
 
 Deteremined AI uses [Playwright 🎭](https://playwright.dev/).
 
-## How to run locally
+## Setup
+
+Everything you need before running tests
+
+### `.env`
+
+Create `.env` file in `webui/react` like `webui/react/.env` and env variables. (`PW_` prefix stands for Playwright)
+
+- `PW_USER_NAME`: user name for determined account
+- `PW_PASSWORD`: password for determined account
+- `PW_SERVER_ADDRESS`: API server address
+- `PW_DET_PATH`: path to `det` if not already in path
+- `PW_DET_MASTER`: typically <http://localhost:8080>", used for CLI commands
+
+### Playwright
 
-- Create `.env` file in `webui/react` like `webui/react/.env`
-- Add env variables `PW_USER_NAME`, `PW_PASSWORD`, and `PW_SERVER_ADDRESS` (`PW_` prefix stands for Playwright)
-  - `PW_USER_NAME`: user name for determined account
-  - `PW_PASSWORD`: password for determined account
-  - `PW_SERVER_ADDRESS`: API server address
-  - `PW_DET_PATH`: path to `det` if not already in path
-  - `PW_DET_MASTER`: typically http://localhost:8080"
-- Run `npx playwright install`
-- Run `SERVER_ADDRESS={set server address} npm run build` in `webui/react`
-  - It is `SERVER_ADDRESS` here. not `PW_SERVER_ADDRESS`, but the both values should be the same
-- Run `npm run e2e` in `webui/react`. Use `-- --project=<browsername>` to run a specific browser.
+Run `npx playwright install`
 
-\*\*Avoid using `make` command because it does not take env variables
+### Determined
 
-### Quick start testing using det deploy
+Pick between live and static
 
-If you don't want to use dev cluster, you can use det deploy to initiate the backend. These commands should run and pass tests on chrome:
+#### Live Changes
 
-1. `det deploy local cluster-up --det-version="0.29.0" --no-gpu --master-port=8080`
-   - Use whatever det-version you want here.
+1. `npm run start` `--prefix webui/react`
+2. `conda activate det &&` `devcluster`
+
+#### Static Build and Cluster Up
+
+1. `det deploy local cluster-up --no-gpu --master-port=8080`
 2. `SERVER_ADDRESS="http://localhost:3001" npm run build --prefix webui/react`
-3. Optional if you want an experiment created for the test: `det experiment create ./examples/tutorials/mnist_pytorch/const.yaml ./examples/tutorials/mnist_pytorch/`
-4. Optional `npm run preview --prefix webui/react` to run the preview app. Won't be used if `CI=true`.
-   1. Consider running `npm run start --prefix webui/react -- --port=3001` for live changes if you're editing page models. The other command will constantly throw build errors if you're editing tests and test hooks at the same time. We use port `3001` because that's the port playwright is configured to use.
-5. To run the tests: `PW_SERVER_ADDRESS="http://localhost:3001"  PW_USER_NAME="admin" PW_PASSWORD="" npm run e2e --prefix webui/react`
-   - Provice `-- -p=firefox` to choose one browser to run on. Full list of projects located in [playwright config](/webui/react/playwright.config.ts).
+3. Optional `npm run preview --prefix webui/react` to run the preview app. Won't be used if `CI=true`.
+4. To run the tests: `PW_SERVER_ADDRESS="http://localhost:3001"  PW_USER_NAME="admin" PW_PASSWORD="" npm run e2e --prefix webui/react`
+   - Provide `-- -p=firefox` to choose one browser to run on. Full list of projects located in [playwright config](/webui/react/playwright.config.ts).
+
+## Run Tests
+
+Use the [VS Code extension](https://marketplace.visualstudio.com/items?itemName=ms-playwright.playwright) to run individual tests. You can also use the commandline, here are [the docs](https://playwright.dev/docs/running-tests).
 
 ## CI
 
@@ -41,9 +53,13 @@ We use `mcr.microsoft.com/playwright` for [docker container](https://playwright.
 Update the docker image version along with Playwright version.
 
 - PW_DET_PATH=/tmp/venv/bin/det"
-- PW_SERVER_ADDRESS=http://localhost:3001"
+- PW_SERVER_ADDRESS=<http://localhost:3001>"
 - PW_USER_NAME=admin
 - PW_PASSWORD=
-- PW_DET_MASTER=http://localhost:8082"
-- DET_WEBPACK_PROXY_URL=http://localhost:8082"
+- PW_DET_MASTER=<http://localhost:8082>"
+- DET_WEBPACK_PROXY_URL=<http://localhost:8082>"
 - DET_WEBSOCKET_PROXY_URL=ws://localhost:8082"
+
+## Appendix
+
+- [Page model Readme](./models/README.md)

webui/react/src/e2e/fixtures/auth.fixture.ts

@@ -22,28 +22,30 @@ export class AuthFixture {
   }
 
   async login({
-    waitForURL = /dashboard/,
+    expectedURL = /dashboard/,
     username = this.#USERNAME,
     password = this.#PASSWORD,
   }: {
-    waitForURL?: string | RegExp | ((url: URL) => boolean);
+    expectedURL?: string | RegExp | ((url: URL) => boolean);
     username?: string;
     password?: string;
   } = {}): Promise<void> {
     const detAuth = this.signInPage.detAuth;
     if (!(await detAuth.pwLocator.isVisible())) {
       await this.#page.goto('/');
       await expect(detAuth.pwLocator).toBeVisible();
+      await expect.soft(this.signInPage.detAuth.submit.pwLocator).toBeDisabled();
     }
     await detAuth.username.pwLocator.fill(username);
     await detAuth.password.pwLocator.fill(password);
     await detAuth.submit.pwLocator.click();
-    await this.#page.waitForURL(waitForURL);
+    await this.#page.waitForURL(expectedURL);
     // BUG [ET-239] can cause the following line to fail
   }
 
   async logout(): Promise<void> {
     await (await this.signInPage.nav.sidebar.headerDropdown.open()).signOut.pwLocator.click();
-    await this.#page.waitForURL(/login/);
+    await expect.soft(this.#page).toHaveTitle(this.signInPage.title);
+    await expect.soft(this.#page).toHaveURL(this.signInPage.getUrlRegExp());
   }
 }

webui/react/src/e2e/models/BaseComponent.ts

@@ -32,22 +32,27 @@ export type NamedComponentArgs =
   | NamedComponentWithAttachment;
 
 /**
- * Returns the representation of a Component.
- * This constructor is a base class for any component in src/components/.
- * @param {object} obj
- * @param {CanBeParent} obj.parent - The parent used to locate this BaseComponent
- * @param {string} obj.selector - Used as a selector uesd to locate this object
+ * Base model for any Component in src/components/
  */
 export class BaseComponent implements ComponentBasics {
   protected _selector: string;
   readonly _parent: CanBeParent;
   protected _locator: Locator | undefined;
 
+  /**
+   * Constructs a BaseComponent
+   * @param {object} obj
+   * @param {CanBeParent} obj.parent - parent component
+   * @param {string} obj.selector - identifier
+   */
   constructor({ parent, selector }: BaseComponentArgs) {
     this._selector = selector;
     this._parent = parent;
   }
 
+  /**
+   * The identifier used to locate this model
+   */
   get selector(): string {
     return this._selector;
   }
@@ -76,15 +81,17 @@ export class BaseComponent implements ComponentBasics {
 }
 
 /**
- * Returns the representation of a React Fragment.
- * React Fragment Components are special in that they group elements, but not under a dir.
- * Fragments cannot have selectors
- * @param {object} obj
- * @param {CanBeParent} obj.parent - The parent used to locate this BaseComponent
+ * BaseReactFragment will preserve the parent locator heirachy while also
+ * providing a way to group elements, just like the React Fragments they model.
  */
 export class BaseReactFragment implements ComponentBasics {
   readonly _parent: CanBeParent;
 
+  /**
+   * Constructs a BaseReactFragment
+   * @param {object} obj
+   * @param {CanBeParent} obj.parent - parent component
+   */
   constructor({ parent }: ComponentArgBasics) {
     this._parent = parent;
   }
@@ -110,36 +117,53 @@ export class BaseReactFragment implements ComponentBasics {
 }
 
 /**
- * Returns a representation of a named component. These components need a defaultSelector.
- * @param {object} obj
- * @param {CanBeParent} obj.parent - The parent used to locate this NamedComponent
- * @param {string} obj.selector - Used as a selector uesd to locate this object
+ * Named Components are components that have a default selector
  */
 export abstract class NamedComponent extends BaseComponent {
   abstract readonly defaultSelector: string;
   readonly #attachment: string;
 
+  /**
+   * The identifier used to locate this model
+   */
   override get selector(): string {
     return this._selector || this.defaultSelector + this.#attachment;
   }
 
-  static getSelector(args: NamedComponentArgs): { selector: string; attachment: string } {
+  /**
+   * Internal method used to compute the named component's selector
+   */
+  private static getSelector(args: NamedComponentArgs): { selector: string; attachment: string } {
     if (NamedComponent.isBaseComponentArgs(args))
       return { attachment: '', selector: args.selector };
     if (NamedComponent.isNamedComponentWithAttachment(args))
       return { attachment: args.attachment, selector: '' };
     else return { attachment: '', selector: '' };
   }
 
-  static isBaseComponentArgs(args: NamedComponentArgs): args is BaseComponentArgs {
+  /**
+   * Internal method to check the type of args passed into the constructor
+   */
+  private static isBaseComponentArgs(args: NamedComponentArgs): args is BaseComponentArgs {
     return 'selector' in args;
   }
 
-  static isNamedComponentWithAttachment(
+  /**
+   * Internal method to check the type of args passed into the constructor
+   */
+  private static isNamedComponentWithAttachment(
     args: NamedComponentArgs,
   ): args is NamedComponentWithAttachment {
     return 'attachment' in args;
   }
+
+  /**
+   * Constructs a NamedComponent
+   * @param {object} args
+   * @param {CanBeParent} args.parent - parent component
+   * @param {string} [args.selector] - identifier to be used in place of defaultSelector
+   * @param {string} [args.attachment] - identifier to be appended to defaultSelector
+   */
   constructor(args: NamedComponentArgs) {
     const { selector, attachment } = NamedComponent.getSelector(args);
     super({ parent: args.parent, selector });

webui/react/src/e2e/models/BasePage.ts

@@ -7,9 +7,7 @@ export interface ModelBasics {
 }
 
 /**
- * Returns the representation of a Page.
- * This constructor is a base class for any component in src/pages/.
- * @param {Page} page - The '@playwright/test' Page being used by a test
+ * Base model for any Page in src/pages/
  */
 export abstract class BasePage implements ModelBasics {
   readonly _page: Page;
@@ -23,6 +21,10 @@ export abstract class BasePage implements ModelBasics {
   abstract readonly url: string | RegExp;
   abstract readonly title: string | RegExp;
 
+  /**
+   * Constructs a BasePage
+   * @param {Page} page - Playwright's Page object
+   */
   constructor(page: Page) {
     this._page = page;
   }
@@ -61,7 +63,7 @@ export abstract class BasePage implements ModelBasics {
   async goto({
     url = this.url,
     verify = true,
-  }: { url?: string | RegExp; verify?: boolean } = {}): Promise<BasePage> {
+  }: { url?: string | RegExp; verify?: boolean } = {}): Promise<this> {
     if (url instanceof RegExp) {
       throw new Error(`${typeof this}.url is a regular expression. Please provide a url to visit.`);
     }