TINY-11177: Vastly improve remote testing

CHANGELOG.md

@@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+
+## 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
+- The Promise polyfill is no longer allowed on modern NodeJS frameworks so it has been removed. #TINY-11177
+
 ## 14.1.4 - 2024-03-27
 
 ### Fixed

modules/runner/package.json

@@ -11,7 +11,6 @@
   },
   "dependencies": {
     "@ephox/bedrock-common": "^14.1.1",
-    "@ephox/wrap-promise-polyfill": "^2.2.0",
     "jquery": "^3.4.1",
     "querystringify": "^2.1.1"
   },

modules/runner/src/main/ts/api/Main.ts

@@ -1,5 +1,4 @@
 import { Failure, Global } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import * as Globals from '../core/Globals';
 import * as TestLoader from '../core/TestLoader';
 import { UrlParams } from '../core/UrlParams';

modules/runner/src/main/ts/core/TestLoader.ts

@@ -1,4 +1,3 @@
-import Promise from '@ephox/wrap-promise-polyfill';
 import { ErrorCatcher } from '../errors/ErrorCatcher';
 
 export const load = (scriptUrl: string): Promise<void> =>
@@ -38,4 +37,4 @@ export const load = (scriptUrl: string): Promise<void> =>
 
     // Add the script to the dom to load it
     document.body.appendChild(script);
-  });
+  });

modules/runner/src/main/ts/core/Utils.ts

@@ -1,5 +1,4 @@
 import { Suite, Test } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import sourceMappedStackTrace from 'sourcemapped-stacktrace';
 
 // eslint-disable-next-line @typescript-eslint/no-empty-function
@@ -62,4 +61,4 @@ export const setStack = (error: Error, stack: string | undefined): void => {
   } catch (err) {
     // Do nothing
   }
-};
+};

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

@@ -1,5 +1,5 @@
 import { ErrorData, Global } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
+
 import { HarnessResponse } from '../core/ServerTypes';
 
 export interface TestErrorData {
@@ -11,7 +11,7 @@ export interface Callbacks {
   readonly loadHarness: () => Promise<HarnessResponse>
   readonly sendKeepAlive: (session: string) => Promise<void>;
   readonly sendInit: (session: string) => Promise<void>;
-  readonly sendTestStart: (session: string, totalTests: number, file: string, name: 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 sendDone: (session: string, error?: string) => Promise<void>;
 }
@@ -64,8 +64,9 @@ export const Callbacks = (): Callbacks => {
     });
   };
 
-  const sendTestStart = (session: string, totalTests: number, file: string, name: string): Promise<void> => {
+  const sendTestStart = (session: string, number: number, totalTests: number, file: string, name: string): Promise<void> => {
     return sendJson('/tests/start', {
+      number,
       totalTests,
       session,
       file,
@@ -104,4 +105,4 @@ export const Callbacks = (): Callbacks => {
     sendTestResult,
     sendDone
   };
-};
+};

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

@@ -1,5 +1,4 @@
 import { LoggedError, Reporter as ErrorReporter } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import { Callbacks } from './Callbacks';
 import { UrlParams } from '../core/UrlParams';
 import { formatElapsedTime, mapStackTrace, setStack } from '../core/Utils';
@@ -47,11 +46,15 @@ const mapError = (e: LoggedError) => mapStackTrace(e.stack).then((mappedStack) =
 
 export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi): Reporter => {
   const initial = new Date();
-  let currentCount = params.offset || 0;
+  const initialOffset = params.offset || 0;
+  let currentCount = initialOffset;
   let passCount = 0;
   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>[] = [];
+
   const summary = () => ({
     offset: Math.max(0, currentCount - 1),
     passed: passCount + (params.offset - params.failed - params.skipped),
@@ -65,6 +68,10 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
     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();
@@ -74,7 +81,17 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
         currentCount++;
 
         testUi.start(file, name);
-        return callbacks.sendTestStart(params.session, totalNumTests, 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;
+        }
+        // don't block, ever ever ever
+        return Promise.resolve();
       }
     };
 
@@ -92,7 +109,9 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
         const testTime = elapsed(starttime);
 
         testUi.pass(testTime, currentCount);
-        return callbacks.sendTestResult(params.session, file, name, true, testTime, null, null);
+
+        // don't block, ever ever ever
+        return Promise.resolve();
       }
     };
 
@@ -105,7 +124,14 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
         const testTime = elapsed(starttime);
 
         testUi.skip(testTime, currentCount);
-        return callbacks.sendTestResult(params.session, file, name, false, testTime, null, reason);
+
+        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();
       }
     };
 
