Fix disabled ledger / payments panel tests

[bbondy]

#6890

/cc @willy-b @bsclifton @mrose17

Ledger Payments advanced panel tests are failing consistently so I had to disable them. Please cleanup as soon as possible. I would have reverted instead but there were too many dependent commits after it.

Ledger tests are failing consistently so I reverted just the tests for now.

I also noticed some problems with the tests so the changesets probably shoudln't have landed.

Some notes that I noticed too about these tests:

• .pause(1000) pauses shouldn't be used at all, instead waitFor some event. If you need to add a new custom waitFor you can add it in test/lib/brave.js
• Some things are being attached to context and then used in later it() calls, each test should be able to be run independently though so some of that logic should go in a before call instead. E.g. context.recoveryFilePathname. You should be able to do npm run test -- --grep="shows an error popover if one recovery key is missing"
• All tests should pass locally and on travis consistently.

Please block landing any further ledger work on fixing this in case we need to revert a bunch of things.

These should be reverted once the tests work:

• 26b4b48
• 15e5628
• 897cebb

Here's from one recent run:

  7) Payments Panel -> Advanced Panel shows an error popover if one recovery key is missing:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  8) Payments Panel -> Advanced Panel shows an error popover if a recovery key is not a UUID:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  9) Payments Panel -> Advanced Panel shows an error popover if both recovery keys are missing:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  10) Payments Panel -> Advanced Panel shows an error popover if the file is empty:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  11) Payments Panel can setup payments "before each" hook for "shows welcome page":
     WaitUntilTimeoutError: Promise was rejected with the following reason: timeout
      at getUrl() - brave.js:212:40

Here's from another:

  4) Payments Panel -> Advanced Panel shows an error popover if one recovery key is missing:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  5) Payments Panel -> Advanced Panel shows an error popover if a recovery key is not a UUID:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  6) Payments Panel -> Advanced Panel shows an error popover if both recovery keys are missing:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  7) Payments Panel -> Advanced Panel shows an error popover if the file is empty:
     Error: timeout of 30000ms exceeded. Ensure the done() callback is being called in this test.
  
  8) Payments Panel can setup payments "before each" hook for "shows welcome page":
     WaitUntilTimeoutError: Promise was rejected with the following reason: timeout
      at getUrl() - brave.js:212:40
  9) Payments Panel can setup payments "after each" hook for "shows welcome page":
     RuntimeError: chrome not reachable
      at window() - application.js:237:19

[bsclifton]

Fixed 897cebb with #6916

[willy-b]

The Advanced Panel tests in test/components/ledgerPanelAdvancedPanelTest.js are failing because ipcRenderer.sendSync now causes a crash in recent builds. Have there been any Muon changes affecting this API?

More detail:
The full call is devTools('electron').ipcRenderer.sendSync in test/lib/brave.js. This API is needed by the translations test client command defined there.
The Advanced Panel tests use the translations command to localize the expected wallet recovery file text.

[bbondy]

no I don't think so, do you know why more specifically they are failing ? Which type of sendSync?

[willy-b]

sure!
more details (hopefully not too much):

the test client translations command calls ipcRenderer.sendSync('translations').

translations command is defined at

browser-laptop/test/lib/brave.js

Line 728 in 1faf82a

