Skip to content

Commit f53e514

Browse files
lawrence-forooghianlawrence-forooghian
committed
Record usages of private APIs in the test suite
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.
1 parent 0bf4291 commit f53e514

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+995
-85
lines changed

‎.github/workflows/test-browser.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ jobs:
3030
- env:
3131
PLAYWRIGHT_BROWSER: ${{ matrix.browser }}
3232
run: npm run test:playwright
33+
- name: Save private API usage data
34+
uses: actions/upload-artifact@v4
35+
with:
36+
name: private-api-usage-${{ matrix.browser }}
37+
path: private-api-usage
3338
- name: Upload test results
3439
if: always()
3540
uses: ably/test-observability-action@v1

‎.github/workflows/test-node.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ jobs:
2828
- run: npm run test:node
2929
env:
3030
CI: true
31+
- name: Save private API usage data
32+
uses: actions/upload-artifact@v4
33+
with:
34+
name: private-api-usage-${{ matrix.node-version }}
35+
path: private-api-usage
3136
- name: Upload test results
3237
if: always()
3338
uses: ably/test-observability-action@v1

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,4 +8,5 @@ build/
88
react/
99
typedoc/generated/
1010
junit/
11+
private-api-usage/
1112
test/support/mocha_junit_reporter/build/

‎CONTRIBUTING.md‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,30 @@ When using the test webserver `npm run test:webserver` the following test variab
158158
- `tls` - true or false to enable/disable use of TLS respectively
159159
- `log_level` - Log level for the client libraries, defaults to 2, 4 is `MICRO`
160160

161+
### Adding private API annotations to tests
162+
163+
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.
164+
165+
When you use a private API in a test, record its usage using the `recordPrivateApi` method on the test’s helper object. For example:
166+
167+
```javascript
168+
/* Sabotage the reattach attempt, then simulate a server-sent detach */
169+
helper.recordPrivateApi('replace.channel.sendMessage');
170+
channel.sendMessage = function () {};
171+
```
172+
173+
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.
174+
175+
The following test files do not utilise private API annotations, and you don’t need to add them:
176+
177+
- [`test/realtime/transports.test.js`](test/realtime/transports.test.js)
178+
- [`test/browser/simple.test.js`](test/browser/simple.test.js)
179+
- [`test/browser/http.test.js`](test/browser/http.test.js)
180+
- [`test/browser/connection.test.js`](test/browser/connection.test.js)
181+
- [`test/browser/modular.test.js`](test/browser/modular.test.js)
182+
- [`test/browser/push.test.js`](test/browser/push.test.js)
183+
- [`test/rest/bufferutils.test.js`](test/rest/bufferutils.test.js)
184+
161185
## React hooks
162186

163187
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.

‎test/common/globals/named_dependencies.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,9 @@ define(function () {
1818
async: { browser: 'node_modules/async/lib/async' },
1919
chai: { browser: 'node_modules/chai/chai', node: 'node_modules/chai/chai' },
2020
ulid: { browser: 'node_modules/ulid/dist/index.umd', node: 'node_modules/ulid/dist/index.umd' },
21+
private_api_recorder: {
22+
browser: 'test/common/modules/private_api_recorder',
23+
node: 'test/common/modules/private_api_recorder',
24+
},
2125
});
2226
});

