Don't throw on request error, improve api consistency

source/api/fetch.ts

@@ -55,9 +55,8 @@ export function api(url: string | any, init: any): Promise<any> {
       const responseJSON = await response.json()
       console.error(`Request failed [${response.status}]: ${response.url}`)
       console.error(`Response: ${JSON.stringify(responseJSON, null, "  ")}`)
-      const msg = response.status === 0 ? "Network Error" : response.statusText
-      throw new (Error as any)(response.status, msg, {response: response})
     }
+
     return response
   })
 }

source/ci_source/ci_source_helpers.ts

@@ -46,7 +46,7 @@ export async function getPullRequestIDForBranch(source: CISource, env: Env, bran
   if (!token) {
     return 0
   }
-  const api = new GitHubAPI(token, source)
+  const api = new GitHubAPI(source, token)
   const prs = await api.getPullRequests()
   const prForBranch: GitHubPRDSL = prs.find((pr: GitHubPRDSL) => pr.head.ref === branch)
   if (prForBranch) {

source/commands/danger-pr.ts

@@ -37,7 +37,7 @@ if (program.args.length === 0) {
     if (validateDangerfileExists(dangerFile)) {
       d(`executing dangerfile at ${dangerFile}`)
       const source = new FakeCI({ DANGER_TEST_REPO: pr.repo, DANGER_TEST_PR: pr.pullRequestNumber })
-      const api = new GitHubAPI(process.env["DANGER_GITHUB_API_TOKEN"], source)
+      const api = new GitHubAPI(source, process.env["DANGER_GITHUB_API_TOKEN"])
       const platform = new GitHub(api)
       runDanger(source, platform, dangerFile)
     }

source/platforms/GitHub.ts

@@ -34,11 +34,8 @@ export class GitHub {
    * @returns {Promise<GitDSL>} the git DSL
    */
   async getReviewDiff(): Promise<GitDSL> {
-    const diffReq = await this.api.getPullRequestDiff()
-    const getCommitsResponse = await this.api.getPullRequestCommits()
-    const getCommits = await getCommitsResponse.json()
-
-    const diff = await diffReq.text()
+    const diff = await this.api.getPullRequestDiff()
+    const getCommits = await this.api.getPullRequestCommits()
 
     const fileDiffs: Array<any> = parseDiff(diff)
 
@@ -89,14 +86,14 @@ export class GitHub {
     const pr = await this.getReviewInfo()
     const commits = await this.api.getPullRequestCommits()
     const reviews = await this.api.getReviews()
-    const requestedReviewers = await this.api.getReviewerRequests()
+    const requested_reviewers = await this.api.getReviewerRequests()
 
     return {
       issue,
       pr,
       commits,
       reviews,
-      requested_reviewers: requestedReviewers
+      requested_reviewers
     }
   }
 
@@ -138,7 +135,10 @@ export class GitHub {
    */
   async deleteMainComment(): Promise<boolean> {
     const commentID = await this.api.getDangerCommentID()
-    if (commentID) { await this.api.deleteCommentWithID(commentID) }
+    if (commentID) {
+      await this.api.deleteCommentWithID(commentID)
+    }
+
     return commentID !== null
   }
 

source/platforms/_tests/GitHub.test.ts

@@ -14,7 +14,7 @@ const EOL = os.EOL
 const mockGitHubWithGetForPath = (expectedPath): GitHub => {
   const mockSource = new FakeCI({})
 
-  const api = new GitHubAPI("token", mockSource)
+  const api = new GitHubAPI(mockSource)
   const github = new GitHub(api)
 
   api.get = (path: string, headers: any = {}, body: any = {}, method: string = "GET"): Promise<any> => {
@@ -50,7 +50,7 @@ export const requestWithFixturedContent = async (path: string): Promise<any> =>
 describe("with fixtured data", () => {
   it("returns the correct github data", async () => {
     const mockSource = new FakeCI({})
-    const api = new GitHubAPI("token", mockSource)
+    const api = new GitHubAPI(mockSource)
     const github = new GitHub(api)
     api.getPullRequestInfo = await requestWithFixturedJSON("github_pr.json")
     api.getPullRequestCommits = await requestWithFixturedJSON("github_commits.json")
@@ -62,11 +62,13 @@ describe("with fixtured data", () => {
   describe("the dangerfile gitDSL", async () => {
     let github: GitHub = {} as any
     beforeEach(async () => {
-      const api = new GitHubAPI("token", new FakeCI({}))
+      const api = new GitHubAPI(new FakeCI({}))
       github = new GitHub(api)
 
-      api.getPullRequestDiff = await requestWithFixturedContent("github_diff.diff")
-      api.getPullRequestCommits = await requestWithFixturedJSON("github_commits.json")
+      const res = await requestWithFixturedContent("github_diff.diff")
+      api.getPullRequestDiff = res().text
+      const jsonFixtures = await requestWithFixturedJSON("github_commits.json")
+      api.getPullRequestCommits = jsonFixtures().json
     })
 
     it("sets the modified/created/deleted", async () => {

source/platforms/_tests/fixtures/static_file.json

@@ -0,0 +1,3 @@
+{
+  "content": "VGhlIEFsbC1EZWZlY3RvciBpcyBhIHB1cnBvcnRlZCBnbGl0Y2ggaW4gdGhlIERpbGVtbWEgUHJpc29uIHRoYXQgYXBwZWFycyB0byBwcmlzb25lcnMgYXMgdGhlbXNlbHZlcy4gVGhpcyBnb2dvbCBhbHdheXMgZGVmZWN0cywgaGVuY2UgdGhlIG5hbWUu"
+}

source/platforms/github/GitHubAPI.ts

@@ -15,7 +15,7 @@ export class GitHubAPI {
   fetch: typeof fetch
   additionalHeaders: any
 
-  constructor(public readonly token: APIToken | undefined, public readonly ciSource: CISource) {
+  constructor(public readonly ciSource: CISource, public readonly token?: APIToken) {
     // This allows Peril to DI in a new Fetch function
     // which can handle unique API edge-cases around integrations
     this.fetch = fetch
@@ -33,13 +33,11 @@ export class GitHubAPI {
   async fileContents(path: string, ref?: string): Promise<string> {
     // Use head of PR (current state of PR) if no ref passed
     if (!ref) {
-      const pr = await this.getPullRequestInfo()
-      const prJSON = await pr.json()
+      const prJSON = await this.getPullRequestInfo()
 
       ref = prJSON.head.ref
     }
-    const fileMetadata = await this.getFileContents(path, ref)
-    const data = await fileMetadata.json()
+    const data = await this.getFileContents(path, ref)
     const buffer = new Buffer(data.content, "base64")
     return buffer.toString()
   }
@@ -48,81 +46,95 @@ export class GitHubAPI {
 
   async getDangerCommentID(): Promise<number | null> {
     const userID = await this.getUserID()
-    const allCommentsResponse = await this.getPullRequestComments()
-    const allComments: any[] = await allCommentsResponse.json()
+    const allComments: any[] = await this.getPullRequestComments()
     const dangerComment = find(allComments, (comment: any) => comment.user.id === userID)
     return dangerComment ? dangerComment.id : null
   }
 
   async updateCommentWithID(id: number, comment: string): Promise<any> {
     const repo = this.ciSource.repoSlug
-    return this.patch(`repos/${repo}/issues/comments/${id}`, {}, {
+    const res = await this.patch(`repos/${repo}/issues/comments/${id}`, {}, {
       body: comment
     })
+
+    return res.json()
   }
 
   async deleteCommentWithID(id: number): Promise<any> {
     const repo = this.ciSource.repoSlug
-    return this.api(`repos/${repo}/issues/comments/${id}`, {}, {}, "DELETE")
+    const res = await this.api(`repos/${repo}/issues/comments/${id}`, {}, {}, "DELETE")
+
+    return res.json()
   }
 
   async getUserID(): Promise<number> {
     const info = await this.getUserInfo()
     return info.id
   }
 
-  postPRComment(comment: string): Promise<any> {
+  async postPRComment(comment: string): Promise<any> {
     const repo = this.ciSource.repoSlug
     const prID = this.ciSource.pullRequestID
-    return this.post(`repos/${repo}/issues/${prID}/comments`, {}, {
+    const res = await this.post(`repos/${repo}/issues/${prID}/comments`, {}, {
       body: comment
     })
+
+    return res.json()
   }
 
-  getPullRequestInfo(): Promise<any> {
+  async getPullRequestInfo(): Promise<any> {
     const repo = this.ciSource.repoSlug
     const prID = this.ciSource.pullRequestID
-    return this.get(`repos/${repo}/pulls/${prID}`)
+    const res = await this.get(`repos/${repo}/pulls/${prID}`)
+
+    return res.ok ?  res.json() : {}
   }
 
-  getPullRequestCommits(): Promise<any> {
+  async getPullRequestCommits(): Promise<any> {
     const repo = this.ciSource.repoSlug
     const prID = this.ciSource.pullRequestID
-    return this.get(`repos/${repo}/pulls/${prID}/commits`)
+    const res = await this.get(`repos/${repo}/pulls/${prID}/commits`)
+
+    return res.ok ? res.json() : []
   }
 
   async getUserInfo(): Promise<any> {
     const response: any = await this.get("user")
-    return await response.json()
+
+    return response.json()
   }
 
   // TODO: This does not handle pagination
-  getPullRequestComments(): Promise<any> {
+  async getPullRequestComments(): Promise<any> {
     const repo = this.ciSource.repoSlug
     const prID = this.ciSource.pullRequestID
-    return this.get(`repos/${repo}/issues/${prID}/comments`)
+    const res = await this.get(`repos/${repo}/issues/${prID}/comments`)
+
+    return res.ok ? res.json() : []
   }
 
-  getPullRequestDiff(): Promise<any> {
+  async getPullRequestDiff(): Promise<string> {
     const repo = this.ciSource.repoSlug
     const prID = this.ciSource.pullRequestID
-    return this.get(`repos/${repo}/pulls/${prID}`, {
+    const res = await this.get(`repos/${repo}/pulls/${prID}`, {
       accept: "application/vnd.github.v3.diff"
     })
+
+    return res.ok ? res.text() : ""
   }
 
-  getFileContents(path: string, ref?: string): Promise<any> {
+  async getFileContents(path: string, ref?: string): Promise<any> {
     const repo = this.ciSource.repoSlug
-    return this.get(`repos/${repo}/contents/${path}?ref=${ref}`, {})
+    const res = await this.get(`repos/${repo}/contents/${path}?ref=${ref}`)
+
+    return res.ok ? res.json() : { content: "" }
   }
 
   async getPullRequests(): Promise<any> {
     const repo = this.ciSource.repoSlug
     const res = await this.get(`repos/${repo}/pulls`)
-    if (res.ok) {
-      return res.json()
-    }
-    return []
+
+    return res.ok ? res.json : []
   }
 
   async getReviewerRequests(): Promise<any> {
@@ -131,10 +143,8 @@ export class GitHubAPI {
     const res = await this.get(`repos/${repo}/pulls/${prID}/requested_reviewers`, {
       accept: "application/vnd.github.black-cat-preview+json"
     })
-    if (res.ok) {
-      return res.json()
-    }
-    return []
+
+    return res.ok ? res.json() : []
   }
 
   async getReviews(): Promise<any> {
@@ -143,26 +153,25 @@ export class GitHubAPI {
     const res = await this.get(`repos/${repo}/pulls/${prID}/reviews`, {
       accept: "application/vnd.github.black-cat-preview+json"
     })
-    if (res.ok) {
-      return res.json()
-    }
-    return []
+
+    return res.ok ? res.json() : []
   }
 
   async getIssue(): Promise<any> {
     const repo = this.ciSource.repoSlug
     const prID = this.ciSource.pullRequestID
     const res = await this.get(`repos/${repo}/issues/${prID}`)
-    if (res.ok) {
-      return res.json()
-    }
-    return {}
+
+    return res.ok ? res.json() : {}
   }
 
+  // API Helpers
+
   private api(path: string, headers: any = {}, body: any = {}, method: string) {
-    if (this.token !== undefined) {
+    if (this.token) {
       headers["Authorization"] = `token ${this.token}`
     }
+
     const baseUrl = process.env["DANGER_GITHUB_API_BASE_URL"] || "https://api.github.com"
     return this.fetch(`${baseUrl}/${path}`, {
       method: method,

source/platforms/github/_tests/_GitHubAPI.test.ts

@@ -18,13 +18,14 @@ const fetch = (api, params): Promise<any> => {
   })
 }
 
-it.skip("fileContents expects to grab PR JSON and pull out a file API call", async () => {
+it("fileContents expects to grab PR JSON and pull out a file API call", async () => {
   const mockSource = new FakeCI({})
-  const api = new GitHubAPI("token", mockSource)
-  // api.fetch = fetch
+  const api = new GitHubAPI(mockSource, "token")
 
-  api.getPullRequestInfo = await requestWithFixturedJSON("github_pr.json")
-  api.getFileContents = await requestWithFixturedContent("static_file.md")
+  const requestFixture = await requestWithFixturedJSON("github_pr.json")
+  api.getPullRequestInfo = requestFixture().json
+  const fileContentFixture = await requestWithFixturedJSON("static_file.json")
+  api.getFileContents = fileContentFixture().json
 
   const info = await api.fileContents("my_path.md")
   expect(info).toEqual("The All-Defector is a purported glitch in the Dilemma Prison that appears to prisoners as themselves. This gogol always defects, hence the name.")//tslint:disable-line:max-line-length
@@ -36,7 +37,7 @@ describe("API testing", () => {
   beforeEach(() => {
     const mockSource = new FakeCI({})
 
-    api = new GitHubAPI("ABCDE", mockSource)
+    api = new GitHubAPI(mockSource, "ABCDE")
   })
 
   it("getUserInfo", async () => {
@@ -52,23 +53,19 @@ describe("API testing", () => {
   })
 
   it("updateCommentWithID", async () => {
-     api.fetch = fetch
-    expect(await api.updateCommentWithID(123, "Hello!")).toMatchObject({
-      api: "https://api.github.com/repos/artsy/emission/issues/comments/123",
-      body: "{\"body\":\"Hello!\"}",
-      headers: {
-        Authorization: "token ABCDE",
-        "Content-Type": "application/json",
-      },
-      method: "PATCH",
-    })
+    api.fetch = fetch
+    api.patch = jest.fn(() => ({ json: jest.fn() }))
+
+    await api.updateCommentWithID(123, "Hello!")
+
+    expect(api.patch).toHaveBeenCalledWith("repos/artsy/emission/issues/comments/123", {}, {"body": "Hello!"})
   })
 })
 
 describe("Peril", () => {
   it("Allows setting additional headers", async () => {
     const mockSource = new FakeCI({})
-    const api = new GitHubAPI("ABCDE", mockSource)
+    const api = new GitHubAPI(mockSource, "ABCDE")
     api.fetch = fetchJSON
     api.additionalHeaders = { "CUSTOM": "HEADER" }
 

source/platforms/platform.ts

@@ -71,7 +71,7 @@ export function getPlatformForEnv(env: Env, source: CISource): Platform {
     throw new Error("Cannot use authenticated API requests.")
   }
 
-  const api = new GitHubAPI(token, source)
+  const api = new GitHubAPI(source, token)
   const github = new GitHub(api)
   return github
 }