fix: Improve node ID check robustness & WebdriverIO test CI stability (#745)

BenHenning · web-flow · commit 0888bc211af8 · 2025-09-25T15:57:40.000-07:00
RaspberryPiFoundation/blockly#9384 revealed that one of the Block comment tests was a bit fragile in directly validating a specific node ID. Since node IDs can change if new elements on the page require a generated ID, the test could easily fail with changes to core Blockly. This PR fixes the issue by using a regex check instead of a direct ID match. The 'comment_textarea' part is _probably_ sufficiently unique for the test to be considered correct (though it is possible for the wrong comment to be focused here, but it seems really unlikely with how simple the test is being set up and, if that actually happened, it's sort of incidentally still correct as there would be _some_ comment that has focus). This PR also includes a number of attempts to improve test stability (particularly in CI): - It introduces a CI-specific command that adds a 30 second Mocha test timeout to provide a lot more time for tests to fail (since CI tends to run on less performant machines than used for development). - It introduces a synchronization between the Mocha timeout and the timeout used for WebdriverIO's `waitFor*` functions. Note that this actually will usually be 60 seconds regardless of the configured Mocha timeout due to the way the hook is set up. However, this is still far better than the 5 second default as at least 1 test seemed to fairly regularly fail against that limit. - It adds a few missing 'wait' calls for helpers that are performing side effects which ensures better synchronization when using `PAUSE_TIME` (and possibly better stability when running in CI, too, though it's unclear of `pause()` actually does anything when the provided timeout is 0). - See #745 (comment) attempts to further stabilize OS X tests only to yield that there are actual test failures. This will be merged into the screen reader experimental branch to unblock the core Blockly PR as well. #746 has been filed to track the discovered test failures.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -63,7 +63,7 @@ jobs:
       - name: Run tests
         run: |
           cd main
-          npm run test
+          npm run test:ci
 
   webdriverio_tests:
     name: WebdriverIO tests (against pinned v12)
@@ -90,4 +90,4 @@ jobs:
         run: npm install
 
       - name: Run tests
-        run: npm run test
+        run: npm run test:ci
diff --git a/package.json b/package.json
@@ -15,13 +15,16 @@
     "prepublishOnly": "npm login --registry https://wombat-dressing-room.appspot.com",
     "start": "blockly-scripts start",
     "test": "npm run test:mocha && npm run test:wdio",
+    "test:ci": "npm run test:mocha && npm run test:wdio:ci",
     "test:mocha": "blockly-scripts test",
     "test:wdio": "npm run wdio:clean && npm run wdio:run",
+    "test:wdio:ci": "npm run wdio:clean && npm run wdio:run:ci",
     "wdio:build": "npm run wdio:build:app && npm run wdio:build:tests",
     "wdio:build:app": "cd test/webdriverio && webpack",
     "wdio:build:tests": "tsc -p ./test/webdriverio/test/tsconfig.json",
     "wdio:clean": "cd test/webdriverio/test && rm -rf dist",
-    "wdio:run": "npm run wdio:build && cd test/webdriverio/test && npx mocha dist"
+    "wdio:run": "npm run wdio:build && cd test/webdriverio/test && npx mocha dist",
+    "wdio:run:ci": "npm run wdio:build && cd test/webdriverio/test && npx mocha --timeout 30000 dist"
   },
   "main": "./dist/index.js",
   "module": "./src/index.js",
