Skip to content
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: close-pr count included unsuccessful runs #113

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ghinks
Copy link
Contributor

@ghinks ghinks commented Dec 29, 2021

Changed the result to look at whether the array actually had a length as on testing I found that it always said "1 PRs closed" when closing a failing PR.

During the writing of the documents I created a failing PR from the dependency and the wiby close-pr command was always saying that it was closing it when in fact it was not. The promise all was returning an empty array.

Changed the result to look at whether the array actually had a length as on testing I found that it always said "1 PRs closed" when closing a failing PR.
lib/closePR.js Outdated
@@ -16,7 +16,7 @@ module.exports = async ({ dependents }) => {
const branch = await context.getTestingBranchName(parentBranchName)
const resp = await github.getChecks(dependentPkgInfo.owner, dependentPkgInfo.name, branch)
const closedPR = await closeDependencyPR(dependentPkgInfo.owner, dependentPkgInfo.name, branch, resp.data.check_runs)
if (closedPR) {
if (closedPR && closedPR.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, is it ever not an array?

Suggested change
if (closedPR && closedPR.length > 0) {
if (closedPR.length > 0) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that is true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the tests are now failing?

I haven't checked if this is a mocked response...

@@ -16,7 +16,7 @@ module.exports = async ({ dependents }) => {
const branch = await context.getTestingBranchName(parentBranchName)
const resp = await github.getChecks(dependentPkgInfo.owner, dependentPkgInfo.name, branch)
const closedPR = await closeDependencyPR(dependentPkgInfo.owner, dependentPkgInfo.name, branch, resp.data.check_runs)
if (closedPR) {
if (closedPR && closedPR.length > 0) {
closedPrs.push(closedPR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it'd be better to always push it unconditionally, and make the "count" code smarter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smart counting is in the TODOs, this is just a tidy up from the first draft update of the readme. But its good to find things when documenting.

assume an array will always be returned
the array length needs to be checked as an array that is empty will be returned by the reduce and promise all combination
@ghinks
Copy link
Contributor Author

ghinks commented Jan 5, 2022

After having written a unit test to cover this scenario where the PRs raised by wiby fail I do believe that the check

if (closedPR && closedPR.length) {
        closedPrs.push(closedPR)
}

was the correct one as the return will always be an array but can possible be empty which means none of the PRs were closed.

@ljharb
Copy link
Member

ljharb commented Jan 5, 2022

closedPR?.length > 0?

@dominykas
Copy link
Member

So the closeDependencyPR function has two return points:

if (!checkRuns) {

and

return await Promise.all(prsToClose.map((pr) => github.closePR(owner, repo, pr.number)))

So yeah, it can be empty, or it can be an array. So we could do what Jordan suggests, or we could return an empty array in the first return point. I'm not too bothered with existing approach as well.

However, this does uncover yet another edge case, where I'm not sure the behavior is 100% correct - we do closedPrs.push(closedPR), however closedPr is already an array - should we not spread it (closedPrs.push(...closedPR))? Or even turn closedPrs into a set, to get a unique list of PRs, because purely theoretically, we could have multiple PRs with the same head (targetting different base branches)?

Change code per suggestions. Using ?
Optional chaining was available from node 14 onwards, we still support node 12 and so although I'm happy with the change we would have to stop supporting node 12.
@ghinks
Copy link
Contributor Author

ghinks commented Jan 15, 2022

I did try the optional chaining grammar as LJ suggested but we support node 12 and this feature was not available until node 14. I'm very happy to change the github actions CI to only test from node 14+ but I"m going to wait on feedback
Screen Shot 2022-01-15 at 11 32 06 AM
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants