Reduce flakiness and improve error handling

Ensure all onDisconnect handlers are called in the global afterEach
hook. When onDisconnect fails, we should associate it with the afterEach
of the failing test, not the beforeEach of the next test.

Drive-by: Fix error with E2E test of the beforeEach hook failed to
set-up the VE logging hook.

Bug: none
Change-Id: Ie7d92caf82f5262f838599cc2b324bf9f946acb3
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6237084
Commit-Queue: Nikolay Vitkov <[email protected]>
Reviewed-by: Simon Zünd <[email protected]>
Reviewed-by: Danil Somsikov <[email protected]>

Author: Lightning00Blade

front_end/testing/DOMHelpers.ts

@@ -55,23 +55,39 @@ function removeChildren(node: Node): void {
     node.removeChild(firstChild);
   }
 }
+
 /**
- * Completely cleans out the test DOM to ensure it's empty for the next test run.
- * This is run automatically between tests - you should not be manually calling this yourself.
+ * Sets up the DOM for testing,
+ * If not clean logs an error and cleans itself
  **/
-export const resetTestDOM = () => {
+export const setupTestDOM = async () => {
   const previousContainer = document.getElementById(TEST_CONTAINER_ID);
   if (previousContainer) {
-    removeChildren(previousContainer);
-    previousContainer.remove();
+    // This should not be reachable, unless the
+    // AfterEach hook fails before cleaning the DOM.
+    // Clean it here and report
+    console.error('Non clean test state found!');
+    await cleanTestDOM();
   }
-
   const newContainer = document.createElement('div');
   newContainer.id = TEST_CONTAINER_ID;
 
   document.body.appendChild(newContainer);
 };
 
+/**
+ * Completely cleans out the test DOM to ensure it's empty for the next test run.
+ * This is run automatically between tests - you should not be manually calling this yourself.
+ **/
+export const cleanTestDOM = async () => {
+  const previousContainer = document.getElementById(TEST_CONTAINER_ID);
+  if (previousContainer) {
+    removeChildren(previousContainer);
+    previousContainer.remove();
+  }
+  await raf();
+};
+
 interface Constructor<T> {
   new(...args: unknown[]): T;
 }

front_end/testing/MockConnection.ts

@@ -7,7 +7,6 @@ import type * as SDK from '../core/sdk/sdk.js';
 import type {ProtocolMapping} from '../generated/protocol-mapping.js';
 import type * as ProtocolProxyApi from '../generated/protocol-proxy-api.js';
 
-import {resetTestDOM} from './DOMHelpers.js';
 import {deinitializeGlobalVars, initializeGlobalVars} from './EnvironmentHelpers.js';
 import {setMockResourceTree} from './ResourceTreeHelpers.js';
 
@@ -138,7 +137,6 @@ async function disable() {
   if (outgoingMessageListenerEntryMap.size > 0) {
     throw new Error('MockConnection still has pending listeners. All promises should be awaited.');
   }
-  resetTestDOM();
   await deinitializeGlobalVars();
   // @ts-ignore Setting back to undefined as a hard reset.
   ProtocolClient.InspectorBackend.Connection.setFactory(undefined);

front_end/testing/test_setup.ts

@@ -12,16 +12,16 @@ import * as Trace from '../models/trace/trace.js';
 import * as Timeline from '../panels/timeline/timeline.js';
 import * as ThemeSupport from '../ui/legacy/theme_support/theme_support.js';
 
-import {resetTestDOM} from './DOMHelpers.js';
+import {cleanTestDOM, setupTestDOM} from './DOMHelpers.js';
 import {createFakeSetting} from './EnvironmentHelpers.js';
 import {
   checkForPendingActivity,
   startTrackingAsyncActivity,
   stopTrackingAsyncActivity,
 } from './TrackAsyncOperations.js';
 
-beforeEach(() => {
-  resetTestDOM();
+beforeEach(async () => {
+  await setupTestDOM();
   // Ensure that no trace data leaks between tests when testing the trace engine.
   for (const handler of Object.values(Trace.Handlers.ModelHandlers)) {
     handler.reset();
@@ -42,6 +42,7 @@ beforeEach(() => {
 });
 
 afterEach(async () => {
+  await cleanTestDOM();
   await checkForPendingActivity();
   sinon.restore();
   stopTrackingAsyncActivity();

front_end/ui/components/markdown_view/CodeBlock.test.ts

@@ -6,7 +6,6 @@ import * as Host from '../../../core/host/host.js';
 import {
   dispatchClickEvent,
   renderElementIntoDOM,
-  resetTestDOM,
 } from '../../../testing/DOMHelpers.js';
 import {describeWithEnvironment} from '../../../testing/EnvironmentHelpers.js';
 
@@ -44,33 +43,24 @@ describeWithEnvironment('CodeBlock', () => {
       assert.strictEqual(buttonContainer.textContent?.trim(), '');
     } finally {
       clock.restore();
-      resetTestDOM();
     }
   });
 
   it('renders no legal notice by default', () => {
-    try {
-      const component = new MarkdownView.CodeBlock.CodeBlock();
-      component.code = 'test';
-      renderElementIntoDOM(component);
-      const notice = component.shadowRoot!.querySelector('.notice') as HTMLElement;
-      assert(notice === null, '.notice was found');
-    } finally {
-      resetTestDOM();
-    }
+    const component = new MarkdownView.CodeBlock.CodeBlock();
+    component.code = 'test';
+    renderElementIntoDOM(component);
+    const notice = component.shadowRoot!.querySelector('.notice') as HTMLElement;
+    assert(notice === null, '.notice was found');
   });
 
   it('renders legal notice if configured', () => {
-    try {
-      const component = new MarkdownView.CodeBlock.CodeBlock();
-      component.code = 'test';
-      component.displayNotice = true;
-      renderElementIntoDOM(component);
-      const notice = component.shadowRoot!.querySelector('.notice') as HTMLElement;
-      assert.exists(notice);
-      assert.strictEqual(notice!.innerText, 'Use code snippets with caution');
-    } finally {
-      resetTestDOM();
-    }
+    const component = new MarkdownView.CodeBlock.CodeBlock();
+    component.code = 'test';
+    component.displayNotice = true;
+    renderElementIntoDOM(component);
+    const notice = component.shadowRoot!.querySelector('.notice') as HTMLElement;
+    assert.exists(notice);
+    assert.strictEqual(notice!.innerText, 'Use code snippets with caution');
   });
 });

front_end/ui/components/text_editor/TextEditor.ts

@@ -18,7 +18,6 @@ declare global {
 }
 
 export class TextEditor extends HTMLElement {
-
   readonly #shadow = this.attachShadow({mode: 'open'});
   #activeEditor: CodeMirror.EditorView|undefined = undefined;
   #dynamicSettings: readonly DynamicSetting<unknown>[] = DynamicSetting.none;
@@ -153,15 +152,14 @@ export class TextEditor extends HTMLElement {
     }
     this.#activeSettingListeners = [];
 
-    const settings = Common.Settings.Settings.instance();
     for (const dynamicSetting of dynamicSettings) {
       const handler = ({data}: {data: unknown}): void => {
         const change = dynamicSetting.sync(this.state, data);
         if (change && this.#activeEditor) {
           this.#activeEditor.dispatch({effects: change});
         }
       };
-      const setting = settings.moduleSetting(dynamicSetting.settingName);
+      const setting = Common.Settings.Settings.instance().moduleSetting(dynamicSetting.settingName);
       setting.addChangeListener(handler);
       this.#activeSettingListeners.push([setting, handler]);
     }

test/conductor/frontend_tab.ts

@@ -80,8 +80,12 @@ export class DevToolsFrontendTab {
     // Clear any local storage settings.
     await this.page.evaluate(() => {
       localStorage.clear();
-      // @ts-ignore Test logging needs debug event logging, which is controlled via localStorage, hence we need to restart test logging here
-      globalThis.setVeDebugLoggingEnabled(true, 'Test');
+
+      // Test logging needs debug event logging,
+      // which is controlled via localStorage, hence we need to restart test logging here
+      // This can be called after a page fails to load DevTools so make it conditional
+      // @ts-expect-error
+      globalThis?.setVeDebugLoggingEnabled(true, 'Test');
     });
     await this.reload();
   }