Final code review

Author: dprevost-LMI

docs/MultiRemote.md

@@ -4,7 +4,7 @@ Multi-remote support is in active development.
 
 ## Usage
 
-By default, multi-remote queries (e.g., `getTitle`) fetch data from all remotes, simplifying tests where browsers share behavior.
+By default, multi-remote matchers fetch data (e.g., `getTitle`) from all remotes, simplifying tests where browsers share the same behavior.
 
 Use the typed global constants:
 ```ts
@@ -37,6 +37,18 @@ export const config: WebdriverIO.MultiremoteConfig = {
 }
 ```
 
+And an `it` test like:
+```ts
+import { multiremotebrowser as multiRemoteBrowser } from '@wdio/globals' 
+
+it('should have title "My Site Title"', async function () {
+    await multiRemoteBrowser.url('https://mysite.com')
+    
+    // ... assertions
+})
+```            
+
+
 ## Single Expected Value
 To test all remotes against the same value, pass a single expected value.
 ```ts
@@ -80,7 +92,7 @@ To assert all remotes with a default value, overriding specific ones:
 - Options (e.g., `StringOptions`) apply globally.
 - Alpha support is limited to the `toHaveTitle` browser matcher.
 - Element matchers are planned.
-- Assertions currently throw on the first error. Future updates will report thrown errors as failures and if all remotes are in error it will throw.
+- Assertions currently throw on the first error. Future updates will report thrown errors as failures, and will only throw if all remotes fail.
 - SoftAssertions, snapshot services and network matchers might come after.
 
 ## Alternatives
@@ -95,25 +107,49 @@ Mocha Parameterized Example
     describe('Multiremote test', async () => {
         multiRemoteBrowser.instances.forEach(function (instance) {
             describe(`Test ${instance}`, function () {
-                it('should have title "The Internet"', async function () {
+                it('should have title "My Site Title"', async function () {
                     const browser = multiRemoteBrowser.getInstance(instance)
                     await browser.url('https://mysite.com')
 
-                    await expect(browser).toHaveTitle("The Internet")
+                    await expect(browser).toHaveTitle("My Site Title")
                 })
             })
         })
     })
 ```
