Github: Update uses of Electron API calls that require Promises in Electron 7+ (callbacks are deprecated)

[DeeDeeG]

Summary

There are some Electron APIs that changed to only return Promises (and not run callbacks passed to them), mostly as of Electron 7+. Updating to be compatible with those would be great for updating Atom's Electron version.

See: https://www.electronjs.org/docs/api/modernization/promisification

Motivation

There is a Work In Progress pull request to update Atom to Electron 9, and I think updating these API calls may be necessary for the github package to continue to work with Electron 9. (https://github.com/atom/atom/pull/21777)

Describe alternatives you've considered

N/A

Additional context

Most (I think all) of these API calls generally _support_ Promises starting with Electron 6. (Some of them support Promises as of even earlier, in Electron 5.) So implementing these changes should still be compatible with all the current and in-development versions of Atom.

github's tests are failing over at Atom core's Electron 9 PR: https://github.com/atom/atom/pull/21777#issuecomment-773708566

Here are the error messages for the github package tests:

Error messages for the github package tests (click to expand):

  2441 passing (15m)
  1 pending
  4 failing

  1) GithubDotcomMarkdown
       opening issueish links
         opens item in pane and activates accordingly:
     Error: Trying to stub property 'open' of null
      at throwOnFalsyObject (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/throw-on-falsy-object.js:7:15)
      at Function.stub (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/stub.js:70:24)
      at Sandbox.stub (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/sandbox.js:331:37)
      at Context.<anonymous> (/Users/runner/work/1/s/node_modules/github/github-dotcom-markdown.test.js:143:13)
      at processImmediate (internal/timers.js:439:21)

  2) GithubDotcomMarkdown
       opening issueish links
         records event for opening issueish in pane item:
     Error: Trying to stub property 'open' of null
      at throwOnFalsyObject (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/throw-on-falsy-object.js:7:15)
      at Function.stub (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/stub.js:70:24)
      at Sandbox.stub (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/sandbox.js:331:37)
      at Context.<anonymous> (/Users/runner/work/1/s/node_modules/github/github-dotcom-markdown.test.js:167:13)
      at processImmediate (internal/timers.js:439:21)

  3) GithubDotcomMarkdown
       opening issueish links
         does not record event if opening issueish in pane item fails:
     Error: Trying to stub property 'open' of null
      at throwOnFalsyObject (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/throw-on-falsy-object.js:7:15)
      at Function.stub (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/stub.js:70:24)
      at Sandbox.stub (/Users/runner/work/1/s/node_modules/github/node_modules/sinon/lib/sinon/sandbox.js:331:37)
      at Context.<anonymous> (/Users/runner/work/1/s/node_modules/github/github-dotcom-markdown.test.js:179:13)
      at processImmediate (internal/timers.js:439:21)

  4) WorkerManager
       when the manager process is destroyed
         destroys all the renderer processes that were created:
     Error: Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/1/s/node_modules/github/test/worker-manager.test.js)

[DeeDeeG]

API calls that may need updating:

• contents.executeJavaScript(code[, userGesture, callback]) --> contents.executeJavaScript(code[, userGesture])
  
  
  
  • Used in a test file: test/worker-manager.test.js
  
  
• dialog.showOpenDialog([browserWindow, ]options[, callback]) --> dialog.showOpenDialog([browserWindow, ]options)
• dialog.showSaveDialog([browserWindow, ]options[, callback]) --> dialog.showSaveDialog([browserWindow, ]options)
• shell.openExternal(url[, options, callback]) --> shell.openExternal(url[, options])
  
  
  
  • This changed in Electron 5, so not actually related to the Electron 9 upgrade.
  
  
• Others???

[Aerijo]

Also see #2605. In general, passing tests isn't enough because the tests may be (now incorrectly) mocking the Electron interface.

[DeeDeeG]

I feel like these tests mock maybe _too much_ of the Electron API. Because I would have liked to see these tests fail earlier if the API usage is wrong. Better to do real operations and clean up after them, I think?

