TINY-11177: Rewriting the entire report system to support batch resul…

…ts. Manual mode works, auto still needs some work.

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## Fixed
 - Reverted TINY-10708 which was a server-side fix
 - Reduced logging from client to server, and stopped waiting for log requests to complete between tests #TINY-11177
+- Removed server-side monitoring of single test timeouts. This is still monitored client side. #TINY-11177
 
 ## Removed
 - The Promise polyfill is no longer allowed on modern NodeJS frameworks so it has been removed. #TINY-11177

modules/runner/src/main/ts/reporter/Callbacks.ts

@@ -2,6 +2,15 @@ import { ErrorData, Global } from '@ephox/bedrock-common';
 
 import { HarnessResponse } from '../core/ServerTypes';
 
+export interface TestReport {
+  file: string;
+  name: string;
+  passed: boolean;
+  time: string;
+  skipped: string | null;
+  error: TestErrorData | null
+}
+
 export interface TestErrorData {
   readonly data: ErrorData;
   readonly text: string;
@@ -12,24 +21,33 @@ export interface Callbacks {
   readonly sendKeepAlive: (session: string) => Promise<void>;
   readonly sendInit: (session: string) => Promise<void>;
   readonly sendTestStart: (session: string, number: number, totalTests: number, file: string, name: string) => Promise<void>;
-  readonly sendTestResult: (session: string, file: string, name: string, passed: boolean, time: string, error: TestErrorData | null, skipped: string | null) => Promise<void>;
+  readonly sendTestResults: (session: string, results: TestReport[]) => Promise<void>;
   readonly sendDone: (session: string, error?: string) => Promise<void>;
 }
 
 declare const $: JQueryStatic;
 
-const sendJson = <T>(url: string, data: any): Promise<T> => {
+function generateErrorMessage(xhr: JQuery.jqXHR<any>, onError: (reason?: any) => void, url: string, requestDetails: string, statusText: 'timeout' | 'error' | 'abort' | 'parsererror', e: string) {
+  if (xhr.readyState === 0) {
+    onError(`Unable to open connection to ${url}, ${requestDetails}. Status text "${statusText}", error thrown "${e}"`);
+  } else {
+    onError(`Response status ${xhr.status} connecting to ${url}, ${requestDetails}. Status text "${statusText}", error thrown "${e}"`);
+  }
+}
+
+const sendJson = <T>(url: string, jsonData: any): Promise<T> => {
   return new Promise((onSuccess, onError) => {
+    const data = JSON.stringify(jsonData);
     $.ajax({
       method: 'post',
       url,
       contentType: 'application/json; charset=UTF-8',
       dataType: 'json',
       success: onSuccess,
       error: (xhr, statusText, e) => {
-        onError(e);
+        generateErrorMessage(xhr, onError, url, `sending ${data}`, statusText, e);
       },
-      data: JSON.stringify(data),
+      data,
     });
   });
 };
@@ -41,7 +59,7 @@ const getJson = <T>(url: string): Promise<T> => {
       dataType: 'json',
       success: onSuccess,
       error: (xhr, statusText, e) => {
-        onError(e);
+        generateErrorMessage(xhr, onError, url, 'as a get request', statusText, e);
       }
     });
   }));
@@ -74,15 +92,10 @@ export const Callbacks = (): Callbacks => {
     });
   };
 