-### Direct Instance Access
+### Direct Instance Access (TypeScript)
 By extending the WebdriverIO `namespace` in TypeScript (see [documentation](https://webdriver.io/docs/multiremote/#extending-typescript-types)), you can directly access each instance and use `expect` on them.
 
 ```ts
     it('should have title per browsers', async () => {
         await multiRemoteBrowser.url('https://mysite.com')
 
-        await expect(multiRemoteBrowser.myChromeBrowser).toHaveTitle('The Internet')
-        await expect(multiRemoteBrowser.myFirefoxBrowser).toHaveTitle('The Internet')
+        await expect(multiRemoteBrowser.myChromeBrowser).toHaveTitle('My Chrome Site Title')
+        await expect(multiRemoteBrowser.myFirefoxBrowser).toHaveTitle('My Firefox Site Title')
     }) 
 ```
 
+Required configuration:
+
+File `type.d.ts`
+```ts
+declare namespace WebdriverIO {
+    interface MultiRemoteBrowser {
+        myChromeBrowser: WebdriverIO.Browser
+        myFirefoxBrowser: WebdriverIO.Browser
+    }
+}
+```
+
+In `tsconfig.json`
+```json
+{
+  "compilerOptions": {
+    ...
+  },
+  "include": [
+    ...
+    "type.d.ts"
+  ]
+}
+```

src/matchers/browser/toHaveTitle.ts

@@ -1,7 +1,7 @@
-import { compareText, waitUntilResult } from '../../utils.js'
+import { compareText, waitUntilResultSucceed } from '../../utils.js'
 import { DEFAULT_OPTIONS } from '../../constants.js'
 import type { MaybeArray } from '../../util/multiRemoteUtil.js'
-import {  getInstancesWithExpected } from '../../util/multiRemoteUtil.js'
+import {  mapExpectedValueWithInstances } from '../../util/multiRemoteUtil.js'
 import { formatFailureMessage } from '../../util/formatMessage.js'
 
 type ExpectedValueType = string | RegExp | WdioAsymmetricMatcher<string>
@@ -33,17 +33,17 @@ export async function toHaveTitle(
         options,
     })
 
-    const browsers = getInstancesWithExpected(browser, expectedValue)
+    const browsersWithExpected = mapExpectedValueWithInstances(browser, expectedValue)
 
-    const conditions = Object.entries(browsers).map(([instance, { browser, expectedValue: expected }]) => async () => {
+    const conditions = Object.entries(browsersWithExpected).map(([instanceName, { browser, expectedValue: expected }]) => async () => {
         const actual = await browser.getTitle()
 
-        const result = compareText(actual, expected as ExpectedValueType, options)
-        result.instance = instance
+        const result = compareText(actual, expected, options)
+        result.instance = instanceName
         return result
     })
 
-    const conditionsResults = await waitUntilResult(
+    const conditionsResults = await waitUntilResultSucceed(
         conditions,
         isNot,
         options,

src/util/multiRemoteUtil.ts

@@ -4,15 +4,16 @@ export const toArray = <T>(value: T | T[] | MaybeArray<T>): T[] => (Array.isArra
 
 export type MaybeArray<T> = T | T[]
 
-export function isArray<T>(value: unknown): value is T[] {
-    return Array.isArray(value)
-}
-
 export const isMultiRemote = (browser: WebdriverIO.Browser | WebdriverIO.MultiRemoteBrowser): browser is WebdriverIO.MultiRemoteBrowser => {
     return (browser as WebdriverIO.MultiRemoteBrowser).isMultiremote === true
 }
 
-export const getInstancesWithExpected = <T>(browsers: WebdriverIO.Browser | WebdriverIO.MultiRemoteBrowser, expectedValues: T): Record<string, { browser: Browser; expectedValue: T; }>  => {
+type BrowserWithExpected<T> = Record<string, {
+    browser: Browser;
+    expectedValue: T;
+}>
+
+export const mapExpectedValueWithInstances = <T>(browsers: WebdriverIO.Browser | WebdriverIO.MultiRemoteBrowser, expectedValues: T | MaybeArray<T>): BrowserWithExpected<T> => {
     if (isMultiRemote(browsers)) {
         if (Array.isArray(expectedValues)) {
             if (expectedValues.length !== browsers.instances.length) {
@@ -21,15 +22,15 @@ export const getInstancesWithExpected = <T>(browsers: WebdriverIO.Browser | Webd
         }
         // TODO multi-remote support: add support for object like { default: 'title', browserA: 'titleA', browserB: 'titleB' } later
 
-        const browsersWithExpected = browsers.instances.reduce((acc, instance, index) => {
+        const browsersWithExpected = browsers.instances.reduce((acc: BrowserWithExpected<T>, instance, index) => {
             const browser = browsers.getInstance(instance)
-            const expectedValue = Array.isArray(expectedValues) ? expectedValues[index] : expectedValues
+            const expectedValue: T = Array.isArray(expectedValues) ? expectedValues[index] : expectedValues
             acc[instance] = { browser, expectedValue }
             return acc
-        }, {} as Record<string,  { browser: WebdriverIO.Browser, expectedValue: T }>)
+        }, {})
         return browsersWithExpected
     }
 
     // TODO multi-remote support: using default could clash if someone use name default, to review later
-    return { default: { browser: browsers, expectedValue: expectedValues } }
+    return { default: { browser: browsers, expectedValue: expectedValues as T } }
 }

src/utils.ts

@@ -49,14 +49,14 @@ function isStringContainingMatcher(expected: unknown): expected is WdioAsymmetri
  * @param isNot     https://jestjs.io/docs/expect#thisisnot
  * @param options   wait, interval, etc
  */
-const waitUntilResult = async <A = unknown, E = unknown>(
+const waitUntilResultSucceed = async <A = unknown, E = unknown>(
     condition: (() => Promise<CompareResult<A, E> | CompareResult<A, E>[]>) | (() => Promise<CompareResult<A, E>>)[],
     isNot = false,
     { wait = DEFAULT_OPTIONS.wait, interval = DEFAULT_OPTIONS.interval } = {},
 ): Promise<{ pass: boolean, results: CompareResult<A, E>[] }> => {
     /**
      * Using array algorithm to handle both single and multiple conditions uniformly
-     * Technically, this is an o(n3) operation, but practically, we process either a single promise with Array or an Array of promises. Review later if we can simplify and only have an array of promises
+     * Technically, this is an o(n3) operation, but practically, we process either a single promise returning Array or an Array of promises. Review later if we can simplify and only have an array of promises
      */
     const conditions = toArray(condition)
     // single attempt
@@ -257,7 +257,7 @@ export const compareText = (
                 expectedValue.toString() === 'StringContaining'
                     ? expect.stringContaining(expectedValue.sample?.toString().toLowerCase())
                     : expect.not.stringContaining(expectedValue.sample?.toString().toLowerCase())
-            ) as WdioAsymmetricMatcher<string>
+            ) satisfies Partial<WdioAsymmetricMatcher<string>> as WdioAsymmetricMatcher<string>
         }
     }
 
@@ -469,7 +469,7 @@ function aliasFn(
 
 export {
     aliasFn, compareNumbers, enhanceError, executeCommand,
-    executeCommandBe, numberError, waitUntil, waitUntilResult, wrapExpectedWithArray
+    executeCommandBe, numberError, waitUntil, waitUntilResultSucceed, wrapExpectedWithArray
 }
 
 function replaceActual(

test/util/multiRemoteUtil.test.ts

@@ -1,5 +1,5 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest'
-import { toArray, isArray, isMultiRemote, getInstancesWithExpected } from '../../src/util/multiRemoteUtil.js'
+import { toArray, isMultiRemote, mapExpectedValueWithInstances } from '../../src/util/multiRemoteUtil.js'
 
 describe('multiRemoteUtil', () => {
     describe(toArray, () => {
@@ -21,16 +21,6 @@ describe('multiRemoteUtil', () => {
         })
     })
 
-    describe(isArray, () => {
-        it('should return true if input is array', () => {
-            expect(isArray([1, 2])).toBe(true)
-        })
-
-        it('should return false if input is not array', () => {
-            expect(isArray(1)).toBe(false)
-        })
-    })
-
     describe(isMultiRemote, () => {
         it('should return true if browser is multi-remote', () => {
             const browser = { isMultiremote: true } satisfies Partial<WebdriverIO.MultiRemoteBrowser> as WebdriverIO.MultiRemoteBrowser
@@ -48,11 +38,11 @@ describe('multiRemoteUtil', () => {
         })
     })
 
-    describe(getInstancesWithExpected, () => {
+    describe(mapExpectedValueWithInstances, () => {
         it('should return default instance for single browser', () => {
             const browser = { isMultiremote: false } satisfies Partial<WebdriverIO.Browser> as WebdriverIO.Browser
             const expected = 'expected'
-            const result = getInstancesWithExpected(browser, expected)
+            const result = mapExpectedValueWithInstances(browser, expected)
             expect(result).toEqual({
                 default: {
                     browser,
@@ -78,7 +68,7 @@ describe('multiRemoteUtil', () => {
 
             it('should return instances for multi-remote browser with single expected value', () => {
                 const expected = 'expected'
-                const result = getInstancesWithExpected(browser, expected)
+                const result = mapExpectedValueWithInstances(browser, expected)
 
                 expect(result).toEqual({
                     browserA: {
@@ -96,7 +86,7 @@ describe('multiRemoteUtil', () => {
 
             it('should return instances for multi-remote browser with array of expected values', () => {
                 const expected = ['expectedA', 'expectedB']
-                const result = getInstancesWithExpected(browser, expected)
+                const result = mapExpectedValueWithInstances(browser, expected)
 
                 expect(result).toEqual({
                     browserA: {
@@ -112,7 +102,7 @@ describe('multiRemoteUtil', () => {
 
             it('should throw error if expected values length does not match instances length', () => {
                 const expected = ['expectedA']
-                expect(() => getInstancesWithExpected(browser, expected)).toThrow('Expected values length (1) does not match number of browser instances (2) in multi-remote setup.')
+                expect(() => mapExpectedValueWithInstances(browser, expected)).toThrow('Expected values length (1) does not match number of browser instances (2) in multi-remote setup.')
             })
         })
     })