Skip to content

Commit

Permalink
Remove sinon sandboxes (MetaMask#788)
Browse files Browse the repository at this point in the history 
Sinon uses the "default sandbox" by default, and this is sufficient for
everything we are using Sinon for. People tend to use the
`createSandbox` method because they misunderstand how sandboxes work.
This often results in interwoven use of a module-specific sandbox and
the default sandbox, which leads to errors.

Instead we should use the default sandbox and restore it after each
test.
  • Loading branch information
@Gudahtt
Gudahtt authored Apr 21, 2022
1 parent 814c99d commit 7221c69
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 81 deletions.
9 changes: 4 additions & 5 deletions src/assets/CollectibleDetectionController.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { createSandbox, stub } from 'sinon';
import sinon from 'sinon';
import nock from 'nock';
import { NetworkController } from '../network/NetworkController';
import { PreferencesController } from '../user/PreferencesController';
Expand All @@ -18,7 +18,6 @@ describe('CollectibleDetectionController', () => {
let network: NetworkController;
let collectiblesController: CollectiblesController;
let assetsContract: AssetsContractController;
const sandbox = createSandbox();

beforeEach(async () => {
preferences = new PreferencesController();
Expand Down Expand Up @@ -198,7 +197,7 @@ describe('CollectibleDetectionController', () => {

afterEach(() => {
nock.cleanAll();
sandbox.reset();
sinon.restore();
});

it('should set default config', () => {
Expand All @@ -214,7 +213,7 @@ describe('CollectibleDetectionController', () => {

it('should poll and detect collectibles on interval while on mainnet', async () => {
await new Promise((resolve) => {
const mockCollectibles = stub(
const mockCollectibles = sinon.stub(
CollectibleDetectionController.prototype,
'detectCollectibles',
);
Expand Down Expand Up @@ -254,7 +253,7 @@ describe('CollectibleDetectionController', () => {

it('should not autodetect while not on mainnet', async () => {
await new Promise((resolve) => {
const mockCollectibles = stub(
const mockCollectibles = sinon.stub(
CollectibleDetectionController.prototype,
'detectCollectibles',
);
Expand Down
54 changes: 24 additions & 30 deletions src/assets/CollectiblesController.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { createSandbox } from 'sinon';
import sinon from 'sinon';
import nock from 'nock';
import HttpProvider from 'ethjs-provider-http';
import { PreferencesController } from '../user/PreferencesController';
Expand Down Expand Up @@ -46,7 +46,6 @@ describe('CollectiblesController', () => {
let preferences: PreferencesController;
let network: NetworkController;
let assetsContract: AssetsContractController;
const sandbox = createSandbox();

beforeEach(() => {
preferences = new PreferencesController();
Expand Down Expand Up @@ -75,10 +74,6 @@ describe('CollectiblesController', () => {
openSeaEnabled: true,
});

sandbox
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(true);

nock(OPEN_SEA_HOST)
.get(`${OPEN_SEA_PATH}/asset_contract/0x01`)
.reply(200, {
Expand Down Expand Up @@ -186,7 +181,7 @@ describe('CollectiblesController', () => {

afterEach(() => {
nock.cleanAll();
sandbox.reset();
sinon.restore();
});

it('should set default state', () => {
Expand Down Expand Up @@ -242,7 +237,7 @@ describe('CollectiblesController', () => {
const firstAddress = '0x123';
const secondAddress = '0x321';

sandbox
sinon
.stub(collectiblesController, 'getCollectibleInformation' as any)
.returns({ name: 'name', image: 'url', description: 'description' });
preferences.update({ selectedAddress: firstAddress });
Expand Down Expand Up @@ -393,7 +388,7 @@ describe('CollectiblesController', () => {
it('should add collectible erc721 and get collectible contract information from contract and OpenSea', async () => {
assetsContract.configure({ provider: MAINNET_PROVIDER });
const { selectedAddress, chainId } = collectiblesController.config;
sandbox
sinon
.stub(
collectiblesController,
'getCollectibleContractInformationFromApi' as any,
Expand Down Expand Up @@ -431,14 +426,14 @@ describe('CollectiblesController', () => {
it('should add collectible erc721 and get collectible contract information only from contract', async () => {
assetsContract.configure({ provider: MAINNET_PROVIDER });
const { selectedAddress, chainId } = collectiblesController.config;
sandbox
sinon
.stub(
collectiblesController,
'getCollectibleContractInformationFromApi' as any,
)
.returns(undefined);

sandbox
sinon
.stub(collectiblesController, 'getCollectibleInformationFromApi' as any)
.returns(undefined);
await collectiblesController.addCollectible(ERC721_KUDOSADDRESS, '1203');
Expand Down Expand Up @@ -472,7 +467,7 @@ describe('CollectiblesController', () => {
const firstNetworkType = 'rinkeby';
const secondNetworkType = 'ropsten';
const { selectedAddress } = collectiblesController.config;
sandbox
sinon
.stub(collectiblesController, 'getCollectibleInformation' as any)
.returns({ name: 'name', image: 'url', description: 'description' });

Expand Down Expand Up @@ -673,7 +668,11 @@ describe('CollectiblesController', () => {
const secondAddress = '0x321';
const { chainId } = collectiblesController.config;

sandbox
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(true);

sinon
.stub(collectiblesController, 'getCollectibleInformation' as any)
.returns({ name: 'name', image: 'url', description: 'description' });
preferences.update({ selectedAddress: firstAddress });
Expand Down Expand Up @@ -701,8 +700,7 @@ describe('CollectiblesController', () => {
});

it('should throw an error if selected address is not owner of input collectible', async () => {
sandbox.restore();
sandbox
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(false);
const firstAddress = '0x123';
Expand Down Expand Up @@ -769,7 +767,7 @@ describe('CollectiblesController', () => {

it('should remove collectible by selected address', async () => {
const { chainId } = collectiblesController.config;
sandbox
sinon
.stub(collectiblesController, 'getCollectibleInformation' as any)
.returns({ name: 'name', image: 'url', description: 'description' });
const firstAddress = '0x123';
Expand Down Expand Up @@ -799,7 +797,7 @@ describe('CollectiblesController', () => {
it('should remove collectible by provider type', async () => {
const { selectedAddress } = collectiblesController.config;

sandbox
sinon
.stub(collectiblesController, 'getCollectibleInformation' as any)
.returns({ name: 'name', image: 'url', description: 'description' });
const firstNetworkType = 'rinkeby';
Expand Down Expand Up @@ -903,7 +901,6 @@ describe('CollectiblesController', () => {
});

it('should not verify the ownership of an ERC-721 collectible with the wrong owner address', async () => {
sandbox.restore();
assetsContract.configure({ provider: MAINNET_PROVIDER });
const isOwner = await collectiblesController.isCollectibleOwner(
'0x0000000000000000000000000000000000000000',
Expand All @@ -924,7 +921,6 @@ describe('CollectiblesController', () => {
});

it('should not verify the ownership of an ERC-1155 collectible with the wrong owner address', async () => {
sandbox.restore();
assetsContract.configure({ provider: MAINNET_PROVIDER });
const isOwner = await collectiblesController.isCollectibleOwner(
'0x0000000000000000000000000000000000000000',
Expand All @@ -935,7 +931,6 @@ describe('CollectiblesController', () => {
});

it('should throw an error for an unsupported standard', async () => {
sandbox.restore();
assetsContract.configure({ provider: MAINNET_PROVIDER });
const error =
'Unable to verify ownership. Probably because the standard is not supported or the chain is incorrect';
Expand Down Expand Up @@ -1136,8 +1131,7 @@ describe('CollectiblesController', () => {
describe('checkAndUpdateCollectiblesOwnershipStatus', () => {
describe('checkAndUpdateAllCollectiblesOwnershipStatus', () => {
it('should check whether collectibles for the current selectedAddress/chainId combination are still owned by the selectedAddress and update the isCurrentlyOwned value to false when collectible is not still owned', async () => {
sandbox.restore();
sandbox
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(false);

Expand Down Expand Up @@ -1166,6 +1160,10 @@ describe('CollectiblesController', () => {
});

it('should check whether collectibles for the current selectedAddress/chainId combination are still owned by the selectedAddress and leave/set the isCurrentlyOwned value to true when collectible is still owned', async () => {
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(true);

const { selectedAddress, chainId } = collectiblesController.config;
await collectiblesController.addCollectible('0x02', '1', {
name: 'name',
Expand All @@ -1190,8 +1188,7 @@ describe('CollectiblesController', () => {
});

it('should check whether collectibles for the current selectedAddress/chainId combination are still owned by the selectedAddress and leave the isCurrentlyOwned value as is when collectible ownership check fails', async () => {
sandbox.restore();
sandbox
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.throws(new Error('Unable to verify ownership'));

Expand Down Expand Up @@ -1243,8 +1240,7 @@ describe('CollectiblesController', () => {
][0].isCurrentlyOwned,
).toBe(true);

sandbox.restore();
sandbox
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(false);

Expand Down Expand Up @@ -1285,8 +1281,7 @@ describe('CollectiblesController', () => {
][0].isCurrentlyOwned,
).toBe(true);

sandbox.restore();
sandbox
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(false);

Expand Down Expand Up @@ -1340,8 +1335,7 @@ describe('CollectiblesController', () => {
][0].isCurrentlyOwned,
).toBe(true);

sandbox.restore();
sandbox
sinon
.stub(collectiblesController, 'isCollectibleOwner' as any)
.returns(false);

Expand Down
55 changes: 29 additions & 26 deletions src/assets/TokenBalancesController.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { createSandbox, stub } from 'sinon';
import sinon from 'sinon';
import { BN } from 'ethereumjs-util';
import { NetworkController } from '../network/NetworkController';
import { PreferencesController } from '../user/PreferencesController';
Expand All @@ -11,8 +11,6 @@ import {
} from './TokenBalancesController';

describe('TokenBalancesController', () => {
const sandbox = createSandbox();

const getToken = (
tokenBalances: TokenBalancesController,
address: string,
Expand All @@ -22,7 +20,7 @@ describe('TokenBalancesController', () => {
};

afterEach(() => {
sandbox.restore();
sinon.restore();
});

it('should re-export BN', () => {
Expand All @@ -31,18 +29,18 @@ describe('TokenBalancesController', () => {

it('should set default state', () => {
const tokenBalances = new TokenBalancesController({
onTokensStateChange: stub(),
onTokensStateChange: sinon.stub(),
getSelectedAddress: () => '0x1234',
getERC20BalanceOf: stub(),
getERC20BalanceOf: sinon.stub(),
});
expect(tokenBalances.state).toStrictEqual({ contractBalances: {} });
});

it('should set default config', () => {
const tokenBalances = new TokenBalancesController({
onTokensStateChange: stub(),
onTokensStateChange: sinon.stub(),
getSelectedAddress: () => '0x1234',
getERC20BalanceOf: stub(),
getERC20BalanceOf: sinon.stub(),
});
expect(tokenBalances.config).toStrictEqual({
interval: 180000,
Expand All @@ -52,12 +50,15 @@ describe('TokenBalancesController', () => {

it('should poll and update balances in the right interval', async () => {
await new Promise<void>((resolve) => {
const mock = stub(TokenBalancesController.prototype, 'updateBalances');
const mock = sinon.stub(
TokenBalancesController.prototype,
'updateBalances',
);
new TokenBalancesController(
{
onTokensStateChange: stub(),
onTokensStateChange: sinon.stub(),
getSelectedAddress: () => '0x1234',
getERC20BalanceOf: stub(),
getERC20BalanceOf: sinon.stub(),
},
{ interval: 10 },
);
Expand All @@ -74,27 +75,27 @@ describe('TokenBalancesController', () => {
it('should not update rates if disabled', async () => {
const tokenBalances = new TokenBalancesController(
{
onTokensStateChange: stub(),
onTokensStateChange: sinon.stub(),
getSelectedAddress: () => '0x1234',
getERC20BalanceOf: stub(),
getERC20BalanceOf: sinon.stub(),
},
{
disabled: true,
interval: 10,
},
);
const mock = stub(tokenBalances, 'update');
const mock = sinon.stub(tokenBalances, 'update');
await tokenBalances.updateBalances();
expect(mock.called).toBe(false);
});

it('should clear previous interval', async () => {
const mock = stub(global, 'clearTimeout');
const mock = sinon.stub(global, 'clearTimeout');
const tokenBalances = new TokenBalancesController(
{
onTokensStateChange: stub(),
onTokensStateChange: sinon.stub(),
getSelectedAddress: () => '0x1234',
getERC20BalanceOf: stub(),
getERC20BalanceOf: sinon.stub(),
},
{ interval: 1337 },
);
Expand All @@ -120,7 +121,7 @@ describe('TokenBalancesController', () => {
{
onTokensStateChange: (listener) => assets.subscribe(listener),
getSelectedAddress: () => preferences.state.selectedAddress,
getERC20BalanceOf: stub().returns(new BN(1)),
getERC20BalanceOf: sinon.stub().returns(new BN(1)),
},
{ interval: 1337, tokens: [{ address, decimals: 18, symbol: 'EOS' }] },
);
Expand All @@ -147,9 +148,9 @@ describe('TokenBalancesController', () => {
});
const errorMsg = 'Failed to get balance';
const address = '0x86fa049857e0209aa7d9e616f7eb3b3b78ecfdb0';
const getERC20BalanceOfStub = stub().returns(
Promise.reject(new Error(errorMsg)),
);
const getERC20BalanceOfStub = sinon
.stub()
.returns(Promise.reject(new Error(errorMsg)));
const tokenBalances = new TokenBalancesController(
{
onTokensStateChange: (listener) => assets.subscribe(listener),
Expand Down Expand Up @@ -191,11 +192,13 @@ describe('TokenBalancesController', () => {
onNetworkStateChange: (listener) => network.subscribe(listener),
});

const supportsInterfaceStub = stub().returns(Promise.resolve(false));
const supportsInterfaceStub = sinon.stub().returns(Promise.resolve(false));

stub(tokensController, '_createEthersContract').callsFake(() =>
Promise.resolve({ supportsInterface: supportsInterfaceStub }),
);
sinon
.stub(tokensController, '_createEthersContract')
.callsFake(() =>
Promise.resolve({ supportsInterface: supportsInterfaceStub }),
);

const tokenBalances = new TokenBalancesController(
{
Expand All @@ -206,7 +209,7 @@ describe('TokenBalancesController', () => {
},
{ interval: 1337 },
);
const updateBalances = sandbox.stub(tokenBalances, 'updateBalances');
const updateBalances = sinon.stub(tokenBalances, 'updateBalances');
await tokensController.addToken('0x00', 'FOO', 18);
const { tokens } = tokensController.state;
const found = tokens.filter((token: Token) => token.address === '0x00');
Expand Down
Loading

0 comments on commit 7221c69

Please sign in to comment.