-  const sendTestResult = (session: string, file: string, name: string, passed: boolean, time: string, error: TestErrorData | null, skipped: string | null): Promise<void> => {
-    return sendJson('/tests/result', {
+  const sendTestResults = (session: string, results: TestReport[]): Promise<void> => {
+    return sendJson('/tests/results', {
       session,
-      file,
-      name,
-      passed,
-      skipped,
-      time,
-      error,
+      results,
     });
   };
 
@@ -102,7 +115,7 @@ export const Callbacks = (): Callbacks => {
     sendInit,
     sendKeepAlive,
     sendTestStart,
-    sendTestResult,
+    sendTestResults,
     sendDone
   };
 };

modules/runner/src/main/ts/reporter/Reporter.ts

@@ -1,21 +1,22 @@
 import { LoggedError, Reporter as ErrorReporter } from '@ephox/bedrock-common';
-import { Callbacks } from './Callbacks';
+import { Callbacks, TestReport } from './Callbacks';
 import { UrlParams } from '../core/UrlParams';
 import { formatElapsedTime, mapStackTrace, setStack } from '../core/Utils';
 
 type LoggedError = LoggedError.LoggedError;
 
 export interface TestReporter {
-  readonly start: () => Promise<void>;
-  readonly retry: () => Promise<void>;
-  readonly pass: () => Promise<void>;
-  readonly skip: (reason: string) => Promise<void>;
-  readonly fail: (e: LoggedError) => Promise<void>;
+  readonly start: () => void;
+  readonly retry: () => void;
+  readonly pass: () => void;
+  readonly skip: (reason: string) => void;
+  readonly fail: (e: LoggedError) => void;
 }
 
 export interface Reporter {
   readonly summary: () => { offset: number; passed: number; failed: number; skipped: number };
   readonly test: (file: string, name: string, totalNumTests: number) => TestReporter;
+  readonly waitForResults: () => Promise<void>;
   readonly done: (error?: LoggedError) => void;
 }
 
@@ -41,7 +42,7 @@ const mapError = (e: LoggedError) => mapStackTrace(e.stack).then((mappedStack) =
     e.logs = logs.replace(originalStack, mappedStack).split('\n');
   }
 
-  return Promise.resolve(e);
+  return e;
 });
 
 export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi): Reporter => {
@@ -52,8 +53,18 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
   let skipCount = 0;
   let failCount = 0;
 
-  // A global list of reports that were sent to the server, we must wait for these before sending `/done` or it may confuse the HUD
-  const reportsInFlight: Promise<void>[] = [];
+  // A list of test results we are going to send as a batch to the server
+  const testResults: TestReport[] = [];
+
+  // A global list of requests that were sent to the server, we must wait for these before sending `/done` or it may confuse the HUD
+  const requestsInFlight: Promise<void>[] = [];
+
+  const sendCurrentResults = () => {
+    if (testResults.length > 0) {
+      requestsInFlight.push(callbacks.sendTestResults(params.session, testResults));
+      testResults.length = 0;
+    }
+  };
 
   const summary = () => ({
     offset: Math.max(0, currentCount - 1),
@@ -63,100 +74,99 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
   });
 
   const test = (file: string, name: string, totalNumTests: number) => {
-    let starttime: Date;
+    let starttime = new Date();
     let reported = false;
     let started = false;
     const testUi = ui.test();
 
-    // In order to prevent overloading the browser's parallel connection count, we only send start notifications when necessary.
-    // And when we do, we want any subsequent report to be blocked until the start notification has completed.
-    let startNotification: () => Promise<void>;
-
-    const start = (): Promise<void> => {
-      if (started) {
-        return Promise.resolve();
-      } else {
+    const start = (): void => {
+      if (!started) {
         started = true;
         starttime = new Date();
         currentCount++;
 
         testUi.start(file, name);
-        startNotification = () => callbacks.sendTestStart(params.session, currentCount, totalNumTests, file, name);
-
-        // once at test start and again every 50 tests (a number chosen without any particular reason)
-        if (currentCount === initialOffset + 1 || currentCount % 50 === 0) {
-          // run immediately and cache the result for use later
-          const callback = startNotification();
-          reportsInFlight.push(callback);
-          startNotification = () => callback;
+
+        if (currentCount === initialOffset + 1) {
+          // we need to send test start once to establish the session
+          const callback = callbacks.sendTestStart(params.session, currentCount, totalNumTests, file, name);
+          requestsInFlight.push(callback);
+        } else if (starttime.getTime() - initial.getTime() > 30 * 1000) {
+          // ping the server with results every 30 seconds or so, otherwise the result data could be gigantic
+          sendCurrentResults();
         }
-        // don't block, ever ever ever
-        return Promise.resolve();
       }
     };
 
-    const retry = (): Promise<void> => {
+    const retry = (): void => {
       starttime = new Date();
-      return Promise.resolve();
     };
 
-    const pass = (): Promise<void> => {
-      if (reported) {
-        return Promise.resolve();
-      } else {
+    const pass = (): void => {
+      if (!reported) {
         reported = true;
         passCount++;
         const testTime = elapsed(starttime);
 
         testUi.pass(testTime, currentCount);
-
-        // don't block, ever ever ever
-        return Promise.resolve();
+        testResults.push({
+          file,
+          name,
+          passed: true,
+          time: testTime,
+          error: null,
+          skipped: null,
+        });
       }
     };
 
-    const skip = (reason: string): Promise<void> => {
-      if (reported) {
-        return Promise.resolve();
-      } else {
+    const skip = (reason: string): void => {
+      if (!reported) {
         reported = true;
         skipCount++;
         const testTime = elapsed(starttime);
 
         testUi.skip(testTime, currentCount);
 
-        const report = startNotification().then(() =>
-           callbacks.sendTestResult(params.session, file, name, false, testTime, null, reason)
-        );
-        reportsInFlight.push(report);
-
-        // don't block, ever ever ever
-        return Promise.resolve();
+        testResults.push({
+          file,
+          name,
+          passed: false,
+          time: testTime,
+          error: null,
+          skipped: reason,
+        });
       }
     };
 
-    const fail = (e: LoggedError): Promise<void> => {
-      if (reported) {
-        return Promise.resolve();
-      } else {
+    const fail = (e: LoggedError): void => {
+      if (!reported) {
         reported = true;
         failCount++;
 
         const testTime = elapsed(starttime);
-        return mapError(e).then((err) => {
+
+        // `sourcemapped-stacktrace` is async, so we need to wait for it
+        requestsInFlight.push(mapError(e).then((err) => {
           const errorData = ErrorReporter.data(err);
           const error = {
             data: errorData,
             text: ErrorReporter.dataText(errorData)
           };
 
+          testResults.push({
+            file,
+            name,
+            passed: false,
+            time: testTime,
+            error,
+            skipped: null,
+          });
+
           testUi.fail(err, testTime, currentCount);
-          // make sure we have sent a `start` before we report a failure, otherwise the HUD goes all weird
-          // this can block, because failure data is critical for the console
-          return startNotification().then(() =>
-            callbacks.sendTestResult(params.session, file, name, false, testTime, error, null)
-          );
-        });
+
+          sendCurrentResults();
+        }));
       }
     };
 
@@ -169,6 +179,19 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
     };
   };
 
+  const waitForResults = (): Promise<void> => {
+    sendCurrentResults();
+    const currentRequests = requestsInFlight.slice(0);
+    requestsInFlight.length = 0;
+    Promise.all(currentRequests).then(() => {
+      // if more things have been queued, such as a failing test stack trace, wait for those as well
+      if (requestsInFlight.length !== 0) {
+        return waitForResults();
+      }
+    });
+    return Promise.resolve();
+  };
+
   const done = (error?: LoggedError): void => {
     const setAsDone = (): void => {
       const totalTime = elapsed(initial);
@@ -178,14 +201,15 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
     const textError = error !== undefined ? ErrorReporter.text(error) : undefined;
 
     // make sure any in progress updates are sent before we clean up
-    Promise.all(reportsInFlight).then(() =>
+    waitForResults().then(() =>
       callbacks.sendDone(params.session, textError).then(setAsDone, setAsDone)
     );
   };
 
   return {
     summary,
     test,
+    waitForResults,
     done
   };
 };

modules/runner/src/main/ts/runner/Runner.ts

@@ -53,27 +53,33 @@ export const Runner = (rootSuite: Suite, params: UrlParams, callbacks: Callbacks
     } else if (params.retry < retries) {
       retryTest(params.retry + 1);
     } else {
-      loadNextTest();
+      // show the failure for 1 second for the purposes of showing the failure on video
+      setTimeout(loadNextTest, 1000);
     }
   };
 
-  const init = (): Promise<HarnessResponse> => {
+  const init = async (): Promise<HarnessResponse> => {
     // Filter the tests to ensure we have an accurate total test count
     filterOnly(rootSuite);
     numTests = countTests(rootSuite);
 
     // Render the initial UI
     ui.render(params.offset, numTests, actions.restartTests, retryTest, loadNextTest);
 
-    // delay this ajax call until after the reporter status elements are in the page
-    const keepAliveTimer = setInterval(() => {
-      callbacks.sendKeepAlive(params.session).catch(() => {
-        // if the server shuts down stop trying to send messages
-        clearInterval(keepAliveTimer);
-      });
-    }, KEEP_ALIVE_INTERVAL);
-
-    return callbacks.sendInit(params.session).then(() => callbacks.loadHarness());
+    return Promise.all([callbacks.sendInit(params.session), callbacks.loadHarness()]).then(([_, harness]) => {
+      // we don't need a keep-alive timer in auto mode,
+      if (harness.mode === 'manual') {
+        // delay this ajax call until after the reporter status elements are in the page
+        const keepAliveTimer = setInterval(() => {
+          callbacks.sendKeepAlive(params.session).catch(() => {
+            // if the server shuts down stop trying to send messages
+            clearInterval(keepAliveTimer);
+          });
+        }, KEEP_ALIVE_INTERVAL);
+      }
+
+      return harness;
+    });
   };
 
   const run = (chunk: number, retries: number, timeout: number, stopOnFailure: boolean): Promise<void> => {