Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve unit tests #219

Merged
merged 6 commits into from Jul 26, 2022
Merged

Improve unit tests #219

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
afeb53a
Improve unit tests
Gudahtt Jul 19, 2022
26185cd
Merge branch 'main' into improve-unit-tests
Gudahtt Jul 20, 2022
ebea8b8
Add comment explaining where mock is setup
Gudahtt Jul 26, 2022
7374597
Simplify mock provider intialization
Gudahtt Jul 26, 2022
6508da2
Improve test function return type
Gudahtt Jul 26, 2022
8eee8b8
Remove protection against responding before init
Gudahtt Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions jest.config.js
View file
Open in desktop
Expand Up @@ -20,10 +20,10 @@ module.exports = {
coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'],
coverageThreshold: {
global: {
branches: 55.75,
functions: 52.81,
lines: 58.22,
statements: 58.52,
branches: 56.19,
functions: 53.93,
lines: 59.01,
statements: 59.29,
},
},
projects: [
Expand Down
21 changes: 0 additions & 21 deletions mocks/DuplexStream.ts
View file
Open in desktop

This file was deleted.

177 changes: 112 additions & 65 deletions src/MetaMaskInpageProvider.test.ts
View file
Open in desktop
@@ -1,4 +1,6 @@
import { MockDuplexStream } from '../mocks/DuplexStream';
import { JsonRpcRequest } from 'json-rpc-engine';
import { MockConnectionStream } from '../test/mocks/MockConnectionStream';
import { getDeconstructedPromise } from '../test/get-deconstructed-promise';
import {
MetaMaskInpageProviderStreamName,
MetaMaskInpageProvider,
Expand All @@ -11,45 +13,92 @@ import messages from './messages';
* {@link MetaMaskInpageProvider._initializeStateAsync}. This helper function
* returns a provider initialized with the specified values.
*
* @param options - Options bag. See {@link MetaMaskInpageProvider._initializeState}.
* @returns A tuple of the initialized provider and its stream.
* @param options - Options bag.
* @param options.initialState - The initial provider state returned on
* initialization. See {@link MetaMaskInpageProvider._initializeState}.
* @param options.onMethodCalled - A set of configuration objects for adding
* method-specific callbacks.
* @param options.onMethodCalled[].substream - The substream of the method that
* the callback is for.
* @param options.onMethodCalled[].method - The name of the method that the
* callback is for.
* @param options.onMethodCalled[].callback - The method callback.
* @returns A tuple of the initialized provider, its stream, and an "onWrite"
* stub that can be used to inspect message sent by the provider.
*/
async function getInitializedProvider({
accounts = [],
chainId = '0x0',
isUnlocked = true,
networkVersion = '0',
}: Partial<Parameters<MetaMaskInpageProvider['_initializeState']>[0]> = {}) {
// This will be called via the constructor
const requestMock = jest
.spyOn(MetaMaskInpageProvider.prototype, 'request')
.mockImplementationOnce(async () => {
return {
accounts,
chainId,
isUnlocked,
networkVersion,
};
});

const mockStream = new MockDuplexStream();
const inpageProvider = new MetaMaskInpageProvider(mockStream);
initialState: {
accounts = [],
chainId = '0x0',
isUnlocked = true,
networkVersion = '0',
} = {},
onMethodCalled = [],
}: {
initialState?: Partial<
Parameters<MetaMaskInpageProvider['_initializeState']>[0]
>;
onMethodCalled?: {
substream: string;
method: string;
callback: (data: JsonRpcRequest<unknown>) => void;
}[];
} = {}) {
let isInitialized = false;
const onWrite = jest.fn();
const connectionStream = new MockConnectionStream((name, data) => {
if (!isInitialized) {
if (
name === 'metamask-provider' &&
data.method === 'metamask_getProviderState'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that with MockDuplexStream, we didn't handle checking for metamask_getProviderState. I think I might know the answer to this, but why do we have to do this now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! Previously the initial provider state was set by mocking out the request function and returning the initial state directly, bypassing the entire RPC layer. metamask_getProviderState was never really called, and any code past that request call was not run at all.

That mock was removed, and instead now we're using a mock connection instead. As such, now the entire initialization process is being tested, rather than just up to the .request invocation.

This means our mock has to behave roughly like a real connection with the wallet would though, so it's a bit more complex. This mock is setup to handle the initialization call, then pass along all other calls to the configured callbacks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool. I like that!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conversation had me re-thinking this initialization code, and in retrospect I think this protection is unnecessary and potentially harmful. This logic was simplified here, with the explanation for why in the commit message: 8eee8b8

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. Sounds good!

) {
isInitialized = true;
// Wrap in `setImmediate` to ensure a reply is recieved by the provider
// after the provider has processed the request, to ensure that the
// provider recognizes the id.
setImmediate(() =>
connectionStream.reply('metamask-provider', {
id: onWrite.mock.calls[0][1].id,
jsonrpc: '2.0',
result: {
accounts,
chainId,
isUnlocked,
networkVersion,
},
}),
);
} else {
// Ignore other messages before initialization
return;
}
}
for (const { substream, method, callback } of onMethodCalled) {
if (name === substream && data.method === method) {
// Wrap in `setImmediate` to ensure a reply is recieved by the provider
// after the provider has processed the request, to ensure that the
// provider recognizes the id.
setImmediate(() => callback(data));
}
}
onWrite(name, data);
});

// Relinquish control of the event loop to ensure that the mocked state is
// retrieved.
await new Promise<void>((resolve) => setTimeout(() => resolve(), 1));
const provider = new MetaMaskInpageProvider(connectionStream);
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
const { resolve: resolveInitialized, promise: waitForInitialization } =
getDeconstructedPromise();
provider.on('_initialized', resolveInitialized);

expect(requestMock).toHaveBeenCalledTimes(1); // Sanity check
requestMock.mockRestore(); // Get rid of the mock
await waitForInitialization;

return [inpageProvider, mockStream] as const;
return [provider, connectionStream, onWrite] as const;
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
}

describe('MetaMaskInpageProvider: RPC', () => {
const MOCK_ERROR_MESSAGE = 'Did you specify a mock return value?';

function initializeProvider() {
const mockStream = new MockDuplexStream();
const mockStream = new MockConnectionStream();
const provider: any | MetaMaskInpageProvider = new MetaMaskInpageProvider(
mockStream,
);
Expand Down Expand Up @@ -670,13 +719,10 @@ describe('MetaMaskInpageProvider: RPC', () => {
resolve(undefined);
});

mockStream.push({
name: MetaMaskInpageProviderStreamName,
data: {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
params: { chainId: '0x1', networkVersion: '1' },
},
mockStream.notify(MetaMaskInpageProviderStreamName, {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
params: { chainId: '0x1', networkVersion: '1' },
});
});
});
Expand All @@ -690,13 +736,10 @@ describe('MetaMaskInpageProvider: RPC', () => {
resolve(undefined);
});

mockStream.push({
name: MetaMaskInpageProviderStreamName,
data: {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
params: { chainId: '0x1', networkVersion: '1' },
},
mockStream.notify(MetaMaskInpageProviderStreamName, {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
params: { chainId: '0x1', networkVersion: '1' },
});
});
});
Expand All @@ -717,16 +760,13 @@ describe('MetaMaskInpageProvider: RPC', () => {
resolve();
});

mockStream.push({
name: MetaMaskInpageProviderStreamName,
data: {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
// A "loading" networkVersion indicates the network is changing.
// Although the chainId is different, chainChanged should not be
// emitted in this case.
params: { chainId: '0x1', networkVersion: 'loading' },
},
mockStream.notify(MetaMaskInpageProviderStreamName, {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
// A "loading" networkVersion indicates the network is changing.
// Although the chainId is different, chainChanged should not be
// emitted in this case.
params: { chainId: '0x1', networkVersion: 'loading' },
});
});

Expand All @@ -745,13 +785,10 @@ describe('MetaMaskInpageProvider: RPC', () => {
resolve();
});

mockStream.push({
name: MetaMaskInpageProviderStreamName,
data: {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
params: { chainId: '0x1', networkVersion: '1' },
},
mockStream.notify(MetaMaskInpageProviderStreamName, {
jsonrpc: '2.0',
method: 'metamask_chainChanged',
params: { chainId: '0x1', networkVersion: '1' },
});
});

Expand All @@ -771,12 +808,12 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
describe('constructor', () => {
it('succeeds if stream is provided', () => {
expect(
() => new MetaMaskInpageProvider(new MockDuplexStream()),
() => new MetaMaskInpageProvider(new MockConnectionStream()),
).not.toThrow();
});

it('succeeds if stream and valid options are provided', () => {
const stream = new MockDuplexStream();
const stream = new MockConnectionStream();

expect(
() =>
Expand Down Expand Up @@ -816,7 +853,7 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
});

it('accepts valid custom logger', () => {
const stream = new MockDuplexStream();
const stream = new MockConnectionStream();
const customLogger = {
debug: console.debug,
error: console.error,
Expand Down Expand Up @@ -847,7 +884,7 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
};
});

const mockStream = new MockDuplexStream();
const mockStream = new MockConnectionStream();
const inpageProvider = new MetaMaskInpageProvider(mockStream);

await new Promise<void>((resolve) => setTimeout(() => resolve(), 1));
Expand All @@ -861,7 +898,9 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {

describe('isConnected', () => {
it('returns isConnected state', () => {
const provider: any = new MetaMaskInpageProvider(new MockDuplexStream());
const provider: any = new MetaMaskInpageProvider(
new MockConnectionStream(),
);
provider.autoRefreshOnNetworkChange = false;

expect(provider.isConnected()).toBe(false);
Expand All @@ -875,4 +914,12 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
expect(provider.isConnected()).toBe(false);
});
});

describe('isMetaMask', () => {
it('should be set to "true"', async () => {
const [provider] = await getInitializedProvider();

expect(provider.isMetaMask).toBe(true);
});
});
});