@@ -125,7 +151,11 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
           };
 
           testUi.fail(err, testTime, currentCount);
-          return callbacks.sendTestResult(params.session, file, name, false, testTime, error, null);
+          // 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)
+          );
         });
       }
     };
@@ -146,12 +176,16 @@ export const Reporter = (params: UrlParams, callbacks: Callbacks, ui: ReporterUi
     };
 
     const textError = error !== undefined ? ErrorReporter.text(error) : undefined;
-    callbacks.sendDone(params.session, textError).then(setAsDone, setAsDone);
+
+    // make sure any in progress updates are sent before we clean up
+    Promise.all(reportsInFlight).then(() =>
+      callbacks.sendDone(params.session, textError).then(setAsDone, setAsDone)
+    );
   };
 
   return {
     summary,
     test,
     done
   };
-};
+};

modules/runner/src/main/ts/runner/Hooks.ts

@@ -1,5 +1,4 @@
 import { Hook, HookType, RunnableState, Suite, Test } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import * as Context from '../core/Context';
 import { SkipError } from '../errors/Errors';
 import { runWithErrorCatcher, runWithTimeout } from './Run';
@@ -75,4 +74,4 @@ export const runAfterEach = (currentTest: Test): Promise<void> => {
   } else {
     return Promise.resolve();
   }
-};
+};

modules/runner/src/main/ts/runner/Run.ts

@@ -1,5 +1,4 @@
 import { Context, ExecuteFn, Failure, Runnable, RunnableState, TestThrowable } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import { isInternalError, MultipleDone, SkipError } from '../errors/Errors';
 import { ErrorCatcher } from '../errors/ErrorCatcher';
 import { Timer } from './Timer';
@@ -128,4 +127,4 @@ export const runWithTimeout = (runnable: Runnable, context: Context, defaultTime
       }).then(resolveIfNotTimedOut, reject);
     });
   }
-};
+};

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

@@ -1,5 +1,4 @@
 import { Suite } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import { HarnessResponse } from '../core/ServerTypes';
 import { UrlParams } from '../core/UrlParams';
 import { noop } from '../core/Utils';
@@ -116,4 +115,4 @@ export const Runner = (rootSuite: Suite, params: UrlParams, callbacks: Callbacks
     init,
     run
   };
-};
+};

modules/runner/src/main/ts/runner/TestRun.ts

@@ -1,5 +1,4 @@
 import { Failure, LoggedError, RunnableState, Suite, Test } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import * as Context from '../core/Context';
 import { InternalError, isInternalError, SkipError } from '../errors/Errors';
 import { Reporter, TestReporter } from '../reporter/Reporter';
@@ -111,4 +110,4 @@ export const runSuite = (suite: Suite, state: RunState, actions: RunActions, rep
 
 export const runSuites = (suites: Suite[], state: RunState, actions: RunActions, reporter: RunReporter): Promise<void> => {
   return loop(suites, (suite) => runSuite(suite, state, actions, reporter));
-};
+};

modules/runner/src/main/ts/runner/Utils.ts

@@ -1,5 +1,4 @@
 import { Suite, Test } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 
 export const countTests = (suite: Suite): number =>
   suite.tests.length + suite.suites.reduce((acc, suite) => acc + countTests(suite), 0);
@@ -46,4 +45,4 @@ export const filterOnly = (suite: Suite): void => {
     suite.suites.forEach(filterOnly);
     suite.suites = onlySuites;
   }
-};
+};

modules/runner/src/test/ts/reporter/ReporterTest.ts

@@ -1,5 +1,4 @@
 import { Failure, LoggedError } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import { assert } from 'chai';
 import * as fc from 'fast-check';
 import { beforeEach, describe, it } from 'mocha';
