Cleanup beforeunload handler after transaction is resolved (#7333)

* Cleanup beforeunload handler after transaction is resolved The notification window was updated to reject transactions upon close in #6340. A handler that rejects the transaction was added to `window.onbeforeunload`, and it was cleared in `actions.js` if it was confirmed or rejected. However, the `onbeforeunload` handler remained uncleared if the transaction was resolved in another window. This results in the transaction being rejected when the notification window closes, even long after the transaction is submitted and confirmed. This has been the cause of many problems with the Firefox e2e tests. Instead the `onbeforeunload` handler is cleared in the `componentWillUnmount` lifecycle function, alongside where it's set in the first place. This ensures that it's correctly unset regardless of how the transaction was resolved, and it better matches user expectations. * Fix indentation and remove redundant export The `run-all.sh` Bash script now uses consistent indentation, and is consistent about only re-exporting the Ganache arguments when they change. * Ensure transactions are completed before checking balance Various intermittent e2e test failures appear to be caused by React re-rendering the transaction list during the test, as the transaction goes from pending to confirmed. To avoid this race condition, the transaction is now explicitly looked for in the confirmed transaction list in each of the tests using this pattern. * Enable all e2e tests on Firefox The remaining tests that were disabled on Firefox now work correctly. Only a few timing adjustments were needed. * Update Firefox used in CI Firefox v70 is now used on CI instead of v68. This necessitated rewriting the function where the extension ID was obtained because the Firefox extensions page was redesigned.
MetaMask · Nov 4, 2019 · d4db12d · d4db12d
1 parent e78d11f
commit d4db12d
Show file tree

Hide file tree

Showing 12 changed files with 96 additions and 123 deletions.
diff --git a/.circleci/scripts/firefox-install b/.circleci/scripts/firefox-install
@@ -4,7 +4,7 @@ set -e
 set -u
 set -o pipefail
 
-FIREFOX_VERSION='68.0'
+FIREFOX_VERSION='70.0'
 FIREFOX_BINARY="firefox-${FIREFOX_VERSION}.tar.bz2"
 FIREFOX_BINARY_URL="https://ftp.mozilla.org/pub/firefox/releases/${FIREFOX_VERSION}/linux-x86_64/en-US/${FIREFOX_BINARY}"
 FIREFOX_PATH='/opt/firefox'

diff --git a/test/e2e/address-book.spec.js b/test/e2e/address-book.spec.js
@@ -210,13 +210,13 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 1)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 1
+      }, 10000)
 
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
-        await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
-      }
+      const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
     })
   })
 
@@ -251,13 +251,13 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 2)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 2
+      }, 10000)
 
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
-        await driver.wait(until.elementTextMatches(txValues, /-2\s*ETH/), 10000)
-      }
+      const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues, /-2\s*ETH/), 10000)
     })
   })
 })
diff --git a/test/e2e/from-import-ui.spec.js b/test/e2e/from-import-ui.spec.js
@@ -226,8 +226,10 @@ describe('Using MetaMask with an existing account', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 1)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 1
+      }, 10000)
 
       const txValues = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
       assert.equal(txValues.length, 1)

diff --git a/test/e2e/func.js b/test/e2e/func.js
@@ -91,7 +91,7 @@ async function getExtensionIdChrome (driver) {
 
 async function getExtensionIdFirefox (driver) {
   await driver.get('about:debugging#addons')
-  const extensionId = await driver.findElement(By.css('dd.addon-target-info-content:nth-child(6) > span:nth-child(1)')).getText()
+  const extensionId = await driver.wait(webdriver.until.elementLocated(By.xpath('//dl/div[contains(., \'Internal UUID\')]/dd')), 1000).getText()
   return extensionId
 }
 

diff --git a/test/e2e/metamask-responsive-ui.spec.js b/test/e2e/metamask-responsive-ui.spec.js
@@ -231,13 +231,13 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 1)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 1
+      }, 10000)
 
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
-        await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
-      }
+      const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
     })
   })
 })
diff --git a/test/e2e/metamask-ui.spec.js b/test/e2e/metamask-ui.spec.js
@@ -289,13 +289,13 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 1)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 1
+      }, 10000)
 
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
-        await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
-      }
+      const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
     })
   })
 
@@ -332,13 +332,13 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 2)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 2
+      }, 10000)
 
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
-        await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
-      }
+      const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
     })
   })
 
@@ -385,13 +385,13 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 3)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 3
+      }, 10000)
 
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
-        await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
-      }
+      const txValues = await findElement(driver, By.css('.transaction-list-item__amount--primary'))
+      await driver.wait(until.elementTextMatches(txValues, /-1\s*ETH/), 10000)
     })
   })
 
