Skip to content

Commit

Permalink
wip private api recorder
Browse files Browse the repository at this point in the history
ECO-4821

note i haven't been very consistent with how many times per test i
record a private api usage

TODO there's some work to do with making some of these APIs’ names
consistent (e.g. get rid of connection.connectionManager)
  • Loading branch information
lawrence-forooghian committed Jun 12, 2024
1 parent 15d8904 commit cc2b8a8
Show file tree
Hide file tree
Showing 31 changed files with 789 additions and 64 deletions.
76 changes: 74 additions & 2 deletions docs/internal/private-api-usage.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,25 @@ None

### `test/realtime/delta.test.js`

Marked in code.

- `channel._lastPayload.messageId = null`
- to make decoding fail

Note that this test passes an overridden VCDiff decoder, but some SDKs don't use a VCDiff decoder

### `test/realtime/encoding.test.js`

Marked in code.

- `var BufferUtils = Ably.Realtime.Platform.BufferUtils;`
- `var Defaults = Ably.Rest.Platform.Defaults;`
- just used for accessing library’s baked-in protocol version, to pass to `request()`

### `test/realtime/presence.test.js`

Marked in code.

- `var createPM = Ably.protocolMessageFromDeserialized;`
- `var PresenceMessage = Ably.Realtime.PresenceMessage;`
- replacing `channel.sendPresence` with a version that checks the presence message’s client ID
Expand All @@ -47,6 +55,8 @@ None

### `test/realtime/event_emitter.test.js`

Marked in code.

- `eventEmitter.emit('custom');` — RTE6 says that `emit` is internal

### `test/realtime/api.test.js`
Expand All @@ -55,15 +65,18 @@ None

### `test/realtime/crypto.test.js`

Marked in code.

- `var BufferUtils = Ably.Realtime.Platform.BufferUtils;`
- `var msgpack = typeof window == 'object' ? Ably.msgpack : require('@ably/msgpack-js');`
- `Message.encode(testMessage, channelOpts)`
- `Message.decode(encryptedMessage, channelOpts);`
- `Message.fromValues(`
- `expect(channel.channelOptions.cipher.algorithm).to.equal('aes');``channel.channelOptions` is not public API

### `test/realtime/failure.test.js`

Marked in code.

- `webSocketConnectTimeout: 50` client option
- replacing `channel.processMessage` to drop `ATTACHED`
- replacing `realtime.connection.connectionManager.activeProtocol.transport.onProtocolMessage` to drop `ACK`
Expand All @@ -75,6 +88,8 @@ None

### `test/realtime/channel.test.js`

Marked in code.

- `var createPM = Ably.protocolMessageFromDeserialized;`
- `expect(channel.channelOptions).to.deep.equal(channelOptions, 'Check requested channel options');` (`channelOptions` isn’t public API)
- listens for `channel._allChannelChanges.on(['update'],`
Expand All @@ -88,13 +103,18 @@ None

### `test/realtime/auth.test.js`

Marked in code.

- `var http = new Ably.Realtime._Http();` — just uses this as an HTTP client to fetch a JWT
- reads `realtime.auth.method` to check it’s `token`
- spies on `rest.time` to count how many times called
- checks `rest.serverTimeOffset`
- spies on `transport.send` to look for an outgoing `AUTH` and check its properties

### `test/realtime/transports.test.js`

Skipped marking in code.

Transports are a JS-specific concept so might not be worth worrying too much about the contents of this file

- `const Defaults = Ably.Rest.Platform.Defaults;`
Expand All @@ -111,12 +131,18 @@ Transports are a JS-specific concept so might not be worth worrying too much abo

### `test/realtime/utils.test.js`

Marked in code.

Ah, I just realised that some of the properties on `shared_helper` actually refer to properties of the library, e.g. `helper.Utils` is actually `Ably.Realtime.Utils`. So perhaps I missed some usages of internal APIs in earlier files. But can figure that out later.

(I’ve addressed this in the context of marking the code; utils stuff is properly marked.)

- this entire file is a test of the internal `utils.getRetryTime(…)` method

### `test/realtime/resume.test.js`

Marked in code.

- `connectionManager.once('transport.active',` and inside the callback it makes an assertion on `transport.params.mode`
- sets a channel’s state: `suspendedChannel.state = 'suspended';`
- sabotages a resume by setting `connection.connectionManager`’s `conectionKey` and `connectionId` to garbage
Expand All @@ -132,6 +158,8 @@ Ah, I just realised that some of the properties on `shared_helper` actually refe

### `test/realtime/message.test.js`

Marked in code.

- `let config = Ably.Realtime.Platform.Config;`
- `var createPM = Ably.protocolMessageFromDeserialized;`
- modifies `transport.send` to check the `clientId` on the outgoing `MESSAGE`
Expand All @@ -141,6 +169,8 @@ Ah, I just realised that some of the properties on `shared_helper` actually refe

### `test/realtime/connection.test.js`

Marked in code.

- creates a `recover` client option which uses knowledge of ably-js’s serialization of recovery key
- spies on `transport.send` to listen for `MESSAGE`, check its properties, and then continue the test
- calls `connectionManager.disconnectAllTransports();`
Expand All @@ -149,6 +179,8 @@ Ah, I just realised that some of the properties on `shared_helper` actually refe

### `test/realtime/init.test.js`

Marked in code.

