chore(jenkins): refactor jenkins-status using events

[mmarchini]

No description provided.

[phillipj]

Interesting alternative approach to verifying scopes that I haven't used before. Any obvious upsides compared to the previous approach?

[mmarchini]

Now that we're using EventEmitter and Promises, the timing is different and as a result the callback below to run before we made a call to the scope URL, so we get an error when we call scope.done()

[phillipj]

Ahh okey, so that's another approach to fixing what was my biggest headache in #258 basically.

What I ended up doing there, was to make sure all event handlers returned a Promise. That way the resulting Promise coming out of the app.emitJenkinsEvent()-equivalent of #258 would actually resolve after all handlers had completed. In the end we can be sure that all handlers (and thereby scope) has finished when the request gets the 200 response back.

No biggie tho, just wanted to understand the pros & reasons behind this new approach introduced.

[mmarchini]

That works too, but no reason to delay the response until app.emitJenkinsEvent finishes since Jenkins doesn't do anything with 200/500 status codes. We could for testability if we wanted to, but this approach seems fine and more robust?

[phillipj]

Assuming we don't need these commented lines anymore.

[phillipj]

Nice refactor 💯

[phillipj]

LGTM after either remove commented lines or keep them if event handlers are adjusted to ensure they have completed before responding to incoming HTTP request.

[phillipj]

I'm not opinionated about how it's done, as long as we can trust the test results I'm all in.

…

On Wed, 5 Aug 2020 at 20:58, mary marchini ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/integration/push-jenkins-update.test.js <#271 (comment)>: > .filteringPath(ignoreQueryParams) .post('/repos/nodejs/citgm/statuses/8a5fec2a6bade91e544a30314d7cf21f8a200de1') .reply(201) + .on('replied', (req, interceptor) => { + t.doesNotThrow(() => interceptor.scope.done()) + }) That works too, but no reason to delay the response until app.emitJenkinsEvent finishes since Jenkins doesn't do anything with 200/500 status codes. We could for testability if we wanted to, but this approach seems fine and more robust? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#271 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMWE3HUOIKPHX2BU4ZVADR7GTTXANCNFSM4PSYX7UQ> .