this.app.client.addCommand('translations', function () {

:

    // retrieve a map of all the translations per existing IPC message 'translations'
    this.app.client.addCommand('translations', function () {
      return this.ipcSendRendererSync('translations')
    })

which calls ipcSendRendererSync, defined at

browser-laptop/test/lib/brave.js

Line 176 in 1faf82a

this.app.client.addCommand('ipcSendRendererSync', function (message, ...param) {

:

    this.app.client.addCommand('ipcSendRendererSync', function (message, ...param) {
      return this.execute(function (message, ...param) {
        return devTools('electron').ipcRenderer.sendSync(message, ...param)
      }, message, ...param)
    })

where the argument passed was 'translations', so the final call is ipcRenderer.sendSync('translations') (at

browser-laptop/test/lib/brave.js

Line 178 in 1faf82a

return devTools('electron').ipcRenderer.sendSync(message, ...param)

).

--
Note: I added these lines in 7bffa13
and @bsclifton verified the tests were passing at that time in #5809 (review) .

[bsclifton]

The tests have been working for me, they just seem intermittent. I'm looking at the regular (not advanced) tests right now and haven't been able to get them to fail locally.

[willy-b]

@bsclifton, this issue is for advanced tests, I think ("Ledger Payments advanced panel tests are failing consistently so I had to disable them" -@bbondy at top, also his examples).

Could you check whether those are failing for you with full browser crash upon retrieving translations (via sendSync) as described in last comment? (one has to re-enable advanced tests by removing describe.skip to see.)

[bsclifton]

Fixed 15e5628 with #6932

@willy-b both regular panel tests and advanced were failing for me regularly when I was maintaining the 0.13.1 branch. With advanced however, the problem seemed to be the first step timing out (which saves the file. If that fails, all of the other tests fail too (since they rely on that being done).

I had already started to look into a fix (have it stashed somewhere) which will redo the setup if needed

[willy-b]

ok @bsclifton , let me make sure I understand:

1. you just disabled the "regular" payments panel tests with the referenced commit (these are tests defined in test/components/ledgerPanelTest.js)
2. you have not tested the advanced ledger panel tests (these are the tests for advanced ledger settings defined in test/components/ledgerPanelAdvancedPanelTest.js) on latest master, and have not observed the ipcRenderer.sendSync crash
3. but you may have a fix for the advanced ledger panel tests (which does not have to do with ipcRenderer.sendSync issue I mentioned)

is that right?

[bsclifton]

@willy-b no, I apologize. I think this issue covering multiple items has made things confusing.

This issue was created by @bbondy because:

1. Preferences screen had a test failure
2. Regular payment panel tests had a failure
3. Advanced payment panel test had a failure

As I maintained the 0.13.1-branch, including merging your changes, I would see #2 and #3 intermittently fail. In this issue at the top, @bbondy marked all 3 tests as skip (so the tests are no longer running). #1 was broke because of a merge. I've fixed #1 and #2, all that's left is advanced payment panel tests

When I looked into #3 (advanced payment panel tests) before, I noticed it would sometimes crash or hang during the first step (which would save the keys to a file). This would then cause subsequent tests to fail. Perhaps this is the IPC issue you're talking about (I hadn't looked into it that much).

Basically, I'm just trying to share that everything except the ones you were talking about are fixed now 😄 Did you want to look into it more or would you mind if I jumped in too?

[willy-b]

ah, totally clear now.

go for it! I wasn't taking the issue, just sharing observations.

do let me know if you see the new crash for Advanced Panel tests too (#6911 (comment))

[luixxiul]

I added the release/not-blocking label according to @bbondy

[willy-b]

the crash I mentioned earlier has been resolved since I last checked, sweet.

opened a PR (#7401) with a couple of other changes needed to fix these tests:

• removed the describe.skip
• update selectors for changes in UI
• rerun setup before each test rather than trying to use a single session for all the tests

[NejcZdovc]

I guess I will take over this issue since I did big refactoring of payments tab. Will address it after all outstanding PR's related to this tab will be merged.

[bsclifton]

@NejcZdovc I can help you look at this 😄 I had been looking at it but didn't make any progress... I think part of the failure is because saving the keys has the same call to printToPDF which was failing (ex: with viewing contribution statement)

[willy-b]

what printToPDF call, @bsclifton? the recovery keys are saved to a text file (.txt not PDF) or printed using webContents.print depending on user preference.

fs.writeFile(filePath, message, (err) => {
    if (err) {
      console.log(err)
    } else {
      Tabs.create({url: fileUrl(filePath)}, (webContents) => {
        if (action.backupAction === 'print') {
          webContents.print({silent: false, printBackground: false})
        } else {
          webContents.downloadURL(fileUrl(filePath))
        }
      })
    }
  })

(Source:

browser-laptop/app/ledger.js

Line 317 in 2aeb514

fs.writeFile(filePath, message, (err) => {

)

[willy-b]

and I don't think there are any tests yet around printing the recovery keys, only saving them to a .TXT file, so there were no test failures from that.

[bsclifton]

@willy-b ah ok, my bad! too many issues, not enough time 😄

edit: I believe the save to file does a printToPDF in Muon

[willy-b]

RE: your comment edit @bsclifton: "I believe the save to file does a printToPDF in Muon"
it generates a .txt file in these tests, there's no PDF file involved.

[bsclifton]

@willy-b correct again 😄