- accesses `var transport = realtime.connection.connectionManager.activeProtocol.transport.uri` or `.recvRequest.recvUri` to check the `v=3` query parameter
- `expect(realtime.options).to.deep.equal(realtime.connection.connectionManager.options);`
- checks `realtime.connection.connectionManager.httpHosts[0];` to check it’s using correct default host, also checks length of that array
Expand All @@ -165,6 +197,8 @@ None

### `test/realtime/connectivity.test.js`

Marked in code.

- directly calls `new Ably.Realtime._Http().checkConnectivity()` and checks it succeeds (i.e. directly tests this method)

### `test/realtime/reauth.test.js`
Expand All @@ -173,19 +207,27 @@ None

### `test/realtime/sync.test.js`

Marked in code.

- calling `channel.processMessage` to inject an `ATTACHED` with a presence flag, and then a `SYNC`, later on a `PRESENCE`
- spies on `channel.processMessage` to, after processing a received `SYNC`, inject a `PRESENCE`

### `test/browser/simple.test.js`

Skipped marking in code since it’s browser-specific.

- checks whether a transport is available using `transport in Ably.Realtime.ConnectionManager.supportedTransports(Ably.Realtime._transports)`

### `test/browser/http.test.js`

Skipped marking in code since it’s browser-specific.

- changes `Ably.Rest.Platform.Config.xhrSupported` to false to make it use Fetch

### `test/browser/connection.test.js`

Skipped marking in code since it’s browser-specific.

(I guess that this is a test that we might not include in the unified test suite.)

- uses knowledge of library’s usage of `window.sessionStorage` for transport preference and recovery key
Expand All @@ -197,6 +239,8 @@ None

### `test/browser/modular.test.js`

Skipped marking in code since it’s browser-specific.

Another file that probably wouldn’t be part of a unified test suite.

- `const BufferUtils = BaseRest.Platform.BufferUtils;`
Expand Down Expand Up @@ -227,10 +271,14 @@ N/A

### `test/common/modules/client_module.js`

- uses `Ably.Realtime.Utils` for its `mixin` function
Marked in code.

- uses `Ably.Realtime.Utils` for its `mixin` and `copy` function

### `test/common/modules/testapp_manager.js`

Marked in code.

- uses `ably.Realtime.Platform.BufferUtils`

### `test/common/modules/testapp_module.js`
Expand All @@ -239,6 +287,8 @@ None

### `test/common/modules/shared_helper.js`

Marked in code.

- `var utils = clientModule.Ably.Realtime.Utils;`
- `var platform = clientModule.Ably.Realtime.Platform;`
- uses `platform.Config.nextTick()`
Expand Down Expand Up @@ -314,15 +364,21 @@ N/A

### `test/rest/bufferutils.test.js`

Skipped marking in code.

This file is a unit test of `Ably.Realtime.Platform.BufferUtils`; something we wouldn’t include in a unified test suite.

### `test/rest/presence.test.js`

Marked in code.

- `var BufferUtils = Ably.Realtime.Platform.BufferUtils;`
- I’m going to stop mentioning the use of BufferUtils as a test util now; the pattern is clear and not hard to fix.

### `test/rest/fallbacks.test.js`

Marked in code.

- checks `rest._currentFallback.{host, validUntil}` to check that the working fallback has been stored correctly
- modifies `rest._currentFallback.validUntil` to check library correctly forgets stored fallback

Expand All @@ -344,12 +400,16 @@ None

### `test/rest/auth.test.js`

Marked in code.

- ditto will stop mentioning usage of `Utils` for stuff that will be pretty easy to replace

None

### `test/rest/http.test.js`

Marked in code.

- accesses `Ably.Rest.Platform.Defaults` to check its `version` is being used to populate `Ably-Agent`
- spies on `rest.http.do` to make assertions about request headers
- replaces `rest.http.do` to simulate a 204 response
Expand All @@ -360,25 +420,35 @@ None

### `test/rest/push.test.js`

Marked in code.

None

### `test/rest/message.test.js`

Marked in code.

- spies on `channel._publish` to verify that client does / doesn’t add a `clientId`
- ditto to check that idempotent REST publishing generates message IDs
- ditto to check `params`
- overrides `Ably.Rest._Http.doUri` to fake a publish error

### `test/rest/init.test.js`

Marked in code.

- accesses various properties of `rest.options` to check the effect of passing various things to the constructor

### `test/rest/history.test.js`

Marked in code.

None

### `test/rest/defaults.test.js`

TODO decide what to do re marking this one.

This appears to be a unit test of the `Defaults` class’s `normaliseOptions()`, `getHosts()`, and `getPort()` methods. But I imagine it’s actually providing the test suite’s coverage of a bunch of spec points which aren’t written in terms of this API.

### `test/rest/status.test.js`
Expand All @@ -387,6 +457,8 @@ None

### `test/rest/request.test.js`

Marked in code.

- overrides `rest.http.do()` to check `X-Ably-Version` request header

### `test/package/browser/template/server/resources/runTest.js`
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 @@ -13,5 +13,9 @@ define(function () {
shared_helper: { browser: 'test/common/modules/shared_helper', node: 'test/common/modules/shared_helper' },
async: { browser: 'node_modules/async/lib/async' },
chai: { browser: 'node_modules/chai/chai', node: 'node_modules/chai/chai' },
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
Loading

0 comments on commit cc2b8a8

Please sign in to comment.