Add e2e tests, fix dial attempt stacking, add testing guidelines

CONTRIBUTING.md

@@ -53,3 +53,54 @@ make test
 make lint
 make format
 ```
+
+## Testing
+
+Our test suite is divided into unit tests and integration tests.
+
+### Unit Testing
+
+We use [Vitest](https://vitest.dev/) for unit testing to ensure that individual components of the SDK are well-tested in isolation.
+
+**Key Principles:**
+
+- **Location:** Unit tests are located in `src/**/__tests__`. Related test data and mocks are stored in adjacent `__fixtures__/` and `__mocks__/` directories, respectively.
+- **Isolation:** Tests must be independent. Use `vi.mock()` to mock external dependencies and ensure each test case can run on its own.
+- **Clarity:** Follow the Arrange-Act-Assert (AAA) pattern to structure tests clearly. Use descriptive names for `describe` blocks and test cases (e.g., `it('should do X when Y')`).
+
+You can run all unit tests with:
+
+```shell
+make test
+```
+
+### Integration Testing
+
+Integration tests verify the end-to-end interaction between the SDK and a live `viam-server`. We use [Vitest](https://vitest.dev/) for Node.js tests and [Playwright](https://playwright.dev/) for browser tests. All integration test code resides in the `e2e/` directory.
+
+**Key Principles:**
+
+- **File Naming:** Tests are separated by environment:
+  - `*.node.spec.ts` for Node.js-only tests.
+  - `*.browser.spec.ts` for browser-only tests.
+- **Browser Testing:** Browser tests interact with a UI test harness (`e2e/index.html`) via a Playwright Page Object Model (`e2e/fixtures/robot-page.ts`). This ensures tests are stable and maintainable.
+- **Node.js Testing:** Node.js tests interact with the SDK directly using a gRPC connection.
+
+Before running integration tests for the first time, you must download the `viam-server` binary:
+
+```shell
+cd e2e && ./setup.sh
+```
+
+You can run the full integration test suite with:
+
+```shell
+make test-e2e
+```
+
+You can also run the Node.js and browser suites separately:
+
+```shell
+npm run e2e:node
+npm run e2e:browser
+```

Makefile

@@ -95,15 +95,20 @@ build-docs: clean-docs build-buf
 
 .PHONY: install-playwright
 install-playwright:
-	cd e2e && npm install
-	cd e2e && npx playwright install --with-deps
+	npm run e2e:browser-install
 
 e2e/bin/viam-server:
 	bash e2e/setup.sh
 
-.PHONY: run-e2e-server
-run-e2e-server: e2e/bin/viam-server
-	e2e/bin/viam-server --config=./e2e/server_config.json
+.PHONY: test-e2e
+test-e2e: e2e/bin/viam-server install-playwright
+	npm run e2e:browser
+	npm run e2e:node
 
-test-e2e: e2e/bin/viam-server build install-playwright
-	cd e2e && npm run e2e:playwright
+.PHONY: test-e2e-node
+test-e2e-node: e2e/bin/viam-server 
+	npm run e2e:node
+
+.PHONY: test-e2e-browser
+test-e2e-browser: e2e/bin/viam-server install-playwright
+	npm run e2e:browser

e2e/fixtures/configs/base.json

@@ -0,0 +1,24 @@
+{
+  "network": {
+    "fqdn": "e2e-ts-sdk",
+    "bind_address": ":9090"
+  },
+  "components": [
+    {
+      "name": "base1",
+      "type": "base",
+      "model": "fake"
+    },
+    {
+      "name": "servo1",
+      "type": "servo",
+      "model": "fake"
+    },
+    {
+      "name": "test-motor",
+      "type": "motor",
+      "model": "fake"
+    }
+  ]
+}
+

e2e/fixtures/configs/dial-configs.ts

@@ -0,0 +1,31 @@
+import type { DialConf } from '../../main';
+
+const DEFAULT_HOST = 'e2e-ts-sdk';
+const DEFAULT_SERVICE_HOST = 'http://localhost:9090';
+const DEFAULT_SIGNALING_ADDRESS = 'http://localhost:9090';
+const DEFAULT_ICE_SERVERS = [{ urls: 'stun:global.stun.twilio.com:3478' }];
+
+export const defaultConfig: DialConf = {
+  host: DEFAULT_HOST,
+  serviceHost: DEFAULT_SERVICE_HOST,
+  signalingAddress: DEFAULT_SIGNALING_ADDRESS,
+  iceServers: DEFAULT_ICE_SERVERS,
+} as const;
+
+export const invalidConfig: DialConf = {
+  host: DEFAULT_HOST,
+  serviceHost: 'http://invalid-host:9999',
+  signalingAddress: DEFAULT_SIGNALING_ADDRESS,
+  iceServers: DEFAULT_ICE_SERVERS,
+  dialTimeout: 2000,
+} as const;
+
+export const defaultNodeConfig: DialConf = {
+  host: DEFAULT_SERVICE_HOST,
+  noReconnect: true,
+} as const;
+
+export const invalidNodeConfig: DialConf = {
+  host: 'http://invalid-host:9999',
+  noReconnect: true,
+} as const;

e2e/fixtures/robot-page.ts

@@ -0,0 +1,126 @@
+import { test as base, type Page } from '@playwright/test';
+import type { Robot, RobotClient } from '../../src/robot';
+import type { ResolvedReturnType } from '../helpers/api-types';
+
+export class RobotPage {
+  private readonly connectionStatusID = 'connection-status';
+  private readonly dialingStatusID = 'dialing-status';
+  private readonly connectButtonID = 'connect-btn';
+  private readonly disconnectButtonID = 'disconnect-btn';
+  private readonly outputID = 'output';
+
+  constructor(private readonly page: Page) {}
+
+  async ensureReady(): Promise<void> {
+    if (!this.page.url().includes('localhost:5173')) {
+      await this.page.goto('/');
+      await this.page.waitForSelector('body[data-ready="true"]');
+    }
+  }
+
+  async connect(): Promise<void> {
+    await this.ensureReady();
+    await this.page.getByTestId(this.connectButtonID).click();
+    await this.page.waitForSelector(
+      `[data-testid="${this.connectionStatusID}"]:is(:text("Connected"))`
+    );
+  }
+
+  async disconnect(): Promise<void> {
+    await this.page.getByTestId(this.disconnectButtonID).click();
+    await this.page.waitForSelector(
+      `[data-testid="${this.connectionStatusID}"]:is(:text("Disconnected"))`
+    );
+  }
+
+  async getConnectionStatus(): Promise<string> {
+    const connectionStatusEl = this.page.getByTestId(this.connectionStatusID);
+    const text = await connectionStatusEl.textContent();
+    return text ?? 'Unknown';
+  }
+
+  async waitForDialing(): Promise<void> {
+    await this.page.waitForSelector(
+      `[data-testid="${this.dialingStatusID}"]:not(:empty)`,
+      { timeout: 5000 }
+    );
+  }
+
+  async waitForFirstDialingAttempt(): Promise<void> {
+    await this.page.waitForFunction(
+      (testId: string) => {
+        const el = document.querySelector(`[data-testid="${testId}"]`);
+        const text = el?.textContent ?? '';
+        const match = text.match(/attempt (?<attemptNumber>\d+)/u);
+        if (!match?.groups) {
+          return false;
+        }
+        const attemptNumber = Number.parseInt(
+          match.groups.attemptNumber ?? '0',
+          10
+        );
+        return attemptNumber === 1;
+      },
+      this.dialingStatusID,
+      { timeout: 10_000 }
+    );
+  }
+
+  async waitForSubsequentDialingAttempts(): Promise<void> {
+    await this.page.waitForFunction(
+      (testId: string) => {
+        const el = document.querySelector(`[data-testid="${testId}"]`);
+        const text = el?.textContent ?? '';
+        const match = text.match(/attempt (?<attemptNumber>\d+)/u);
+        if (!match?.groups) {
+          return false;
+        }
+        const attemptNumber = Number.parseInt(
+          match.groups.attemptNumber ?? '0',
+          10
+        );
+        return attemptNumber > 1;
+      },
+      this.dialingStatusID,
+      { timeout: 10_000 }
+    );
+  }
+
+  async getDialingStatus(): Promise<string> {
+    const dialingStatusEl = this.page.getByTestId(this.dialingStatusID);
+    const text = await dialingStatusEl.textContent();
+    return text ?? '';
+  }
+
+  async getOutput<T extends Robot, K extends keyof T>(): Promise<
+    ResolvedReturnType<T[K]>
+  > {
+    // Wait for the output to be updated by checking for the data-has-output attribute
+    await this.page.waitForSelector(
+      `[data-testid="${this.outputID}"][data-has-output="true"]`,
+      { timeout: 30_000 }
+    );
+    const outputEl = this.page.getByTestId(this.outputID);
+    const text = await outputEl.textContent();
+    return JSON.parse(text ?? '{}') as ResolvedReturnType<T[K]>;
+  }
+
+  async clickButton(testId: string): Promise<void> {
+    await this.page.click(`[data-testid="${testId}"]`);
+  }
+
+  async clickRobotAPIButton(apiName: keyof RobotClient): Promise<void> {
+    await this.page.click(`[data-robot-api="${apiName}"]`);
+  }
+
+  getPage(): Page {
+    return this.page;
+  }
+}
+
+export const withRobot = base.extend<{ robotPage: RobotPage }>({
+  robotPage: async ({ page }, use) => {
+    const robotPage = new RobotPage(page);
+    await use(robotPage);
+  },
+});

e2e/helpers/api-types.ts

@@ -0,0 +1,9 @@
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export type ArgumentsType<T> = T extends (...args: infer U) => any ? U : never;
+
+export type ResolvedReturnType<T> = T extends (
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  ...args: any[]
+) => Promise<infer R>
+  ? R
+  : never;

e2e/helpers/global-setup.ts

@@ -0,0 +1,113 @@
+/* eslint-disable no-console, no-await-in-loop */
+import { spawn, type ChildProcess } from 'node:child_process';
+import path from 'node:path';
+import url from 'node:url';
+import fs from 'node:fs';
+
+const dirname = path.dirname(url.fileURLToPath(import.meta.url));
+
+let serverProcess: ChildProcess | undefined;
+
+const VIAM_SERVER_PORT = 9090;
+const VIAM_SERVER_HOST = 'localhost';
+const VIAM_SERVER_FQDN = 'e2e-ts-sdk';
+
+/**
+ * Wait for the viam-server to be ready by checking if the port is accepting
+ * connections.
+ */
+const waitForServer = async (
+  host: string,
+  port: number,
+  maxAttempts = 30
+): Promise<void> => {
+  for (let i = 0; i < maxAttempts; i += 1) {
+    try {
+      const response = await fetch(`http://${host}:${port}/`);
+      if (response.ok) {
+        console.log(`✓ viam-server is ready at ${host}:${port}`);
+        return;
+      }
+    } catch {
+      await new Promise((resolve) => {
+        setTimeout(resolve, 1000);
+      });
+    }
+  }
+  throw new Error(`viam-server failed to start within ${maxAttempts} seconds`);
+};
+
+/** Global setup function - starts viam-server before all tests */
+export const setup = async (): Promise<void> => {
+  console.log('Starting viam-server for E2E tests...');
+
+  const binaryPath = path.resolve(dirname, '../bin/viam-server');
+  if (!fs.existsSync(binaryPath)) {
+    throw new Error(
+      `viam-server binary not found at ${binaryPath}. Run 'cd e2e && ./setup.sh' to download it.`
+    );
+  }
+
+  const configPath = path.resolve(dirname, '../fixtures/configs/base.json');
+  if (!fs.existsSync(configPath)) {
+    throw new Error(`Test robot config not found at ${configPath}`);
+  }
+
+  serverProcess = spawn(binaryPath, ['-config', configPath], {
+    stdio: ['ignore', 'pipe', 'pipe'],
+    detached: false,
+  });
+
+  serverProcess.stdout?.on('data', (data) => {
+    console.log(`[viam-server]: ${String(data).trim()}`);
+  });
+
+  serverProcess.stderr?.on('data', (data) => {
+    console.error(`[viam-server ERROR]: ${String(data).trim()}`);
+  });
+
+  serverProcess.on('error', (error) => {
+    console.error('Failed to start viam-server:', error);
+    throw error;
+  });
+
+  serverProcess.on('exit', (code, signal) => {
+    if (code !== 0 && code !== null) {
+      console.error(`viam-server exited with code ${code}`);
+    }
+    if (signal) {
+      console.log(`viam-server killed with signal ${signal}`);
+    }
+  });
+
+  await waitForServer(VIAM_SERVER_HOST, VIAM_SERVER_PORT);
+
+  process.env.VIAM_SERVER_HOST = VIAM_SERVER_HOST;
+  process.env.VIAM_SERVER_PORT = String(VIAM_SERVER_PORT);
+  process.env.VIAM_SERVER_FQDN = VIAM_SERVER_FQDN;
+  process.env.VIAM_SERVER_URL = `http://${VIAM_SERVER_HOST}:${VIAM_SERVER_PORT}`;
+
+  console.log('✓ Global setup complete');
+};
+
+/** Global teardown function - stops viam-server after all tests */
+export const teardown = async (): Promise<void> => {
+  console.log('Stopping viam-server...');
+
+  if (serverProcess) {
+    const exitPromise = new Promise<void>((resolve) => {
+      serverProcess?.on('exit', () => {
+        serverProcess = undefined;
+        resolve();
+      });
+    });
+
+    serverProcess.kill('SIGTERM');
+    await exitPromise;
+  }
+
+  console.log('✓ Global teardown complete');
+};
+
+// For Playwright, which expects a default export
+export default setup;