(I'm also biased, because I'm not great at rewriting the tests. I find rewriting the tests more involved if you have to update all the mocks to be similar to real Electron after a major version Electron upgrade.)

[Aerijo]

I feel like these tests mock maybe too much of the Electron API. Because I would have liked to see these tests fail earlier if the API usage is wrong. Better to do real operations and clean up after them, I think?

I agree in general, but don't know why they were written that way in the first place. Maybe the API can't be programmatically emulated, or caused flaky tests. @smashwilson might be able to comment on this.

[smashwilson]

We have to mock shell.openExternal so that the test suite doesn't actually open browser windows! When you're running the suite locally it would be disruptive; on CI it would either slow things down by starting a bunch of browser processes that would never close, or fail outright because they're running headless.

I agree that I'd have liked to get earlier notice that these functions had changed signatures. I think the best way to do _that_ would be using something like TypeScript types, though.

[DeeDeeG]

I think I found all of them. Updated my earlier comment: https://github.com/atom/github/issues/2624#issuecomment-776880041

[DeeDeeG]

I have a method to run this repo's tests against the "Electron 9 Atom" build artifacts from the Electron 9 PR. https://github.com/atom/github/compare/master...DeeDeeG:test-with-Atom-Electron-9

Feel free to open PRs at my fork against that branch to run the tests. Or @smashwilson if you would like to host a copy of that branch here it might make testing fixes easier. (Although I could optionally clean up the commit history of that branch quite a bit first.)

[asturur]

i made this https://github.com/atom/github/pull/2626

[smashwilson]

2625 and #2626 have been merged and published in [email protected].

[smashwilson]

Do we have confirmation that we caught them all in an Electron 9 build so we can close this issue? 😄

[asturur]

i don't. I made a manual search and seems like that. But i do not feel like give it for granted.

[DeeDeeG]

I also manually searched the github package code for all the things listed in https://www.electronjs.org/docs/api/modernization/promisification.

I am confident we got all of those. So I do think this issue can be closed.

(I was hoping this would lead to the github tests passing with the Electron 9 Atom build. But they're still failing for some reason. I'm not sure if any of the test outcomes changed? (I believe they are the same as before all the Promisification changes in github.) Kind of mysterious. I made a CI run here: CI run. I'm glad these API calls are now being done correctly for future Electron versions. The failing tests are still stumping me though.)

[asturur]

Yes test continue to fail on windows ( inconsistently ) and on MacOs are a bummer.

[DeeDeeG]

For what it's worth: In order to get some direction to pursue here, I am planning on getting my Electron 7 branch (with minimal diff from Atom master) running again, and now that these API changes are Electron 7 compatible, I can test if this package's tests pass with Electron 7.

Update: This repo's tests appear to pass with Electron 7: https://github.com/DeeDeeG/github/actions/runs/582856544

Update 2: Also passing in Atom's CI with Electron 7: https://dev.azure.com/DeeDeeG/b/_build/results?buildId=1103&view=logs&j=6de74252-975b-522d-7a3d-227ca79ca6c1&t=a70a2b1c-2767-57d7-6509-edd8e9181730&l=58

Now to find out if it's because of the "Electron 7" part or the "minimal diff from master branch" part, to see which sets my build of Atom apart from the one from the Electron 9 PR.

[DeeDeeG]

This documentation page seems to be better-maintained (than the modernization/Promisification.md one): https://www.electronjs.org/docs/breaking-changes

Update: I went through all of the changes for Electron 7 (https://www.electronjs.org/docs/breaking-changes#planned-breaking-api-changes-70) and github and Atom seem to be ready for those.

[DeeDeeG]

Some of these test failures begin with Electron 8, apparently: https://github.com/DeeDeeG/github/actions/runs/594334321

[asturur]

maybe we can do the same process with the electron 8 breaking changes IF the changes are already supported by electron 6. Otherwise the upgrade to electron 9 is not possible and we need to move from 6 to 7 first.

[DeeDeeG]

This can definitely be closed thanks to https://github.com/atom/github/pull/2631. Thanks @sadick254!

🎉