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

@@ -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)
-
-
     })
   })
 

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,

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(() => {
+      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(() => {
+      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(() => {
+
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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(() => {
+      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)

test/unit/ui/app/selectors.spec.js

@@ -152,17 +152,11 @@ describe('Selectors', function () {
     assert.equal(selectedTokenToFiatRate, '0.21880988420033492152')
   })
 
-  describe('#getSelectedTokenContract', () => {
-
-    beforeEach(() => {
+  it('#getSelectedTokenContract', () => {
     global.eth = new Eth(provider)
-    })
-
-    it('', () => {
     const selectedTokenContract = selectors.getSelectedTokenContract(mockState)
     assert(selectedTokenContract.abi)
   })
-  })
 
   it('#getCurrentViewContext', () => {
     const currentViewContext = selectors.getCurrentViewContext(mockState)

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()

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)
       })
     })
   }