-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: resolve implementation for multiple contributors #439
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
TODO:
|
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #439 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 16 -1
Lines 485 451 -34
Branches 57 53 -4
=========================================
- Hits 485 451 -34
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
test("Happy path, add correct new multiple contributors", async () => { | ||
const mock = nock("https://api.github.com") | ||
.get( | ||
`/repos/all-contributors/all-contributors-bot/git/ref/heads%2Fall-contributors%2Fadd-gr2m` | ||
) | ||
.reply(404) | ||
|
||
.get( | ||
`/repos/all-contributors/all-contributors-bot/git/ref/heads%2Fall-contributors%2Fadd-tenshiamd` | ||
) | ||
.reply(404) | ||
|
||
.get( | ||
"/repos/all-contributors/all-contributors-bot/contents/.all-contributorsrc?ref=master" | ||
) | ||
.reply(200, reposGetContentsAllContributorsRCdata) | ||
|
||
.get("/users/gr2m") | ||
.reply(200, usersGetByUsernameGr2mData) | ||
|
||
.get("/users/tenshiamd") | ||
.reply(200, usersGetByUsernameTenshiamdData) | ||
|
||
.get( | ||
"/repos/all-contributors/all-contributors-bot/contents/README.md?ref=master" | ||
) | ||
.reply(200, reposGetContentsREADMEMDdata) | ||
|
||
.get( | ||
`/repos/all-contributors/all-contributors-bot/git/ref/heads%2Fmaster` | ||
) | ||
.reply(200, gitGetRefdata) | ||
|
||
.post(`/repos/all-contributors/all-contributors-bot/git/refs`, (body) => { | ||
expect(body).toMatchSnapshot("request body"); | ||
return true; | ||
}) | ||
.reply(201, gitCreateRefdata) | ||
|
||
.put( | ||
`/repos/all-contributors/all-contributors-bot/contents/.all-contributorsrc`, | ||
(body) => { | ||
expect(body).toMatchSnapshot("request body"); | ||
return true; | ||
} | ||
) | ||
.reply(200, reposUpdateFiledata) | ||
|
||
.put( | ||
`/repos/all-contributors/all-contributors-bot/contents/README.md`, | ||
(body) => { | ||
expect(body).toMatchSnapshot("request body"); | ||
return true; | ||
} | ||
) | ||
.reply(200, reposUpdateFiledata) | ||
|
||
.post(`/repos/all-contributors/all-contributors-bot/pulls`, (body) => { | ||
expect(body).toMatchSnapshot("request body"); | ||
return true; | ||
}) | ||
.reply(201, pullsCreatedata) | ||
|
||
.post( | ||
"/repos/all-contributors/all-contributors-bot/issues/1/comments", | ||
(body) => { | ||
expect(body).toMatchSnapshot("request body"); | ||
return true; | ||
} | ||
) | ||
.reply(200); | ||
|
||
await probot.receive({ | ||
name: "issue_comment", | ||
id: "1", | ||
payload: issueCommentCreatedMultipleContributorsPayload, | ||
}); | ||
|
||
expect(mock.activeMocks()).toStrictEqual([]); | ||
expect(output).toMatchSnapshot("logs"); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gr2m @Berkmann18 please check this one as I cannot find any solution for this one to make it somehow pass. I need your advice regarding this matter. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can probably look into it next week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can probably look into it next week
@gr2m please do check thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you see a "Nock: No match for request" error, a good way to debug it is to run the test with nock debugging enabled
DEBUG=nock* npx jest --testNamePattern "Happy path, add correct new multiple contributors" test/integration/issue_comment.test.js
It seems that the app attempts to send these requests twice:
GET /repos/all-contributors/all-contributors-bot/contents/.all-contributorsrc?ref=master
GET /repos/all-contributors/all-contributors-bot/contents/README.md?ref=master
GET /repos/all-contributors/all-contributors-bot/git/ref/heads%2Fmaster
nock supports mock the same requests twice
.get(
"/repos/all-contributors/all-contributors-bot/contents/.all-contributorsrc?ref=master"
)
.twice()
.reply(200, reposGetContentsAllContributorsRCdata)
however this looks like a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gr2m I already tried DEBUG=nock*
and did have some other details but still not helpful 😞
however this looks like a bug?
Looks like it is. I also tried updating nock
to the latest version just now but it still does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean it looks like a bug with the code, not with nock. Why is the code sending these requests twice? Does it send them once per contributor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it send them once per contributor?
@gr2m all tests passed now after adding .twice()
. yes, currently it is one PR per contributor. Making one PR for many contributors is still on our TODO list for improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gr2m looks like all is good now. anything else we missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀:shipit:
🎉 This PR is included in version 1.18.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What:
Why:
How:
Checklist:
Bot Usage