diff --git a/test/webdriverio/test/actions_test.ts b/test/webdriverio/test/actions_test.ts
@@ -92,12 +92,10 @@ suite('Menus test', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    // This is the first test suite, which must wait for Chrome +
-    // chromedriver to start up, which can be slow—perhaps a few
-    // seconds.  Allow 30s just in case.
-    this.timeout(30000);
-
-    this.browser = await testSetup(testFileLocations.MORE_BLOCKS);
+    this.browser = await testSetup(
+      testFileLocations.MORE_BLOCKS,
+      this.timeout(),
+    );
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/basic_test.ts b/test/webdriverio/test/basic_test.ts
@@ -31,7 +31,10 @@ suite('Keyboard navigation on Blocks', function () {
 
   // Clear the workspace and load start blocks.
   suiteSetup(async function () {
-    this.browser = await testSetup(testFileLocations.NAVIGATION_TEST_BLOCKS);
+    this.browser = await testSetup(
+      testFileLocations.NAVIGATION_TEST_BLOCKS,
+      this.timeout(),
+    );
   });
 
   test('Default workspace', async function () {
@@ -226,7 +229,10 @@ suite('Keyboard navigation on Fields', function () {
 
   // Clear the workspace and load start blocks.
   suiteSetup(async function () {
-    this.browser = await testSetup(testFileLocations.NAVIGATION_TEST_BLOCKS);
+    this.browser = await testSetup(
+      testFileLocations.NAVIGATION_TEST_BLOCKS,
+      this.timeout(),
+    );
   });
 
   test('Up from first field selects block', async function () {
diff --git a/test/webdriverio/test/block_comment_test.ts b/test/webdriverio/test/block_comment_test.ts
@@ -23,7 +23,10 @@ suite('Block comment navigation', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.NAVIGATION_TEST_BLOCKS);
+    this.browser = await testSetup(
+      testFileLocations.NAVIGATION_TEST_BLOCKS,
+      this.timeout(),
+    );
     await this.browser.execute(() => {
       Blockly.getMainWorkspace()
         .getBlockById('p5_canvas_1')
@@ -36,7 +39,8 @@ suite('Block comment navigation', function () {
     await keyRight(this.browser);
     await sendKeyAndWait(this.browser, Key.Enter);
     const focusedNodeId = await getCurrentFocusNodeId(this.browser);
-    chai.assert.equal(focusedNodeId, 'blockly-2s_comment_textarea_');
+    chai.assert.isDefined(focusedNodeId);
+    chai.assert.match(focusedNodeId, /blockly-\w+_comment_textarea_/);
   });
 
   test('Escape from a focused comment focuses its bubble', async function () {
diff --git a/test/webdriverio/test/clipboard_test.ts b/test/webdriverio/test/clipboard_test.ts
@@ -27,7 +27,7 @@ suite('Clipboard test', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.BASE);
+    this.browser = await testSetup(testFileLocations.BASE, this.timeout());
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/delete_test.ts b/test/webdriverio/test/delete_test.ts
@@ -27,7 +27,10 @@ suite('Deleting Blocks', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.NAVIGATION_TEST_BLOCKS);
+    this.browser = await testSetup(
+      testFileLocations.NAVIGATION_TEST_BLOCKS,
+      this.timeout(),
+    );
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/duplicate_test.ts b/test/webdriverio/test/duplicate_test.ts
@@ -23,7 +23,7 @@ suite('Duplicate test', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.BASE);
+    this.browser = await testSetup(testFileLocations.BASE, this.timeout());
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/flyout_test.ts b/test/webdriverio/test/flyout_test.ts
@@ -28,7 +28,7 @@ suite('Toolbox and flyout test', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.BASE);
+    this.browser = await testSetup(testFileLocations.BASE, this.timeout());
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/hooks.ts b/test/webdriverio/test/hooks.ts
@@ -16,7 +16,7 @@ export const mochaHooks: RootHookObject = {
   async beforeAll(this: Mocha.Context) {
     // Set a long timeout for startup.
     this.timeout(60000);
-    return await driverSetup();
+    return await driverSetup(this.timeout());
   },
   async afterAll() {
     return await driverTeardown();
diff --git a/test/webdriverio/test/insert_test.ts b/test/webdriverio/test/insert_test.ts
@@ -28,7 +28,7 @@ suite('Insert test', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.BASE);
+    this.browser = await testSetup(testFileLocations.BASE, this.timeout());
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/keyboard_mode_test.ts b/test/webdriverio/test/keyboard_mode_test.ts
@@ -32,7 +32,10 @@ suite(
 
     setup(async function () {
       // Reload the page between tests
-      this.browser = await testSetup(testFileLocations.NAVIGATION_TEST_BLOCKS);
+      this.browser = await testSetup(
+        testFileLocations.NAVIGATION_TEST_BLOCKS,
+        this.timeout(),
+      );
 
       await this.browser.pause(PAUSE_TIME);
 
diff --git a/test/webdriverio/test/move_test.ts b/test/webdriverio/test/move_test.ts
@@ -25,7 +25,10 @@ suite('Move start tests', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.MOVE_START_TEST_BLOCKS);
+    this.browser = await testSetup(
+      testFileLocations.MOVE_START_TEST_BLOCKS,
+      this.timeout(),
+    );
     await this.browser.pause(PAUSE_TIME);
   });
 
@@ -186,6 +189,7 @@ suite('Statement move tests', function () {
   setup(async function () {
     this.browser = await testSetup(
       testFileLocations.MOVE_STATEMENT_TEST_BLOCKS,
+      this.timeout(),
     );
     await this.browser.pause(PAUSE_TIME);
   });
@@ -468,6 +472,7 @@ suite(`Value expression move tests`, function () {
           createTestUrl(
             new URLSearchParams({renderer, scenario: 'moveValueTestBlocks'}),
           ),
+          this.timeout(),
         );
         await this.browser.pause(PAUSE_TIME);
       });
diff --git a/test/webdriverio/test/mutator_test.ts b/test/webdriverio/test/mutator_test.ts
@@ -26,7 +26,10 @@ suite('Mutator navigation', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.NAVIGATION_TEST_BLOCKS);
+    this.browser = await testSetup(
+      testFileLocations.NAVIGATION_TEST_BLOCKS,
+      this.timeout(),
+    );
     this.openMutator = async () => {
       await focusOnBlock(this.browser, 'controls_if_1');
       await this.browser.pause(PAUSE_TIME);
diff --git a/test/webdriverio/test/scroll_test.ts b/test/webdriverio/test/scroll_test.ts
@@ -25,7 +25,7 @@ suite('Scrolling into view', function () {
   //
   // N.B. that this is called only one per suite, not once per test.
   suiteSetup(async function () {
-    this.browser = await testSetup(testFileLocations.BASE);
+    this.browser = await testSetup(testFileLocations.BASE, this.timeout());
     this.windowSize = await this.browser.getWindowSize();
     await this.browser.setWindowSize(800, 600);
     await this.browser.pause(PAUSE_TIME);
@@ -41,7 +41,7 @@ suite('Scrolling into view', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    await testSetup(testFileLocations.BASE);
+    await testSetup(testFileLocations.BASE, this.timeout());
   });
 
   test('Insert scrolls new block into view', async function () {
diff --git a/test/webdriverio/test/stack_navigation.ts b/test/webdriverio/test/stack_navigation.ts
@@ -18,7 +18,7 @@ import {
 suite('Stack navigation', function () {
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.COMMENTS);
+    this.browser = await testSetup(testFileLocations.COMMENTS, this.timeout());
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/styling_test.ts b/test/webdriverio/test/styling_test.ts
@@ -23,7 +23,7 @@ suite('Styling test', function () {
 
   // Clear the workspace and load start blocks.
   setup(async function () {
-    this.browser = await testSetup(testFileLocations.BASE);
+    this.browser = await testSetup(testFileLocations.BASE, this.timeout());
     await this.browser.pause(PAUSE_TIME);
   });
 
diff --git a/test/webdriverio/test/test_setup.ts b/test/webdriverio/test/test_setup.ts
@@ -49,10 +49,14 @@ export const PAUSE_TIME = 0;
  * done once, to avoid constantly popping browser windows open and
  * closed.
  *
+ * @param wdioWaitTimeoutMs The timeout for WebdriverIO waitFor*() commands, in
+ *     milliseconds.
  * @returns A Promise that resolves to a WebdriverIO browser that
  *     tests can manipulate.
  */
-export async function driverSetup(): Promise<webdriverio.Browser> {
+export async function driverSetup(
+  wdioWaitTimeoutMs: number,
+): Promise<webdriverio.Browser> {
   const options = {
     capabilities: {
       'browserName': 'chrome',
@@ -66,6 +70,7 @@ export async function driverSetup(): Promise<webdriverio.Browser> {
       // eslint-disable-next-line @typescript-eslint/naming-convention
       'wdio:enforceWebDriverClassic': true,
     },
+    waitforTimeout: wdioWaitTimeoutMs,
     logLevel: 'warn' as const,
   };
 
@@ -104,20 +109,24 @@ export async function driverTeardown() {
  *
  * @param playgroundUrl The URL to open for the test, which should be
  *     a Blockly playground with a workspace.
+ * @param wdioWaitTimeoutMs The timeout for WebdriverIO waitFor*() commands, in
+ *     milliseconds. This should generally be synchronized with the default
+ *     Mocah test timeout.
  * @returns A Promise that resolves to a WebdriverIO browser that
  *     tests can manipulate.
  */
 export async function testSetup(
   playgroundUrl: string,
+  wdioWaitTimeoutMs: number,
 ): Promise<webdriverio.Browser> {
   if (!driver) {
-    driver = await driverSetup();
+    driver = await driverSetup(wdioWaitTimeoutMs);
   }
   await driver.url(playgroundUrl);
   // Wait for the workspace to exist and be rendered.
   await driver
     .$('.blocklySvg .blocklyWorkspace > .blocklyBlockCanvas')
-    .waitForExist({timeout: 2000});
+    .waitForExist();
   return driver;
 }
 
@@ -193,6 +202,7 @@ export async function focusWorkspace(browser: WebdriverIO.Browser) {
     '#blocklyDiv > div > svg.blocklySvg > g',
   );
   await workspaceElement.click({x: 100});
+  await browser.pause(PAUSE_TIME);
 }
 
 /**
@@ -205,7 +215,7 @@ export async function moveToToolboxCategory(
   browser: WebdriverIO.Browser,
   category: string,
 ) {
-  await browser.keys('t');
+  await sendKeyAndWait(browser, 't');
   const categoryIndex = await browser.execute((category) => {
     const all = Array.from(
       document.querySelectorAll('.blocklyToolboxCategoryLabel'),
@@ -275,12 +285,13 @@ export async function focusOnBlock(
   browser: WebdriverIO.Browser,
   blockId: string,
 ) {
-  return await browser.execute((blockId) => {
+  await browser.execute((blockId) => {
     const workspaceSvg = Blockly.getMainWorkspace() as Blockly.WorkspaceSvg;
     const block = workspaceSvg.getBlockById(blockId);
     if (!block) throw new Error(`No block found with ID: ${blockId}.`);
     Blockly.getFocusManager().focusNode(block);
   }, blockId);
+  await browser.pause(PAUSE_TIME);
 }
 
 /**
@@ -295,14 +306,15 @@ export async function focusOnWorkspaceComment(
   browser: WebdriverIO.Browser,
   commentId: string,
 ) {
-  return await browser.execute((commentId) => {
+  await browser.execute((commentId) => {
     const workspaceSvg = Blockly.getMainWorkspace() as Blockly.WorkspaceSvg;
     const comment = workspaceSvg.getCommentById(commentId);
     if (!comment) {
       throw new Error(`No workspace comment found with ID: ${commentId}.`);
     }
     Blockly.getFocusManager().focusNode(comment);
   }, commentId);
+  await browser.pause(PAUSE_TIME);
 }
 
 /**
@@ -320,7 +332,7 @@ export async function focusOnBlockField(
   blockId: string,
   fieldName: string,
 ) {
-  return await browser.execute(
+  await browser.execute(
     (blockId, fieldName) => {
       const workspaceSvg = Blockly.getMainWorkspace() as Blockly.WorkspaceSvg;
       const block = workspaceSvg.getBlockById(blockId);
@@ -334,6 +346,7 @@ export async function focusOnBlockField(
     blockId,
     fieldName,
   );
+  await browser.pause(PAUSE_TIME);
 }
 
 /**
@@ -466,6 +479,7 @@ export async function tabNavigateToWorkspace(
   // there's no straightforward way to do that; see
   // https://stackoverflow.com/q/51518855/4969945
   await browser.execute(() => document.getElementById('focusableDiv')?.focus());
+  await browser.pause(PAUSE_TIME);
   // Navigate to workspace.
   if (hasToolbox) await tabNavigateForward(browser);
   if (hasFlyout) await tabNavigateForward(browser);
@@ -639,7 +653,7 @@ export async function contextMenuExists(
   const item = await browser.$(
     `//div[contains(@class, "blocklyMenuItem")]//*[text()="${itemText}"]`,
   );
-  return await item.waitForExist({timeout: 200, reverse: reverse});
+  return await item.waitForExist({reverse: reverse});
 }
 
 /**
@@ -711,11 +725,13 @@ export async function clickBlock(
     blockId,
     findableId,
   );
+  await browser.pause(PAUSE_TIME);
 
   // In the test context, get the WebdriverIO Element that we've identified.
   const elem = await browser.$(`#${findableId}`);
 
   await elem.click(clickOptions);
+  await browser.pause(PAUSE_TIME);
 
   // In the browser context, remove the ID.
   await browser.execute((elemId) => {
@@ -735,4 +751,5 @@ export async function rightClickOnFlyoutBlockType(
 ) {
   const elem = await browser.$(`.blocklyFlyout .${blockType}`);
   await elem.click({button: 'right'});
+  await browser.pause(PAUSE_TIME);
 }
diff --git a/test/webdriverio/test/toast_test.ts b/test/webdriverio/test/toast_test.ts
diff --git a/test/webdriverio/test/workspace_comment_test.ts b/test/webdriverio/test/workspace_comment_test.ts
