Peril always crashes when posting comments

[spinx]

Hey,

I've been trying to debug this one myself and getting nowhere. It's probably something really trivial 😬

The same happens for Peril hosted on Heroku and development version via ngrok. Deployed latest master@ff390dc65b4561634ffae56f10fa3a52265afdf3

Github app only has access to a few repos in the org. I tried granting all permissions on dev so it's not that.

Peril settings file:

{
  "$schema": "https://raw.githubusercontent.com/danger/peril/master/peril-settings-json.schema",
  "settings": {
    "ignored_repos": [],
    "disable_github_check": true
  },
  "rules": {},
  "repos": {
    "my-org/peril-settings": {
      "pull_request": "dangerfile.ts"
    },
    "my-org/snowplow": {
      "pull_request": "dangerfile.ts"
    },
    "my-org/peril-test-repo": {
      "pull_request": "dangerfile.ts"
    }
  }
}

dangerfile.ts:

import {message, danger} from "danger"
message("Works!")

Peril crashes out with this error after dangerfile is evaluated:

danger:executor` Posting to platform: { fails: [],
  markdowns: [],
  messages: [ { message: 'Works!', file: undefined, line: undefined } ],
  warnings: [] } +0ms
Found only messages, passing those to review.
  danger:GitHubAPI getPullRequestCommits:: Sending pull request commit request for first page +0ms
  danger:GitHubAPI getPullRequestCommits:: Request url generated "repos/my-org/peril-test-repo/pulls/4/comments" +1ms
  danger:GitHubAPI Sending:  https://api.github.com/repos/my-org/peril-test-repo/pulls/4/comments { 'Content-Type': 'application/json',
  Authorization: 'token v1.xxxxxxxxxxxxxxxxxxxxxxx',
  Accept: 'application/vnd.github.machine-man-preview+json' } +0ms
  danger:GitHubAPI getNextPageFromLinkHeader:: Given response does not contain link header for pagination +1s

TypeError: Cannot read property 'length' of undefined
    at Executor.<anonymous> (/Users/test/peril/api/node_modules/danger/distribution/runner/Executor.js:310:60)
    at step (/Users/test/peril/api/node_modules/danger/distribution/runner/Executor.js:32:23)
    at Object.next (/Users/test/peril/api/node_modules/danger/distribution/runner/Executor.js:13:53)
    at fulfilled (/Users/test/peril/api/node_modules/danger/distribution/runner/Executor.js:4:58)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Any idea what this could be ?

[orta]

Interesting, the only times the length is used in the Executor is on the different attributes of fails, markdowns, messages and warnings - so somehow that's being corrupted in there?

[spinx]

Going through the generated JS it actually fails on this: https://github.com/danger/danger-js/blob/7.0.19/source/runner/Executor.ts#L279

git is undefined at this point

locally danger posts to github just fine, but that's with my token not via github app

[jtreanor]

I believe I am seeing the same issue with a brand new Github app with very simple settings.

peril-setings.json:

{
  "rules": {
    "pull_request, pull_request.labeled, pull_request.unlabeled": "rules.ts"
  }
}

rules.ts:

import {warn} from "danger";
warn("Test warning");

When master (b69e580b34afb12f8b08340e1094879969fb4d25) is deployed to Heroku or when running locally, I'm getting this crash:

error: UnhandledRejection Error: Cannot read property 'length' of undefined {"stack":"TypeError: Cannot read property 'length' of undefined\n    at Executor.<anonymous> (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:309:60)\n    at step (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:32:23)\n    at Object.next (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:13:53)\n    at fulfilled (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:4:58)\n    at process._tickCallback (internal/process/next_tick.js:68:7)"}
/Users/james/src/peril/api/out/peril.js:24
        throw reason;
        ^

TypeError: Cannot read property 'length' of undefined
    at Executor.<anonymous> (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:309:60)
    at step (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:32:23)
    at Object.next (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:13:53)
    at fulfilled (/Users/james/src/peril/api/node_modules/danger/distribution/runner/Executor.js:4:58)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Using git bisect, I have confirmed that the breaking change was made when Danger was updated to 7.0.9 in 66b415e. The temporary solution is to use an old version, but I would really like to get to the bottom of this.

Is there any other info I can give to help with debugging? The Github app is set up exactly as described in the guide so this seems to be as stock as a setup could be unless I'm missing something.

Could this be related to the other issue I reported in https://github.com/danger/peril/issues/367#issuecomment-467363340? It was also introduced by the Danger update.

I'll keep digging.

[jtreanor]

This line was added to Executor.js in the Danger update (see https://github.com/danger/danger-js/compare/6.1.9...7.0.9#diff-bb13471af891e272bfdbcaee1d82a94dR269):

const commitID = git.commits[git.commits.length - 1].sha

It looks like this may be the line that it's crashing on. So git.commits appears to be undefined here. This matches what @spinx observed above.

I have tried adding console.log(danger.git.commits); to my rules file and it does give a list of commits as expected.

[nkete]

As @jtreanor pointed out it works with danger@6.1.9. Looking at the code I found this line and it looks like it could be related to 66b415e mentioned above (commenting.ts, danger_runner.ts)

[jtreanor]

The root cause of this crash is that Peril does not have a GitDSL object to pass to Danger, but handleResults requires it. I have opened a PR to make this parameter optional in Danger.js here: https://github.com/danger/danger-js/pull/887.

[orta]

Alright, Peril is now up to date with the changes in Danger JS in master ^

[jtreanor]

Great!

The PR to close this is here: https://github.com/danger/peril/pull/438

I have tested it and I'm not seeing the crash anymore 🎉