@@ -838,12 +838,10 @@ describe('MetaMask', function () {
     it('renders the correct ETH balance', async () => {
       const balance = await findElement(driver, By.css('.transaction-view-balance__primary-balance'))
       await delay(regularDelayMs)
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        await driver.wait(until.elementTextMatches(balance, /^87.*\s*ETH.*$/), 10000)
-        const tokenAmount = await balance.getText()
-        assert.ok(/^87.*\s*ETH.*$/.test(tokenAmount))
-        await delay(regularDelayMs)
-      }
+      await driver.wait(until.elementTextMatches(balance, /^87.*\s*ETH.*$/), 10000)
+      const tokenAmount = await balance.getText()
+      assert.ok(/^87.*\s*ETH.*$/.test(tokenAmount))
+      await delay(regularDelayMs)
     })
   })
 
@@ -1002,22 +1000,15 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 1)
-
-      const txValues = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
-      assert.equal(txValues.length, 1)
-
-      // test cancelled on firefox until https://github.com/mozilla/geckodriver/issues/906 is resolved,
-      // or possibly until we use latest version of firefox in the tests
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        await driver.wait(until.elementTextMatches(txValues[0], /-1\s*TST/), 10000)
-      }
-
       await driver.wait(async () => {
         const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
         return confirmedTxes.length === 1
       }, 10000)
+
+      const txValues = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
+      assert.equal(txValues.length, 1)
+      await driver.wait(until.elementTextMatches(txValues[0], /-1\s*TST/), 10000)
+
       const txStatuses = await findElements(driver, By.css('.transaction-list-item__action'))
       await driver.wait(until.elementTextMatches(txStatuses[0], /Sent\sToken/i), 10000)
     })
@@ -1104,7 +1095,6 @@ describe('MetaMask', function () {
         return confirmedTxes.length === 2
       }, 10000)
 
-      await delay(regularDelayMs)
       const txValues = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
       await driver.wait(until.elementTextMatches(txValues[0], /-1.5\s*TST/))
       const txStatuses = await findElements(driver, By.css('.transaction-list-item__action'))
@@ -1115,14 +1105,10 @@ describe('MetaMask', function () {
 
       const tokenListItems = await findElements(driver, By.css('.token-list-item'))
       await tokenListItems[0].click()
-      await delay(regularDelayMs)
+      await delay(1000)
 
-      // test cancelled on firefox until https://github.com/mozilla/geckodriver/issues/906 is resolved,
-      // or possibly until we use latest version of firefox in the tests
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        const tokenBalanceAmount = await findElements(driver, By.css('.transaction-view-balance__primary-balance'))
-        await driver.wait(until.elementTextMatches(tokenBalanceAmount[0], /7.500\s*TST/), 10000)
-      }
+      const tokenBalanceAmount = await findElements(driver, By.css('.transaction-view-balance__primary-balance'))
+      await driver.wait(until.elementTextMatches(tokenBalanceAmount[0], /7.500\s*TST/), 10000)
     })
   })
 
