Improve tests

[ewolfe]

• We can get a little closer to 100%
• nock's URL's need to have additional meta data (body payload) attached to truly verify the test - in the current state there's not really any confidence in the test suite due to the nature of the input/output of this program really dependent on the side effect calls to GitHub's API. Thus the need to build more confidence in the calls to the GH API by setting up stricter nock methods.

[ewolfe]

Should also look into https://facebook.github.io/jest/docs/en/mock-functions.html

[ewolfe]

In retrospect, I want to take a different approach. At a high level, PRLint is not a pure function f(x) = y. In fact, the consumer of PRLint (GitHub's webhooks) actually care very little about the return value y. The real value of the webhooks PRLint exposes is in it's side effects of updating a pull request with a fail/pass status.

With that said, I haven't found an ideal way of testing side effects for this application. So, in the mean time I'm going to wait for Spotify to release some tooling that sounds promising https://github.com/facebook/jest/issues/6081#issuecomment-417849367

[ewolfe]

More resources:

• https://hackernoon.com/api-testing-with-jest-d1ab74005c0a
• https://sinonjs.org/releases/v2.1.0/fake-xhr-and-server/#fake-server
• https://codeutopia.net/blog/2015/01/30/how-to-unit-test-nodejs-http-requests/
• https://mherman.org/blog/stubbing-http-requests-with-sinon/
• https://www.npmjs.com/package/request-spy

[mrchief]

At a glance, this seems like something #96 can help.

Yes, I'd say so 🙂 The functions would be jest spies essentially, so you would be able to do toHaveBeenCalledWith() and other goodies.

We can do this with #96 although I like the idea of verifying the endpoint behavior which we have right now rather than bothering with covering every line of code. If we had the later, verifying the functionality after a major refactor (like #96) would have been much harder. Lines of code may and will change but the contract of the app should not. The current tests enforce that.

Thoughts?

[ewolfe]

expect(body).toMatchObject(issueCreatedBody) via https://probot.github.io/docs/testing/ seems like it would give us the real confidence we want in a test. Pasting the full snippet here for posterity:

const nock = require('nock')
// Requiring our app implementation
const myProbotApp = require('..')
const { Probot } = require('probot')
// Requiring our fixtures
const payload = require('./fixtures/issues.opened')
const issueCreatedBody = { body: 'Thanks for opening this issue!' }

nock.disableNetConnect()

describe('My Probot app', () => {
  let probot

  beforeEach(() => {
    probot = new Probot({})
    // Load our app into probot
    const app = probot.load(myProbotApp)

    // just return a test token
    app.app = () => 'test'
  })

  test('creates a passing check', async () => {
    // Test that we correctly return a test token
    nock('https://api.github.com')
      .post('/app/installations/2/access_tokens')
      .reply(200, { token: 'test' })

    // Test that a commented is posted
    nock('https://api.github.com')
      .post('/repos/hiimbex/testing-things/issues/1/comments', (body) => {
        expect(body).toMatchObject(issueCreatedBody)
        return true
      })
      .reply(200)

    // Receive a webhook event
    await probot.receive({ name: 'issues', payload })
  })
})

[mrchief]

expect(body).toMatchObject(issueCreatedBody) via https://probot.github.io/docs/testing/ seems like it would give us the real confidence we want in a test

How are these for confidence? :)

• index.test.js#L84
• index.test.js#L121
• index.test.js#L135

[ewolfe]

Awesome, looks good!