Testing webhooks and node core labels #60
Conversation
phillipj
force-pushed
the
webhooks-testing
branch
from
5832e66 to
2ddb8f5
Compare
phillipj
changed the title
| data.action = data.action ? event + '.' + data.action : event | ||
|
|
||
| if (!signature || signature !== sign(secret, req.raw)) { | ||
| if (!githubSecret.isValid(req)) { |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
@nodejs/github-bot I'm done with this for now, any feedback would be much appreciated ❤️ |
|
Had to read up on |
Yepp, which ends up breaking the tests |
|
Looks solid to me. Will |
|
Hm, yeah that would throw, but I'll double check to see if those exceptions are bubbled up correctly making the tests explode |
|
Found that nock returned error to the request callbacks, which didn't really make the tests explode as badly as I wanted. Just pushed a commit fixing that -- I'll probably make that |
Includes minor fixes in existing code due to breaking changes in the github dependency.
Previously we adding labels, we actually edited issues with an array of all the labels the issue should have. That meant we had to include both the pre existing labels, as well as the labels we resolved by the filepaths changed. Now with the upgrade of the github npm dependency, we got a new API for *adding new labels* to issues. That means we don't have to fetch pre existing labels and so forth, simplifying the process a whole lot.
These changes extracts the logic related to verifying the secret sent from github.com when webhooks are invoked. Mainly done to enable turning off the secret verification while running tests.
phillipj
force-pushed
the
webhooks-testing
branch
from
f285a16 to
e2bf71b
Compare
|
@Starefossen what's your preference on commits / squashing? I've tried to separate each set of changes and include a description where needed. Obviously if I squash 'em all it won't be as descriptive in the git log... |
|
Decided to merge this, as it kinda stalled progress on #55. Kept each commit as they're pretty useful on their own, will try to keep the next PRs more focused. |
|
Sorry I dropped the ball on this one @phillipj 😢 I often like to have individual commits in the timeline if they are logical units of work with well written messages instead of squashing them. |
No worries! Seems we agree on commits vs squashing tho :) |
This is a first shot at testing the code related to webhooks, and node core labelling specifically.
The gist of this is to POST a recorded payload against the bot's
/hooks/githubendpoint, mock responses from github.com, and verify that the bot tried to GET/POST a set of expected github.com API URLs.While working on this I noticed a couple of unexpected URLs being requested. Found a bug in the old github npm dependency we used, so decided to update that dependency as well.
Also moved all tests into
tests/{integration,unit}directories.Refs #55