@@ -1141,9 +1127,6 @@ describe('MetaMask', function () {
       const transferTokens = await findElement(driver, By.xpath(`//button[contains(text(), 'Approve Tokens')]`))
       await transferTokens.click()
 
-      if (process.env.SELENIUM_BROWSER !== 'firefox') {
-        await closeAllWindowHandlesExcept(driver, [extension, dapp])
-      }
       await driver.switchTo().window(extension)
       await delay(regularDelayMs)
 
@@ -1232,10 +1215,6 @@ describe('MetaMask', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      if (process.env.SELENIUM_BROWSER === 'firefox') {
-        this.skip()
-      }
-
       await driver.wait(async () => {
         const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
         return confirmedTxes.length === 3
@@ -1249,12 +1228,6 @@ describe('MetaMask', function () {
   })
 
   describe('Tranfers a custom token from dapp when no gas value is specified', () => {
-    before(function () {
-      if (process.env.SELENIUM_BROWSER === 'firefox') {
-        this.skip()
-      }
-    })
-
     it('transfers an already created token, without specifying gas', async () => {
       const windowHandles = await driver.getAllWindowHandles()
       const extension = windowHandles[0]
@@ -1267,7 +1240,6 @@ describe('MetaMask', function () {
       const transferTokens = await findElement(driver, By.xpath(`//button[contains(text(), 'Transfer Tokens Without Gas')]`))
       await transferTokens.click()
 
-      await closeAllWindowHandlesExcept(driver, [extension, dapp])
       await driver.switchTo().window(extension)
       await delay(regularDelayMs)
 
@@ -1304,12 +1276,6 @@ describe('MetaMask', function () {
   })
 
   describe('Approves a custom token from dapp when no gas value is specified', () => {
-    before(function () {
-      if (process.env.SELENIUM_BROWSER === 'firefox') {
-        this.skip()
-      }
-    })
-
     it('approves an already created token', async () => {
       const windowHandles = await driver.getAllWindowHandles()
       const extension = windowHandles[0]
@@ -1323,7 +1289,6 @@ describe('MetaMask', function () {
       const transferTokens = await findElement(driver, By.xpath(`//button[contains(text(), 'Approve Tokens Without Gas')]`))
       await transferTokens.click()
 
-      await closeAllWindowHandlesExcept(driver, extension)
       await driver.switchTo().window(extension)
       await delay(regularDelayMs)
 
@@ -1346,7 +1311,7 @@ describe('MetaMask', function () {
     })
 
     it('submits the transaction', async function () {
-      await delay(regularDelayMs)
+      await delay(1000)
       const confirmButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Confirm')]`))
       await confirmButton.click()
       await delay(regularDelayMs)

diff --git a/test/e2e/run-all.sh b/test/e2e/run-all.sh
@@ -37,22 +37,20 @@ concurrently --kill-others \
   'yarn ganache:start' \
   'sleep 5 && mocha test/e2e/from-import-ui.spec'
 
-export GANACHE_ARGS="${BASE_GANACHE_ARGS} --deterministic --account=0x53CB0AB5226EEBF4D872113D98332C1555DC304443BEE1CF759D15798D3C55A9,25000000000000000000"
 concurrently --kill-others \
   --names 'ganache,e2e' \
   --prefix '[{time}][{name}]' \
   --success first \
   'npm run ganache:start' \
   'sleep 5 && mocha test/e2e/send-edit.spec'
 
-
-  concurrently --kill-others \
-    --names 'ganache,dapp,e2e' \
-    --prefix '[{time}][{name}]' \
-    --success first \
-    'yarn ganache:start' \
-    'yarn dapp' \
-    'sleep 5 && mocha test/e2e/ethereum-on.spec'
+concurrently --kill-others \
+  --names 'ganache,dapp,e2e' \
+  --prefix '[{time}][{name}]' \
+  --success first \
+  'yarn ganache:start' \
+  'yarn dapp' \
+  'sleep 5 && mocha test/e2e/ethereum-on.spec'
 
 export GANACHE_ARGS="${BASE_GANACHE_ARGS} --deterministic --account=0x250F458997A364988956409A164BA4E16F0F99F916ACDD73ADCD3A1DE30CF8D1,0  --account=0x53CB0AB5226EEBF4D872113D98332C1555DC304443BEE1CF759D15798D3C55A9,25000000000000000000"
 concurrently --kill-others \
@@ -73,12 +71,11 @@ concurrently --kill-others \
   'sleep 5 && mocha test/e2e/address-book.spec'
 
 export GANACHE_ARGS="${BASE_GANACHE_ARGS} --deterministic --account=0x53CB0AB5226EEBF4D872113D98332C1555DC304443BEE1CF759D15798D3C55A9,25000000000000000000"
-  concurrently --kill-others \
-    --names 'ganache,dapp,e2e' \
-    --prefix '[{time}][{name}]' \
-    --success first \
-    'node test/e2e/mock-3box/server.js' \
-    'yarn ganache:start' \
-    'yarn dapp' \
-    'sleep 5 && mocha test/e2e/threebox.spec'
-
+concurrently --kill-others \
+  --names 'ganache,dapp,e2e' \
+  --prefix '[{time}][{name}]' \
+  --success first \
+  'node test/e2e/mock-3box/server.js' \
+  'yarn ganache:start' \
+  'yarn dapp' \
+  'sleep 5 && mocha test/e2e/threebox.spec'
diff --git a/test/e2e/send-edit.spec.js b/test/e2e/send-edit.spec.js
@@ -218,8 +218,10 @@ describe('Using MetaMask with an existing account', function () {
     })
 
     it('finds the transaction in the transactions list', async function () {
-      const transactions = await findElements(driver, By.css('.transaction-list-item'))
-      assert.equal(transactions.length, 1)
+      await driver.wait(async () => {
+        const confirmedTxes = await findElements(driver, By.css('.transaction-list__completed-transactions .transaction-list-item'))
+        return confirmedTxes.length === 1
+      }, 10000)
 
       const txValues = await findElements(driver, By.css('.transaction-list-item__amount--primary'))
       assert.equal(txValues.length, 1)

diff --git a/ui/app/components/app/signature-request.js b/ui/app/components/app/signature-request.js
@@ -107,7 +107,7 @@ SignatureRequest.prototype.componentDidMount = function () {
   const { clearConfirmTransaction, cancel } = this.props
   const { metricsEvent } = this.context
   if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
-    window.onbeforeunload = event => {
+    this._onBeforeUnload = event => {
       metricsEvent({
         eventOpts: {
           category: 'Transactions',
@@ -118,6 +118,13 @@ SignatureRequest.prototype.componentDidMount = function () {
       clearConfirmTransaction()
       cancel(event)
     }
+    window.addEventListener('beforeunload', this._onBeforeUnload)
+  }
+}
+
+SignatureRequest.prototype.componentWillUnmount = function () {
+  if (getEnvironmentType(window.location.href) === ENVIRONMENT_TYPE_NOTIFICATION) {
+    window.removeEventListener('beforeunload', this._onBeforeUnload)
   }
 }