René's URL Explorer Experiment

Title: Respond to incoming HTTPS requests when all work is done by phillipj · Pull Request #258 · nodejs/github-bot · GitHub

Open Graph Title: Respond to incoming HTTPS requests when all work is done by phillipj · Pull Request #258 · nodejs/github-bot

X Title: Respond to incoming HTTPS requests when all work is done by phillipj · Pull Request #258 · nodejs/github-bot

Description: TLDR; upgrading the package we use to talk to github.com causes integrations tests to break, and those tests are currently unreliable. The package we're using to talk to github.com uses a deprecated authentication mechanism that I regularly get emails about: https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/. While naively trying to upgrade that package, I stumbled opened up a can of worms that I want to fix properly first... What has changed? Make integration tests execute all external API request expectations Previously only a few of the external API request expectations setup with nock.js has been run (details explained in the long story below). How we run those expectations was introduced in the beginning of this project, a blooper I'm most likely to blame for, and copy-pasted later on. Make server respond to incoming HTTPS requests after all event listeners has finished executing Primarily to make our tests more reliable. For that to happen we need to be confident the code we're running tests on, has finished executing. Replace internal pub/sub mechanism from raw EventEmitter to something where we know when all listeners has finished executing Lots of the internals of the github-bot is setup as event listeners subscribing for events like pull_request.opened. Those events are in practise GitHub webhook events. For us to be able to know when all event listeners are done with their work, using pure EventEmitter does not cut it because our listeners perform asynchronous work. To mitigate that, events-async has been installed. It allows us to know when all (webhook) event listeners has executed as long as listeners returns a Promise. Refactoring our listeners from a callback/fire-n-forget type of code, to return a Promise, obviously involves quite a lot of changes. Those changes has been written to not change any behaviour, but rather only promisify them. The long story Up until now, a handful of integration tests has been flawed because we didn't actually verify all nock.js expectations (read: nock.js scope) had been fulfilled. That was due to a desire to run several methods as a one-liner instead of one per line; filesScope.done() && existingRepoLabelsScope.done() That could make sense at first glance, but there's a very important catch we didn't think about when writing it; it requires .done() to return a truthy value or else what comes after .done() will not be executed. The .done() method on nock.js' scope either throws an exception if the expectation it represents has not been met, or returns undefined when all is good. That undefined value stops the subsequent methods on the same line to be executed. In other words, we have always just checked the first expectation, and none of the subsequent ones on the same line. The changes introduced in this commit executes these .done()s on their own line which sadly causes all of the related tests to explode at the moment. Why most of these expectations haven't been met is probably due to timing issues, since we don't wait for all the related code to have finished executing before the tests are run. As of now, the code has been written in a fashion that allows incoming HTTPS requests to be get their response ASAP, whilst the code pushing PR statuses to github.com or trigger Jenkins builds are still running. That raises some challenges for our integration tests since they don't really know when the code is finished, meaning tests can run. Upcoming changes will fix that by ensuring incoming requests will get their response after all relevant code has succeeded or failed. That will introduce quite a big refactor job, but the end result will be a lot more robust tests that can be trusted. Sooo with that braindump out of the way, any immediate thoughts? I assume there's very few comfortable reviewing and approving these changes. Still would appreciate any kinds of feedback, good and bad, high level or detailed, I'm all ears! Most importantly is getting ack from someone active near the core collaborator community that can point my way if any awkwardness happens that I don't catch myself. I'm ready to jump and investigate when needed.

Open Graph Description: TLDR; upgrading the package we use to talk to github.com causes integrations tests to break, and those tests are currently unreliable. The package we're using to talk to github.com uses a depre...

X Description: TLDR; upgrading the package we use to talk to github.com causes integrations tests to break, and those tests are currently unreliable. The package we're using to talk to github.com uses a d...

Opengraph URL: https://github.com/nodejs/github-bot/pull/258

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width