Uh oh!
There was an error while loading. Please reload this page.
Testing webhooks and node core labels - #60
Conversation
5832e66 to
2ddb8f5Compare| 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.
Uh oh!
There was an error while loading. Please reload this page.
phillipj
commented
Aug 26, 2016
@nodejs/github-bot I'm done with this for now, any feedback would be much appreciated ❤️ |
Starefossen
commented
Aug 26, 2016
Had to read up on |
phillipj
commented
Aug 26, 2016
Yepp, which ends up breaking the tests |
Starefossen
commented
Aug 26, 2016
Looks solid to me. Will |
phillipj
commented
Aug 26, 2016
Hm, yeah that would throw, but I'll double check to see if those exceptions are bubbled up correctly making the tests explode |
phillipj
commented
Aug 26, 2016
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.
f285a16 to
e2bf71bComparephillipj
commented
Aug 31, 2016
@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... |
phillipj
commented
Sep 5, 2016
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. |
Starefossen
commented
Sep 5, 2016
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. |
phillipj
commented
Sep 5, 2016
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