Skip to content

Commit

Permalink
Record usages of private APIs in the test suite
Browse files Browse the repository at this point in the history
This adds annotations to each place in the test suite where a private
API is called. When the tests are run, these annotations are used to
emit a JSON file containing private API usage information. (In ECO-4834,
we’ll process this JSON file to generate some useful reports.) This JSON
file also contains a list of all of the tests which get run, which will
be used as part of the aforementioned reporting to allow us to generate
a report of the tests that do not use any private APIs.

The motivation for this gathering this data is that we intend to try and
reuse the ably-js test suite as a unified test suite for all of our
client libraries. In order to do this, we’ll need to address the usage
of ably-js private APIs in the test suite, and to do that we need to
first gather data about the current usage.

I originally intended to put the changes into the current commit into a
separate unified test suite branch, instead of merging them into `main`.
But I don’t really want to maintain a long-running feature branch, and
also I want to make sure that we add private API annotations to any new
tests that we add to ably-js, hence I decided to put this commit into
`main`. But maybe, once we’re done with the unified test suite, we’ll
decide to remove these annotations.

Given the context in which we’re gathering this data, in the interests
of saving time I skipped adding some annotations to some files that I’m
pretty sure will not form part of the unified test suite:

- test/realtime/transports.test.js
- test/browser/simple.test.js
- test/browser/http.test.js
- test/browser/connection.test.js
- test/browser/modular.test.js
- test/browser/push.test.js
- test/rest/bufferutils.test.js

If we decide at some point that we’d like to for whatever reason gather
data on these tests, we can do that later.

(Note I haven't been very consistent with how many times per test I
record the usage of a given private API; it’s not a stat I’m
particularly interested in here. But for the most part I’m doing it at
the point of each invocation of the private API.)

Resolves ECO-4821.
  • Loading branch information