@@ -10,6 +9,7 @@ import { noop } from '../TestUtils';
 
 interface StartTestData {
   readonly session: string;
+  readonly currentCount: number;
   readonly totalTests: number;
   readonly file: string;
   readonly name: string;
@@ -51,8 +51,8 @@ describe('Reporter.test', () => {
     loadHarness: () => Promise.resolve({ retries: 0, chunk: 100, stopOnFailure: true, mode: 'manual', timeout: 10000 }),
     sendKeepAlive: () => Promise.resolve(),
     sendInit: () => Promise.resolve(),
-    sendTestStart: (session, totalTests, file, name) => {
-      startTestData.push({ session, totalTests, file, name });
+    sendTestStart: (session, currentCount, totalTests, file, name) => {
+      startTestData.push({ session, currentCount, totalTests, file, name });
       return Promise.resolve();
     },
     sendTestResult: (session, file, name, passed, time, error, skipped) => {
@@ -82,15 +82,16 @@ describe('Reporter.test', () => {
       reset();
       const test = reporter.test(fileName + 'Test.ts', testName, testCount);
       return test.start().then(() => {
-        assert.equal(startTestData.length, 1);
+        assert.equal(startTestData.length, 1, 'Checking start test data length');
         assert.deepEqual(startTestData[0], {
+          currentCount: offset + 1,
           session: sessionId,
           totalTests: testCount,
           file: fileName + 'Test.ts',
           name: testName
-        });
+        }, 'Checking start test data contents');
 
-        assert.equal(endTestData.length, 0);
+        assert.equal(endTestData.length, 0, 'Checking end test data length');
         assert.deepEqual(reporter.summary(), {
           offset,
           passed: offset,
@@ -103,35 +104,6 @@ describe('Reporter.test', () => {
     }));
   });
 
-  it('should report the session id, file, name, passed state and time on a test success', () => {
-    return fc.assert(fc.asyncProperty(fc.hexaString(), fc.asciiString(), fc.integer(offset), (fileName, testName, testCount) => {
-      reset();
-      const test = reporter.test(fileName + 'Test.ts', testName, testCount);
-      return test.start()
-        .then(test.pass)
-        .then(() => {
-          assert.equal(endTestData.length, 1);
-          const data = endTestData[0];
-          assert.equal(data.session, sessionId);
-          assert.equal(data.file, fileName + 'Test.ts');
-          assert.equal(data.name, testName);
-          assert.isTrue(data.passed);
-          assert.isNull(data.skipped);
-          assert.isNull(data.error);
-          assert.isString(data.time);
-
-          assert.deepEqual(reporter.summary(), {
-            offset,
-            passed: offset + 1,
-            failed: 0,
-            skipped: 0
-          }, 'Summary has one passed test');
-
-          assert.isFalse(doneCalled);
-        });
-    }));
-  });
-
   it('should report the session id, file, name, passed state and time on a skipped test', () => {
     return fc.assert(fc.asyncProperty(fc.hexaString(), fc.asciiString(), fc.asciiString(), fc.integer(offset), (fileName, testName, skippedMessage, testCount) => {
       reset();
@@ -192,15 +164,19 @@ describe('Reporter.test', () => {
     }));
   });
 
-  it('should report done', () => {
+  it('should report done', async () => {
     reporter.done();
+    // let the async callbacks run
+    await Promise.resolve();
     assert.isTrue(doneCalled);
     assert.isUndefined(doneError);
   });
 
-  it('should report done with an error', () => {
+  it('should report done with an error', async () => {
     reporter.done(Failure.prepFailure('Unexpected error occurred'));
+    // let the async callbacks run
+    await Promise.resolve();
     assert.isTrue(doneCalled);
     assert.include(doneError, 'Unexpected error occurred');
   });
-});
+});

modules/runner/src/test/ts/runner/RunnerTestUtils.ts

@@ -1,5 +1,4 @@
 import { Hook, HookType, LoggedError, Suite } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import { createHook } from '../../../main/ts/core/Hook';
 import { Reporter, TestReporter } from '../../../main/ts/reporter/Reporter';
 import { RunState } from '../../../main/ts/runner/TestRun';
@@ -63,4 +62,4 @@ export const createRunState = (offset: number, chunk: number, count = 0): RunSta
   chunk,
   timeout: TEST_TIMEOUT,
   testCount: count
-});
+});

modules/runner/src/test/ts/runner/TestRunTest.ts

@@ -1,5 +1,4 @@
 import { Context, HookType, RunnableState, Suite, Test } from '@ephox/bedrock-common';
-import Promise from '@ephox/wrap-promise-polyfill';
 import { assert } from 'chai';
 import * as fc from 'fast-check';
 import { beforeEach, describe, it } from 'mocha';
@@ -355,4 +354,4 @@ describe('TestRun.runSuite', () => {
       assert.deepEqual(hooks, [ HookType.Before, HookType.BeforeEach, HookType.AfterEach, HookType.BeforeEach, HookType.AfterEach, HookType.After ]);
     });
   });
-});
+});