‎test/common/modules/client_module.js‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,11 @@
55
define(['ably', 'globals', 'test/common/modules/testapp_module'], function (Ably, ablyGlobals, testAppHelper) {
66
var utils = Ably.Realtime.Utils;
77

8-
function ablyClientOptions(options) {
8+
function ablyClientOptions(helper, options) {
9+
helper = helper.addingHelperFunction('ablyClientOptions');
10+
helper.recordPrivateApi('call.Utils.copy');
911
var clientOptions = utils.copy(ablyGlobals);
12+
helper.recordPrivateApi('call.Utils.mixin');
1013
utils.mixin(clientOptions, options);
1114
var authMethods = ['authUrl', 'authCallback', 'token', 'tokenDetails', 'key'];
1215

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

25-
function ablyRest(options) {
26-
return new Ably.Rest(ablyClientOptions(options));
28+
function ablyRest(helper, options) {
29+
helper = helper.addingHelperFunction('ablyRest');
30+
return new Ably.Rest(ablyClientOptions(helper, options));
2731
}
2832

29-
function ablyRealtime(options) {
30-
return new Ably.Realtime(ablyClientOptions(options));
33+
function ablyRealtime(helper, options) {
34+
helper = helper.addingHelperFunction('ablyRealtime');
35+
return new Ably.Realtime(ablyClientOptions(helper, options));
3136
}
3237

3338
return (module.exports = {

‎test/common/modules/private_api_recorder.js‎

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
'use strict';
2+
3+
define(['test/support/output_directory_paths'], function (outputDirectoryPaths) {
4+
const privateAPIIdentifiers = [
5+
'call.BufferUtils.areBuffersEqual',
6+
'call.BufferUtils.base64Decode',
7+
'call.BufferUtils.base64Encode',
8+
'call.BufferUtils.hexEncode',
9+
'call.BufferUtils.isBuffer',
10+
'call.BufferUtils.toArrayBuffer',
11+
'call.BufferUtils.utf8Encode',
12+
'call.ConnectionManager.supportedTransports',
13+
'call.Defaults.getHost',
14+
'call.Defaults.getHost',
15+
'call.Defaults.getHosts',
16+
'call.Defaults.getPort',
17+
'call.Defaults.normaliseOptions',
18+
'call.EventEmitter.emit',
19+
'call.Message.decode',
20+
'call.Message.encode',
21+
'call.Platform.Config.push.storage.clear',
22+
'call.Platform.nextTick',
23+
'call.PresenceMessage.fromValues',
24+
'call.ProtocolMessage.setFlag',
25+
'call.Utils.copy',
26+
'call.Utils.getRetryTime',
27+
'call.Utils.inspectError',
28+
'call.Utils.keysArray',
29+
'call.Utils.mixin',
30+
'call.Utils.toQueryString',
31+
'call.auth.getAuthHeaders',
32+
'call.channel.checkPendingState',
33+
'call.channel.processMessage',
34+
'call.channel.requestState',
35+
'call.channel.sendPresence',
36+
'call.channel.sync',
37+
'call.connectionManager.activeProtocol.getTransport',
38+
'call.connectionManager.disconnectAllTransports',
39+
'call.connectionManager.notifyState',
40+
'call.connectionManager.onChannelMessage',
41+
'call.connectionManager.requestState',
42+
'call.connectionManager.send',
43+
'call.filteredSubscriptions.has',
44+
'call.http._getHosts',
45+
'call.http.checkConnectivity',
46+
'call.http.doUri',
47+
'call.msgpack.decode',
48+
'call.msgpack.encode',
49+
'call.presence._myMembers.put',
50+
'call.presence.waitSync',
51+
'call.protocolMessageFromDeserialized',
52+
'call.realtime.baseUri',
53+
'call.rest.baseUri',
54+
'call.rest.http.do',
55+
'call.restChannel._publish',
56+
'call.transport.onProtocolMessage',
57+
'call.transport.send',
58+
'delete.auth.authOptions.requestHeaders',
59+
'deserialize.recoveryKey',
60+
'listen.channel._allChannelChanges.attached',
61+
'listen.channel._allChannelChanges.update',
62+
'listen.connectionManager.connectiondetails',
63+
'listen.connectionManager.transport.active',
64+
'listen.connectionManager.transport.pending',
65+
'listen.transport.disposed',
66+
'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.
67+
'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
68+
'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
69+
'pass.clientOption.pushRecipientChannel',
70+
'pass.clientOption.webSocketConnectTimeout',
71+
'read.Defaults.protocolVersion',
72+
'read.Defaults.version',
73+
'read.EventEmitter.events',
74+
'read.Platform.Config.push',
75+
'read.Realtime._transports',
76+
'read.auth.authOptions.authUrl',
77+
'read.auth.key',
78+
'read.auth.method',
79+
'read.auth.tokenParams.version',
80+
'read.channel.channelOptions',
81+
'read.channel.channelOptions.cipher',
82+
'read.connectionManager.activeProtocol',
83+
'read.connectionManager.activeProtocol.transport',
84+
'read.connectionManager.baseTransport',
85+
'read.connectionManager.connectionId',
86+
'read.connectionManager.connectionId',
87+
'read.connectionManager.connectionStateTtl',
88+
'read.connectionManager.httpHosts',
89+
'read.connectionManager.msgSerial',
90+
'read.connectionManager.options',
91+
'read.connectionManager.options.timeouts.httpMaxRetryDuration',
92+
'read.connectionManager.options.timeouts.httpRequestTimeout',
93+
'read.connectionManager.pendingTransport',
94+
'read.connectionManager.queuedMessages.messages',
95+
'read.connectionManager.states.disconnected.retryDelay',
96+
'read.connectionManager.states.suspended.retryDelay',
97+
'read.connectionManager.webSocketTransportAvailable',
98+
'read.realtime.options',
99+
'read.realtime.options.key',
100+
'read.realtime.options.maxMessageSize',
101+
'read.realtime.options.token',
102+
'read.rest._currentFallback',
103+
'read.rest._currentFallback.host',
104+
'read.rest._currentFallback.validUntil',
105+
'read.rest.options.key',
106+
'read.rest.options.realtimeHost',
107+
'read.rest.options.token',
108+
'read.rest.serverTimeOffset',
109+
'read.transport.params.mode',
110+
'read.transport.recvRequest.recvUri',
111+
'read.transport.uri',
112+
'replace.channel.attachImpl',
113+
'replace.channel.processMessage',
114+
'replace.channel.sendMessage',
115+
'replace.channel.sendPresence',
116+
'replace.connectionManager.onChannelMessage',
117+
'replace.connectionManager.send',
118+
'replace.connectionManager.tryATransport',
119+
'replace.http.doUri',
120+
'replace.rest.http.do',
121+
'replace.rest.time',
122+
'replace.restChannel._publish',
123+
'replace.transport.onProtocolMessage',
124+
'replace.transport.send',
125+
'serialize.recoveryKey',
126+
'write.Defaults.ENVIRONMENT',
127+
'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.
128+
'write.auth.authOptions.requestHeaders',
129+
'write.auth.key',
130+
'write.auth.tokenDetails.token',
131+
'write.channel._lastPayload',
132+
'write.channel.state',
133+
'write.connectionManager.connectionDetails.maxMessageSize',
134+
'write.connectionManager.connectionId',
135+
'write.connectionManager.connectionKey',
136+
'write.connectionManager.lastActivity',
137+
'write.connectionManager.msgSerial',
138+
'write.realtime.options.timeouts.realtimeRequestTimeout',
139+
'write.rest._currentFallback.validUntil',
140+
];
141+
142+
class PrivateApiRecorder {
143+
privateAPIUsages = [];
144+
145+
/**
146+
* Creates a recording context for the current Mocha test case.
147+
*
148+
* @param context A description of the context for which the calls will be recorded.
149+
*/
150+
createContext(context) {
151+
if (!context) {
152+
throw new Error('No description passed to createContext');
153+
}
154+
155+
const loggingDescription = JSON.stringify(context);
156+
157+
return {
158+
record: (privateAPIIdentifier) => {
159+
if (privateAPIIdentifiers.indexOf(privateAPIIdentifier) == -1) {
160+
throw new Error(`(${loggingDescription}) Recorded unknown private API: ${privateAPIIdentifier}`);
161+
}
162+
this.privateAPIUsages.push({ context, privateAPIIdentifier });
163+
},
164+
recordTestStart: () => {
165+
this.privateAPIUsages.push({ context, privateAPIIdentifier: null });
166+
},
167+
};
168+
}
169+
170+
dump() {
171+
if (isBrowser) {
172+
window.dispatchEvent(new CustomEvent('privateApiUsageData', { detail: this.privateAPIUsages }));
173+
} else {
174+
try {
175+
const fs = require('fs');
176+
const path = require('path');
177+
178+
if (!fs.existsSync(outputDirectoryPaths.privateApiUsage)) {
179+
fs.mkdirSync(outputDirectoryPaths.privateApiUsage);
180+
}
181+
const filename = `node-${process.version.split('.')[0]}.json`;
182+
fs.writeFileSync(
183+
path.join(outputDirectoryPaths.privateApiUsage, filename),
184+
JSON.stringify(this.privateAPIUsages),
185+
{ encoding: 'utf-8' },
186+
);
187+
} catch (err) {
188+
console.log('Failed to write private API usage data: ', err);
189+
throw err;
190+
}
191+
}
192+
}
193+
}
194+
195+
return (module.exports = new PrivateApiRecorder());
196+
});

0 commit comments

Comments
 (0)