lawrence-forooghian committed Jul 24, 2024
1 parent 0bf4291 commit f53e514
Show file tree
Hide file tree
Showing 41 changed files with 995 additions and 85 deletions.
5 changes: 5 additions & 0 deletions .github/workflows/test-browser.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ jobs:
- env:
PLAYWRIGHT_BROWSER: ${{ matrix.browser }}
run: npm run test:playwright
- name: Save private API usage data
uses: actions/upload-artifact@v4
with:
name: private-api-usage-${{ matrix.browser }}
path: private-api-usage
- name: Upload test results
if: always()
uses: ably/test-observability-action@v1
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/test-node.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ jobs:
- run: npm run test:node
env:
CI: true
- name: Save private API usage data
uses: actions/upload-artifact@v4
with:
name: private-api-usage-${{ matrix.node-version }}
path: private-api-usage
- name: Upload test results
if: always()
uses: ably/test-observability-action@v1
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ build/
react/
typedoc/generated/
junit/
private-api-usage/
test/support/mocha_junit_reporter/build/
24 changes: 24 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions test/common/globals/named_dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,9 @@ define(function () {
async: { browser: 'node_modules/async/lib/async' },
chai: { browser: 'node_modules/chai/chai', node: 'node_modules/chai/chai' },
ulid: { browser: 'node_modules/ulid/dist/index.umd', node: 'node_modules/ulid/dist/index.umd' },
private_api_recorder: {
browser: 'test/common/modules/private_api_recorder',
node: 'test/common/modules/private_api_recorder',
},
});
});
15 changes: 10 additions & 5 deletions test/common/modules/client_module.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@
define(['ably', 'globals', 'test/common/modules/testapp_module'], function (Ably, ablyGlobals, testAppHelper) {
var utils = Ably.Realtime.Utils;

function ablyClientOptions(options) {
function ablyClientOptions(helper, options) {
helper = helper.addingHelperFunction('ablyClientOptions');
helper.recordPrivateApi('call.Utils.copy');
var clientOptions = utils.copy(ablyGlobals);
helper.recordPrivateApi('call.Utils.mixin');
utils.mixin(clientOptions, options);
var authMethods = ['authUrl', 'authCallback', 'token', 'tokenDetails', 'key'];

Expand All @@ -22,12 +25,14 @@ define(['ably', 'globals', 'test/common/modules/testapp_module'], function (Ably
return clientOptions;
}

function ablyRest(options) {
return new Ably.Rest(ablyClientOptions(options));
function ablyRest(helper, options) {
helper = helper.addingHelperFunction('ablyRest');
return new Ably.Rest(ablyClientOptions(helper, options));
}

function ablyRealtime(options) {
return new Ably.Realtime(ablyClientOptions(options));
function ablyRealtime(helper, options) {
helper = helper.addingHelperFunction('ablyRealtime');
return new Ably.Realtime(ablyClientOptions(helper, options));
}

return (module.exports = {
Expand Down
196 changes: 196 additions & 0 deletions test/common/modules/private_api_recorder.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
'use strict';

define(['test/support/output_directory_paths'], function (outputDirectoryPaths) {
const privateAPIIdentifiers = [
'call.BufferUtils.areBuffersEqual',
'call.BufferUtils.base64Decode',
'call.BufferUtils.base64Encode',
'call.BufferUtils.hexEncode',
'call.BufferUtils.isBuffer',
'call.BufferUtils.toArrayBuffer',
'call.BufferUtils.utf8Encode',
'call.ConnectionManager.supportedTransports',
'call.Defaults.getHost',
'call.Defaults.getHost',
'call.Defaults.getHosts',
'call.Defaults.getPort',
'call.Defaults.normaliseOptions',
'call.EventEmitter.emit',
'call.Message.decode',
'call.Message.encode',
'call.Platform.Config.push.storage.clear',
'call.Platform.nextTick',
'call.PresenceMessage.fromValues',
'call.ProtocolMessage.setFlag',
'call.Utils.copy',
'call.Utils.getRetryTime',
'call.Utils.inspectError',
'call.Utils.keysArray',
'call.Utils.mixin',
'call.Utils.toQueryString',
'call.auth.getAuthHeaders',
'call.channel.checkPendingState',
'call.channel.processMessage',
'call.channel.requestState',
'call.channel.sendPresence',
'call.channel.sync',
'call.connectionManager.activeProtocol.getTransport',
'call.connectionManager.disconnectAllTransports',
'call.connectionManager.notifyState',
'call.connectionManager.onChannelMessage',
'call.connectionManager.requestState',
'call.connectionManager.send',
'call.filteredSubscriptions.has',
'call.http._getHosts',
'call.http.checkConnectivity',
'call.http.doUri',
'call.msgpack.decode',
'call.msgpack.encode',
'call.presence._myMembers.put',
'call.presence.waitSync',
'call.protocolMessageFromDeserialized',
'call.realtime.baseUri',
'call.rest.baseUri',
'call.rest.http.do',
'call.restChannel._publish',
'call.transport.onProtocolMessage',
'call.transport.send',
'delete.auth.authOptions.requestHeaders',
'deserialize.recoveryKey',
'listen.channel._allChannelChanges.attached',
'listen.channel._allChannelChanges.update',
'listen.connectionManager.connectiondetails',
'listen.connectionManager.transport.active',
'listen.connectionManager.transport.pending',
'listen.transport.disposed',
'new.Crypto.CipherParams', // This is in a bit of a grey area re whether it’s private API. The CipherParams class is indeed part of the public API in the spec, but it’s not clear whether you’re meant to construct one directly, and furthermore the spec doesn’t describe it as being exposed as a property on Crypto.
'pass.clientOption.connectivityCheckUrl', // actually ably-js public API (i.e. it’s in the TypeScript typings) but no other SDK has it and it doesn’t enable ably-js-specific functionality
'pass.clientOption.disableConnectivityCheck', // actually ably-js public API (i.e. it’s in the TypeScript typings) but no other SDK has it and it doesn’t enable ably-js-specific functionality
'pass.clientOption.pushRecipientChannel',
'pass.clientOption.webSocketConnectTimeout',
'read.Defaults.protocolVersion',
'read.Defaults.version',
'read.EventEmitter.events',
'read.Platform.Config.push',
'read.Realtime._transports',
'read.auth.authOptions.authUrl',
'read.auth.key',
'read.auth.method',
'read.auth.tokenParams.version',
'read.channel.channelOptions',
'read.channel.channelOptions.cipher',
'read.connectionManager.activeProtocol',
'read.connectionManager.activeProtocol.transport',
'read.connectionManager.baseTransport',
'read.connectionManager.connectionId',
'read.connectionManager.connectionId',
'read.connectionManager.connectionStateTtl',
'read.connectionManager.httpHosts',
'read.connectionManager.msgSerial',
'read.connectionManager.options',
'read.connectionManager.options.timeouts.httpMaxRetryDuration',
'read.connectionManager.options.timeouts.httpRequestTimeout',
'read.connectionManager.pendingTransport',
'read.connectionManager.queuedMessages.messages',
'read.connectionManager.states.disconnected.retryDelay',
'read.connectionManager.states.suspended.retryDelay',
'read.connectionManager.webSocketTransportAvailable',
'read.realtime.options',
'read.realtime.options.key',
'read.realtime.options.maxMessageSize',
'read.realtime.options.token',
'read.rest._currentFallback',
'read.rest._currentFallback.host',
'read.rest._currentFallback.validUntil',
'read.rest.options.key',
'read.rest.options.realtimeHost',
'read.rest.options.token',
'read.rest.serverTimeOffset',
'read.transport.params.mode',
'read.transport.recvRequest.recvUri',
'read.transport.uri',
'replace.channel.attachImpl',
'replace.channel.processMessage',
'replace.channel.sendMessage',
'replace.channel.sendPresence',
'replace.connectionManager.onChannelMessage',
'replace.connectionManager.send',
'replace.connectionManager.tryATransport',
'replace.http.doUri',
'replace.rest.http.do',
'replace.rest.time',
'replace.restChannel._publish',
'replace.transport.onProtocolMessage',
'replace.transport.send',
'serialize.recoveryKey',
'write.Defaults.ENVIRONMENT',
'write.Platform.Config.push', // This implies using a mock implementation of the internal IPlatformPushConfig interface. Our mock (in push_channel_transport.js) then interacts with internal objects and private APIs of public objects to implement this interface; I haven’t added annotations for that private API usage, since there wasn’t an easy way to pass test context information into the mock. I think that for now we can just say that if we wanted to get rid of this private API usage, then we’d need to remove this mock entirely.
'write.auth.authOptions.requestHeaders',
'write.auth.key',
'write.auth.tokenDetails.token',
'write.channel._lastPayload',
'write.channel.state',
'write.connectionManager.connectionDetails.maxMessageSize',
'write.connectionManager.connectionId',
'write.connectionManager.connectionKey',
'write.connectionManager.lastActivity',
'write.connectionManager.msgSerial',
'write.realtime.options.timeouts.realtimeRequestTimeout',
'write.rest._currentFallback.validUntil',
];

class PrivateApiRecorder {
privateAPIUsages = [];

/**
* Creates a recording context for the current Mocha test case.
*
* @param context A description of the context for which the calls will be recorded.
*/
createContext(context) {
if (!context) {
throw new Error('No description passed to createContext');
}

const loggingDescription = JSON.stringify(context);

return {
record: (privateAPIIdentifier) => {
if (privateAPIIdentifiers.indexOf(privateAPIIdentifier) == -1) {
throw new Error(`(${loggingDescription}) Recorded unknown private API: ${privateAPIIdentifier}`);
}
this.privateAPIUsages.push({ context, privateAPIIdentifier });
},
recordTestStart: () => {
this.privateAPIUsages.push({ context, privateAPIIdentifier: null });
},
};
}

dump() {
if (isBrowser) {
window.dispatchEvent(new CustomEvent('privateApiUsageData', { detail: this.privateAPIUsages }));
} else {
try {
const fs = require('fs');
const path = require('path');

if (!fs.existsSync(outputDirectoryPaths.privateApiUsage)) {
fs.mkdirSync(outputDirectoryPaths.privateApiUsage);
}
const filename = `node-${process.version.split('.')[0]}.json`;
fs.writeFileSync(
path.join(outputDirectoryPaths.privateApiUsage, filename),
JSON.stringify(this.privateAPIUsages),
{ encoding: 'utf-8' },
);
} catch (err) {
console.log('Failed to write private API usage data: ', err);
throw err;
}
}
}
}

return (module.exports = new PrivateApiRecorder());
});
Loading

0 comments on commit f53e514

Please sign in to comment.