Merge pull request #1800 from ably/ECO-4834-do-something-with-private…

…-api-data

Gather data about private API usage in test suite

.github/workflows/test-browser.yml

@@ -30,6 +30,15 @@ jobs:
       - env:
           PLAYWRIGHT_BROWSER: ${{ matrix.browser }}
         run: npm run test:playwright
+      - name: Generate private API usage reports
+        run: npm run process-private-api-data private-api-usage/*.json
+      - name: Save private API usage data
+        uses: actions/upload-artifact@v4
+        with:
+          name: private-api-usage-${{ matrix.browser }}
+          path: |
+            private-api-usage
+            private-api-usage-reports
       - name: Upload test results
         if: always()
         uses: ably/test-observability-action@v1

.github/workflows/test-node.yml

@@ -28,6 +28,15 @@ jobs:
       - run: npm run test:node
         env:
           CI: true
+      - name: Generate private API usage reports
+        run: npm run process-private-api-data private-api-usage/*.json
+      - name: Save private API usage data
+        uses: actions/upload-artifact@v4
+        with:
+          name: private-api-usage-${{ matrix.node-version }}
+          path: |
+            private-api-usage
+            private-api-usage-reports
       - name: Upload test results
         if: always()
         uses: ably/test-observability-action@v1

.gitignore

@@ -8,4 +8,6 @@ build/
 react/
 typedoc/generated/
 junit/
+private-api-usage/
+private-api-usage-reports/
 test/support/mocha_junit_reporter/build/

CONTRIBUTING.md

@@ -158,6 +158,30 @@ When using the test webserver `npm run test:webserver` the following test variab
 - `tls` - true or false to enable/disable use of TLS respectively
 - `log_level` - Log level for the client libraries, defaults to 2, 4 is `MICRO`
 
+### Adding private API annotations to tests
+
+We have an ongoing project which aims to reuse the ably-js test suite as a unified test suite for all of our client libraries. To enable this work, we want to be able to monitor the test suite’s usage of APIs that are private to ably-js.
+
+When you use a private API in a test, record its usage using the `recordPrivateApi` method on the test’s helper object. For example:
+
+```javascript
+/* Sabotage the reattach attempt, then simulate a server-sent detach */
+helper.recordPrivateApi('replace.channel.sendMessage');
+channel.sendMessage = function () {};
+```
+
+The current list of private API usage identifiers can be found in [`test/common/modules/private_api_recorder.js`](test/common/modules/private_api_recorder.js); add new ones there as necessary.
+
+The following test files do not utilise private API annotations, and you don’t need to add them:
+
+- [`test/realtime/transports.test.js`](test/realtime/transports.test.js)
+- [`test/browser/simple.test.js`](test/browser/simple.test.js)
+- [`test/browser/http.test.js`](test/browser/http.test.js)
+- [`test/browser/connection.test.js`](test/browser/connection.test.js)
+- [`test/browser/modular.test.js`](test/browser/modular.test.js)
+- [`test/browser/push.test.js`](test/browser/push.test.js)
+- [`test/rest/bufferutils.test.js`](test/rest/bufferutils.test.js)
+
 ## React hooks
 
 The react sample application is configured to execute using Vite - which will load a sample web app that acts as a simple test harness for the hooks.

package.json

@@ -79,6 +79,7 @@
     "chai": "^4.2.0",
     "cli-table": "^0.3.11",
     "cors": "^2.8.5",
+    "csv": "^6.3.9",
     "dox": "^1.0.0",
     "esbuild": "^0.18.10",
     "esbuild-plugin-umd-wrapper": "ably-forks/esbuild-plugin-umd-wrapper#1.0.7-optional-amd-named-module",
@@ -155,11 +156,12 @@
     "lint": "eslint .",
     "lint:fix": "eslint --fix .",
     "prepare": "npm run build",
-    "format": "prettier --write --ignore-path .gitignore --ignore-path .prettierignore src test ably.d.ts modular.d.ts webpack.config.js Gruntfile.js scripts/*.[jt]s docs/**/*.md grunt",
-    "format:check": "prettier --check --ignore-path .gitignore --ignore-path .prettierignore src test ably.d.ts modular.d.ts webpack.config.js Gruntfile.js scripts/*.[jt]s docs/**/*.md grunt",
+    "format": "prettier --write --ignore-path .gitignore --ignore-path .prettierignore src test ably.d.ts modular.d.ts webpack.config.js Gruntfile.js scripts/**/*.[jt]s docs/**/*.md grunt",
+    "format:check": "prettier --check --ignore-path .gitignore --ignore-path .prettierignore src test ably.d.ts modular.d.ts webpack.config.js Gruntfile.js scripts/**/*.[jt]s docs/**/*.md grunt",
     "sourcemap": "source-map-explorer build/ably.min.js",
     "modulereport": "tsc --noEmit --esModuleInterop scripts/moduleReport.ts && esr scripts/moduleReport.ts",
     "speccoveragereport": "tsc --noEmit --esModuleInterop --target ES2017 --moduleResolution node scripts/specCoverageReport.ts && esr scripts/specCoverageReport.ts",
+    "process-private-api-data": "tsc --noEmit --esModuleInterop --strictNullChecks scripts/processPrivateApiData/run.ts && esr scripts/processPrivateApiData/run.ts",
     "docs": "typedoc"
   }
 }

scripts/processPrivateApiData/dto.ts

@@ -0,0 +1,53 @@
+export type TestPrivateApiContextDto = {
+  type: 'test';
+  title: string;
+  /**
+   * null means that either the test isn’t parameterised or that this usage is unique to the specific parameter
+   */
+  parameterisedTestTitle: string | null;
+  helperStack: string[];
+  file: string;
+  suite: string[];
+};
+
+export type HookPrivateApiContextDto = {
+  type: 'hook';
+  title: string;
+  helperStack: string[];
+  file: string;
+  suite: string[];
+};
+
+export type RootHookPrivateApiContextDto = {
+  type: 'hook';
+  title: string;
+  helperStack: string[];
+  file: null;
+  suite: null;
+};
+
+export type TestDefinitionPrivateApiContextDto = {
+  type: 'definition';
+  label: string;
+  helperStack: string[];
+  file: string;
+  suite: string[];
+};
+
+export type PrivateApiContextDto =
+  | TestPrivateApiContextDto
+  | HookPrivateApiContextDto
+  | RootHookPrivateApiContextDto
+  | TestDefinitionPrivateApiContextDto;
+
+export type PrivateApiUsageDto = {
+  context: PrivateApiContextDto;
+  privateAPIIdentifier: string;
+};
+
+export type TestStartRecord = {
+  context: TestPrivateApiContextDto;
+  privateAPIIdentifier: null;
+};
+
+export type Record = PrivateApiUsageDto | TestStartRecord;

scripts/processPrivateApiData/exclusions.ts

@@ -0,0 +1,23 @@
+import { PrivateApiUsageDto } from './dto';
+
+type ExclusionRule = {
+  privateAPIIdentifier: string;
+  // i.e. only ignore when called from within this helper
+  helper?: string;
+};
+
+/**
+ * This exclusions mechanism is not currently being used on `main`, but I will use it on a separate unified test suite branch in order to exclude some private API usage that can currently be disregarded in the context of the unified test suite.
+ */
+export function applyingExclusions(usageDtos: PrivateApiUsageDto[]) {
+  const exclusionRules: ExclusionRule[] = [];
+
+  return usageDtos.filter(
+    (usageDto) =>
+      !exclusionRules.some(
+        (exclusionRule) =>
+          exclusionRule.privateAPIIdentifier === usageDto.privateAPIIdentifier &&
+          (!('helper' in exclusionRule) || usageDto.context.helperStack.includes(exclusionRule.helper!)),
+      ),
+  );
+}

scripts/processPrivateApiData/grouping.ts

@@ -0,0 +1,76 @@
+import { PrivateApiUsageDto } from './dto';
+
+export type Group<Key, Value> = {
+  key: Key;
+  values: Value[];
+};
+
+export function grouped<Key, Value>(
+  values: Value[],
+  keyForValue: (value: Value) => Key,
+  areKeysEqual: (key1: Key, key2: Key) => boolean,
+) {
+  const result: Group<Key, Value>[] = [];
+
+  for (const value of values) {
+    const key = keyForValue(value);
+
+    let existingGroup = result.find((group) => areKeysEqual(group.key, key));
+
+    if (existingGroup === undefined) {
+      existingGroup = { key, values: [] };
+      result.push(existingGroup);
+    }
+
+    existingGroup.values.push(value);
+  }
+
+  return result;
+}
+
+/**
+ * Makes sure that each private API is only listed once in a given context.
+ */
+function dedupeUsages<Key>(contextGroups: Group<Key, PrivateApiUsageDto>[]) {
+  for (const contextGroup of contextGroups) {
+    const newUsages: typeof contextGroup.values = [];
+
+    for (const usage of contextGroup.values) {
+      const existing = newUsages.find((otherUsage) => otherUsage.privateAPIIdentifier === usage.privateAPIIdentifier);
+      if (existing === undefined) {
+        newUsages.push(usage);
+      }
+    }
+
+    contextGroup.values = newUsages;
+  }
+}
+
+export function groupedAndDeduped<Key>(
+  usages: PrivateApiUsageDto[],
+  keyForUsage: (usage: PrivateApiUsageDto) => Key,
+  areKeysEqual: (key1: Key, key2: Key) => boolean,
+) {
+  const result = grouped(usages, keyForUsage, areKeysEqual);
+  dedupeUsages(result);
+  return result;
+}
+
+/**
+ * Return value is sorted in decreasing order of usage of a given private API identifer
+ */
+export function groupedAndSortedByPrivateAPIIdentifier<Key>(
+  groupedByKey: Group<Key, PrivateApiUsageDto>[],
+): Group<string, Key>[] {
+  const flattened = groupedByKey.flatMap((group) => group.values.map((value) => ({ key: group.key, value })));
+
+  const groupedByPrivateAPIIdentifier = grouped(
+    flattened,
+    (value) => value.value.privateAPIIdentifier,
+    (id1, id2) => id1 === id2,
+  ).map((group) => ({ key: group.key, values: group.values.map((value) => value.key) }));
+
+  groupedByPrivateAPIIdentifier.sort((group1, group2) => group2.values.length - group1.values.length);
+
+  return groupedByPrivateAPIIdentifier;
+}

scripts/processPrivateApiData/load.ts

@@ -0,0 +1,16 @@
+import { readFileSync } from 'fs';
+import { applyingExclusions } from './exclusions';
+import { splittingRecords, stripFilePrefix } from './utils';
+import { Record } from './dto';
+
+export function load(jsonFilePath: string) {
+  let records = JSON.parse(readFileSync(jsonFilePath).toString('utf-8')) as Record[];
+
+  stripFilePrefix(records);
+
+  let { usageDtos, testStartRecords } = splittingRecords(records);
+
+  usageDtos = applyingExclusions(usageDtos);
+
+  return { usageDtos, testStartRecords };
+}