From daa20b4b63dc0509ac4911ebbb1bf7534e299fd6 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Sat, 31 Aug 2019 13:34:27 -0300 Subject: [PATCH] Return after rejecting promise in action (#7079) * Add missing test descriptions * Fix async tests that expect a rejection These tests expected the function under test to return a rejected promise, and had assertions to be run in the `catch` clause. However, the tests would still pass if the function didn't reject, with the assertions never being run. The tests have been updated to fail if the function doesn't throw. * Handle ignored promise rejection In the case where `forceUpdateMetamaskState` rejects, the function `setSeedPhraseBackedUp` would never resolve. It has been updated to pass along the rejection instead. * Return after rejecting promise in action A few actions would continue after encountering an error, resulting in errors being compounded. Instead all actions will now return after encountering an error (which it looks like was the intention in these cases anyway). --- test/e2e/web3.spec.js | 4 +- test/unit/app/buy-eth-url.spec.js | 2 +- test/unit/ui/app/actions.spec.js | 249 ++++++++++-------- test/unit/ui/app/selectors.spec.js | 14 +- .../app/dropdowns/tests/dropdown.test.js | 2 +- ui/app/store/actions.js | 18 +- 6 files changed, 156 insertions(+), 133 deletions(-) diff --git a/test/e2e/web3.spec.js b/test/e2e/web3.spec.js index 36a1c0509..42f235045 100644 --- a/test/e2e/web3.spec.js +++ b/test/e2e/web3.spec.js @@ -181,7 +181,7 @@ describe('Using MetaMask with an existing account', function () { await delay(largeDelayMs * 2) }) - it('', async () => { + it('connects to dapp', async () => { await openNewPage(driver, 'http://127.0.0.1:8080/') await delay(regularDelayMs) @@ -198,8 +198,6 @@ describe('Using MetaMask with an existing account', function () { await driver.switchTo().window(dapp) await delay(regularDelayMs) - - }) }) diff --git a/test/unit/app/buy-eth-url.spec.js b/test/unit/app/buy-eth-url.spec.js index 6cf3e7d75..7db992244 100644 --- a/test/unit/app/buy-eth-url.spec.js +++ b/test/unit/app/buy-eth-url.spec.js @@ -1,7 +1,7 @@ const assert = require('assert') const getBuyEthUrl = require('../../../app/scripts/lib/buy-eth-url') -describe('', function () { +describe('buy-eth-url', function () { const mainnet = { network: '1', amount: 5, diff --git a/test/unit/ui/app/actions.spec.js b/test/unit/ui/app/actions.spec.js index 5bf82a185..33b223c62 100644 --- a/test/unit/ui/app/actions.spec.js +++ b/test/unit/ui/app/actions.spec.js @@ -75,7 +75,7 @@ describe('Actions', () => { verifySeedPhraseSpy.restore() }) - it('', async () => { + it('calls submitPassword and verifySeedPhrase', async () => { const store = mockStore({}) @@ -89,7 +89,7 @@ describe('Actions', () => { }) }) - it('errors on submitPassword will fail', () => { + it('errors on submitPassword will fail', async () => { const store = mockStore({}) @@ -107,13 +107,15 @@ describe('Actions', () => { callback(new Error('error in submitPassword')) }) - return store.dispatch(actions.tryUnlockMetamask('test')) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.tryUnlockMetamask('test')) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) - it('displays warning error and unlock failed when verifySeed fails', () => { + it('displays warning error and unlock failed when verifySeed fails', async () => { const store = mockStore({}) const displayWarningError = [ { type: 'DISPLAY_WARNING', value: 'error' } ] const unlockFailedError = [ { type: 'UNLOCK_FAILED', value: 'error' } ] @@ -123,14 +125,16 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.tryUnlockMetamask('test')) - .catch(() => { - const actions = store.getActions() - const warning = actions.filter(action => action.type === 'DISPLAY_WARNING') - const unlockFailed = actions.filter(action => action.type === 'UNLOCK_FAILED') - assert.deepEqual(warning, displayWarningError) - assert.deepEqual(unlockFailed, unlockFailedError) - }) + try { + await store.dispatch(actions.tryUnlockMetamask('test')) + assert.fail('Should have thrown error') + } catch (_) { + const actions = store.getActions() + const warning = actions.filter(action => action.type === 'DISPLAY_WARNING') + const unlockFailed = actions.filter(action => action.type === 'UNLOCK_FAILED') + assert.deepEqual(warning, displayWarningError) + assert.deepEqual(unlockFailed, unlockFailedError) + } }) }) @@ -142,19 +146,21 @@ describe('Actions', () => { createNewVaultAndRestoreSpy.restore() }) - it('restores new vault', () => { + it('restores new vault', async () => { const store = mockStore({}) createNewVaultAndRestoreSpy = sinon.spy(background, 'createNewVaultAndRestore') - return store.dispatch(actions.createNewVaultAndRestore()) - .catch(() => { - assert(createNewVaultAndRestoreSpy.calledOnce) - }) + + try { + await store.dispatch(actions.createNewVaultAndRestore()) + assert.fail('Should have thrown error') + } catch (_) { + assert(createNewVaultAndRestoreSpy.calledOnce) + } }) - it('errors when callback in createNewVaultAndRestore throws', () => { - + it('errors when callback in createNewVaultAndRestore throws', async () => { const store = mockStore({}) const expectedActions = [ @@ -169,10 +175,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.createNewVaultAndRestore()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.createNewVaultAndRestore()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -190,7 +198,7 @@ describe('Actions', () => { }) }) - it('displays warning error message then callback in background errors', () => { + it('displays warning error message then callback in background errors', async () => { const store = mockStore() const expectedActions = [ @@ -204,10 +212,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.requestRevealSeedWords()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.requestRevealSeedWords()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -236,7 +246,7 @@ describe('Actions', () => { }) }) - it('displays warning error message when removeAccount callback errors', () => { + it('displays warning error message when removeAccount callback errors', async () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -248,10 +258,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.removeAccount('0xe18035bf8712672935fdb4e5e431b1a0183d2dfc')) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.removeAccount('0xe18035bf8712672935fdb4e5e431b1a0183d2dfc')) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -267,7 +279,7 @@ describe('Actions', () => { addNewKeyringSpy.restore() }) - it('', () => { + it('calls addNewKeyring', () => { const privateKey = 'c87509a1c067bbde78beb793e6fa76530b6382a4c0241e5e4a9ec0a0f44dc0d3' const store = mockStore() @@ -301,7 +313,7 @@ describe('Actions', () => { resetAccountSpy.restore() }) - it('', () => { + it('resets account', async () => { const store = mockStore() @@ -320,7 +332,7 @@ describe('Actions', () => { }) }) - it('', () => { + it('throws if resetAccount throws', async () => { const store = mockStore() const expectedActions = [ @@ -334,10 +346,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.resetAccount()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.resetAccount()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -362,7 +376,7 @@ describe('Actions', () => { }) }) - it('displays warning error message when importAccount in background callback errors', () => { + it('displays warning error message when importAccount in background callback errors', async () => { const store = mockStore() const expectedActions = [ @@ -376,10 +390,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.importNewAccount()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.importNewAccount()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -391,7 +407,7 @@ describe('Actions', () => { addNewAccountSpy.restore() }) - it('', () => { + it('Adds a new account', () => { const store = mockStore({ metamask: devState }) addNewAccountSpy = sinon.spy(background, 'addNewAccount') @@ -415,14 +431,14 @@ describe('Actions', () => { setCurrentCurrencySpy.restore() }) - it('', () => { + it('calls setCurrentCurrency', () => { const store = mockStore() store.dispatch(actions.setCurrentCurrency('jpy')) assert(setCurrentCurrencySpy.calledOnce) }) - it('', () => { + it('throws if setCurrentCurrency throws', () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -471,7 +487,7 @@ describe('Actions', () => { }) - it('errors when signMessage in background throws', () => { + it('errors when signMessage in background throws', async () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -485,10 +501,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.signMsg()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.signMsg()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -514,7 +532,7 @@ describe('Actions', () => { signPersonalMessageSpy.restore() }) - it('', () => { + it('calls signPersonalMessage', () => { const store = mockStore() signPersonalMessageSpy = sinon.spy(background, 'signPersonalMessage') @@ -526,7 +544,7 @@ describe('Actions', () => { }) - it('', () => { + it('throws if signPersonalMessage throws', async () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -540,10 +558,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.signPersonalMsg(msgParams)) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.signPersonalMsg(msgParams)) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -595,7 +615,7 @@ describe('Actions', () => { tokenSpy.restore() }) - it('', () => { + it('calls eth.contract', () => { const store = mockStore() store.dispatch(actions.signTokenTx()) assert(tokenSpy.calledOnce) @@ -609,7 +629,7 @@ describe('Actions', () => { backgroundSetLockedSpy.restore() }) - it('', () => { + it('calls setLocked', () => { const store = mockStore() backgroundSetLockedSpy = sinon.spy(background, 'setLocked') @@ -695,7 +715,7 @@ describe('Actions', () => { assert(setSelectedAddressSpy.calledOnce) }) - it('', () => { + it('displays warning if setSelectedAddress throws', () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -706,7 +726,6 @@ describe('Actions', () => { callback(new Error('error')) }) - store.dispatch(actions.showAccountDetail()) assert.deepEqual(store.getActions(), expectedActions) }) @@ -732,23 +751,24 @@ describe('Actions', () => { }) }) - it('errors when addToken in background throws', () => { + it('errors when addToken in background throws', async () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, { type: 'HIDE_LOADING_INDICATION' }, { type: 'DISPLAY_WARNING', value: 'error' }, - { type: 'UPDATE_TOKENS', newTokens: undefined }, ] addTokenSpy.callsFake((_, __, ___, ____, callback) => { callback(new Error('error')) }) - return store.dispatch(actions.addToken()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.addToken()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -772,23 +792,24 @@ describe('Actions', () => { }) }) - it('errors when removeToken in background fails', () => { + it('errors when removeToken in background fails', async () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, { type: 'HIDE_LOADING_INDICATION' }, { type: 'DISPLAY_WARNING', value: 'error' }, - { type: 'UPDATE_TOKENS', newTokens: undefined }, ] removeTokenSpy.callsFake((_, callback) => { callback(new Error('error')) }) - store.dispatch(actions.removeToken()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.removeToken()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -805,12 +826,12 @@ describe('Actions', () => { setProviderTypeSpy.restore() }) - it('', () => { + it('calls setProviderType', () => { store.dispatch(actions.setProviderType()) assert(setProviderTypeSpy.calledOnce) }) - it('', () => { + it('displays warning when setProviderType throws', () => { const expectedActions = [ { type: 'DISPLAY_WARNING', value: 'Had a problem changing networks!' }, ] @@ -836,13 +857,13 @@ describe('Actions', () => { setRpcTargetSpy.restore() }) - it('', () => { + it('calls setRpcTarget', () => { const store = mockStore() store.dispatch(actions.setRpcTarget('http://localhost:8545')) assert(setRpcTargetSpy.calledOnce) }) - it('', () => { + it('displays warning when setRpcTarget throws', () => { const store = mockStore() const expectedActions = [ { type: 'DISPLAY_WARNING', value: 'Had a problem changing networks!' }, @@ -868,7 +889,7 @@ describe('Actions', () => { addToAddressBookSpy.restore() }) - it('', () => { + it('calls setAddressBook', () => { const store = mockStore({ metamask: devState }) store.dispatch(actions.addToAddressBook('test')) assert(addToAddressBookSpy.calledOnce) @@ -902,7 +923,7 @@ describe('Actions', () => { }) }) - it('returns action errors when first func callback errors', () => { + it('returns action errors when first func callback errors', async () => { const store = mockStore(devState) const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -915,13 +936,15 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.exportAccount(password, '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc')) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.exportAccount(password, '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc')) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) - it('returns action errors when second func callback errors', () => { + it('returns action errors when second func callback errors', async () => { const store = mockStore(devState) const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -934,10 +957,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.exportAccount(password, '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc')) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.exportAccount(password, '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc')) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -948,7 +973,7 @@ describe('Actions', () => { setAccountLabelSpy = sinon.stub(background, 'setAccountLabel') }) - it('', () => { + it('calls setAccountLabel', () => { const store = mockStore() store.dispatch(actions.setAccountLabel('0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc', 'test')) assert(setAccountLabelSpy.calledOnce) @@ -968,7 +993,7 @@ describe('Actions', () => { .reply(200) }) - it('', () => { + it('calls expected actions', () => { const store = mockStore() // issue with dispatch action in callback not showing const expectedActions = [ @@ -999,7 +1024,7 @@ describe('Actions', () => { assert(setFeatureFlagSpy.calledOnce) }) - it('errors when setFeatureFlag in background throws', () => { + it('errors when setFeatureFlag in background throws', async () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -1011,10 +1036,12 @@ describe('Actions', () => { callback(new Error('error')) }) - store.dispatch(actions.setFeatureFlag()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.setFeatureFlag()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -1044,7 +1071,7 @@ describe('Actions', () => { getTransactionCountSpy.restore() }) - it('', () => { + it('calls getTransactionCount', () => { const store = mockStore() getTransactionCountSpy = sinon.spy(global.ethQuery, 'getTransactionCount') @@ -1054,7 +1081,7 @@ describe('Actions', () => { }) }) - it('', () => { + it('errors when getTransactionCount throws', async () => { const store = mockStore() const expectedActions = [ { type: 'DISPLAY_WARNING', value: 'error' }, @@ -1065,10 +1092,12 @@ describe('Actions', () => { callback(new Error('error')) }) - return store.dispatch(actions.updateNetworkNonce()) - .catch(() => { - assert.deepEqual(store.getActions(), expectedActions) - }) + try { + await store.dispatch(actions.updateNetworkNonce()) + assert.fail('Should have thrown error') + } catch (_) { + assert.deepEqual(store.getActions(), expectedActions) + } }) }) @@ -1120,7 +1149,7 @@ describe('Actions', () => { fetchMock.restore() }) - it('', () => { + it('calls expected actions', () => { const store = mockStore() setCurrentLocaleSpy = sinon.spy(background, 'setCurrentLocale') @@ -1138,7 +1167,7 @@ describe('Actions', () => { }) }) - it('', () => { + it('calls expected actions', () => { const store = mockStore() const expectedActions = [ { type: 'SHOW_LOADING_INDICATION', value: undefined }, @@ -1168,7 +1197,7 @@ describe('Actions', () => { markPasswordForgottenSpy.restore() }) - it('', () => { + it('calls markPasswordForgotten', () => { const store = mockStore() store.dispatch(actions.markPasswordForgotten()) assert(markPasswordForgottenSpy.calledOnce) @@ -1186,7 +1215,7 @@ describe('Actions', () => { unMarkPasswordForgottenSpy.restore() }) - it('', () => { + it('calls unMarkPasswordForgotten', () => { const store = mockStore() store.dispatch(actions.unMarkPasswordForgotten()) assert(unMarkPasswordForgottenSpy.calledOnce) diff --git a/test/unit/ui/app/selectors.spec.js b/test/unit/ui/app/selectors.spec.js index b4aa8e272..a190462b0 100644 --- a/test/unit/ui/app/selectors.spec.js +++ b/test/unit/ui/app/selectors.spec.js @@ -152,16 +152,10 @@ describe('Selectors', function () { assert.equal(selectedTokenToFiatRate, '0.21880988420033492152') }) - describe('#getSelectedTokenContract', () => { - - beforeEach(() => { - global.eth = new Eth(provider) - }) - - it('', () => { - const selectedTokenContract = selectors.getSelectedTokenContract(mockState) - assert(selectedTokenContract.abi) - }) + it('#getSelectedTokenContract', () => { + global.eth = new Eth(provider) + const selectedTokenContract = selectors.getSelectedTokenContract(mockState) + assert(selectedTokenContract.abi) }) it('#getCurrentViewContext', () => { diff --git a/ui/app/components/app/dropdowns/tests/dropdown.test.js b/ui/app/components/app/dropdowns/tests/dropdown.test.js index 2b026589a..cba4b30b7 100644 --- a/ui/app/components/app/dropdowns/tests/dropdown.test.js +++ b/ui/app/components/app/dropdowns/tests/dropdown.test.js @@ -4,7 +4,7 @@ import sinon from 'sinon' import { shallow } from 'enzyme' import { DropdownMenuItem } from '../components/dropdown.js' -describe('', () => { +describe('Dropdown', () => { let wrapper const onClickSpy = sinon.spy() const closeMenuSpy = sinon.spy() diff --git a/ui/app/store/actions.js b/ui/app/store/actions.js index cbf8e6283..2b0b54c60 100644 --- a/ui/app/store/actions.js +++ b/ui/app/store/actions.js @@ -1196,7 +1196,7 @@ function updateAndApproveTx (txData) { dispatch(actions.txError(err)) dispatch(actions.goHome()) log.error(err.message) - reject(err) + return reject(err) } resolve(txData) @@ -1679,7 +1679,7 @@ function addToken (address, symbol, decimals, image) { dispatch(actions.hideLoadingIndication()) if (err) { dispatch(actions.displayWarning(err.message)) - reject(err) + return reject(err) } dispatch(actions.updateTokens(tokens)) resolve(tokens) @@ -1696,7 +1696,7 @@ function removeToken (address) { dispatch(actions.hideLoadingIndication()) if (err) { dispatch(actions.displayWarning(err.message)) - reject(err) + return reject(err) } dispatch(actions.updateTokens(tokens)) resolve(tokens) @@ -1786,7 +1786,7 @@ function retryTransaction (txId, gasPrice) { background.retryTransaction(txId, gasPrice, (err, newState) => { if (err) { dispatch(actions.displayWarning(err.message)) - reject(err) + return reject(err) } const { selectedAddressTxList } = newState @@ -1809,7 +1809,7 @@ function createCancelTransaction (txId, customGasPrice) { background.createCancelTransaction(txId, customGasPrice, (err, newState) => { if (err) { dispatch(actions.displayWarning(err.message)) - reject(err) + return reject(err) } const { selectedAddressTxList } = newState @@ -1832,7 +1832,7 @@ function createSpeedUpTransaction (txId, customGasPrice) { background.createSpeedUpTransaction(txId, customGasPrice, (err, newState) => { if (err) { dispatch(actions.displayWarning(err.message)) - reject(err) + return reject(err) } const { selectedAddressTxList } = newState @@ -2185,7 +2185,7 @@ function setAccountLabel (account, label) { if (err) { dispatch(actions.displayWarning(err.message)) - reject(err) + return reject(err) } dispatch({ @@ -2786,7 +2786,9 @@ function setSeedPhraseBackedUp (seedPhraseBackupState) { dispatch(actions.displayWarning(err.message)) return reject(err) } - return forceUpdateMetamaskState(dispatch).then(() => resolve()) + return forceUpdateMetamaskState(dispatch) + .then(resolve) + .catch(reject) }) }) }