Test Workflow Does Not Test Against Expectations - Use Mocks Instead of GitHub App

[kj4ezj]

Problem

Respectfully, pull request 64 from pull request 55 and issue 54 just broke all 78 forks of your project and increased the barrier to entry for contributors to submit pull requests.

For example, here I pushed a commit to my fork of your project containing my proof that P = NP, and here you can see your workflow fails in my fork because I have not installed a GitHub App that satisfies the new test. This can also happen on base branches, release branches, and tags in forks and people may incorrectly believe your action is broken when it is not. While it may sound simple to you and I to install an app that satisfies your test, that process is undocumented and it requires admin approval in organizations that could be prohibitive for developers to obtain.

A test that relies on a GitHub repo to be configured in a specific way such that it fails on forks is not a robust or reliable test. There are several reasons why a test should not rely on external factors like this:

1. Non-deterministic results: Tests that rely on external factors like the configuration of a GitHub repo may produce inconsistent or non-deterministic results, making it difficult to know if the system is working correctly or not.
2. Environment-specific: Tests that rely on external factors may only work in specific environments and may not be portable across different development, staging, or production environments.
3. Maintenance: Tests that rely on external factors may require additional maintenance to ensure that they continue to work correctly as the external factors change over time.
4. Lack of isolation: Tests that rely on external factors are not properly isolated from the environment and other dependencies, and therefore they may be affected by changes that are unrelated to the code being tested.
5. Lack of scalability: Tests that rely on external factors may not be able to scale as the system or the number of users grows, leading to performance issues or time-consuming tests.

It is important to design tests that are robust, reliable, and isolated from external factors to ensure that they can produce consistent and accurate results and can be easily maintained over time.

Consider the purpose of your test and what it is fundamentally testing. Your existing test does not only test code correctness, it also tests for the existence of a GitHub App. We should write a test that tests for code correctness and nothing else.

Solution

There are a lot of valid ways to solve this problem. I, personally, really love the jest test framework. It allows you to mock libraries including octokit very easily.

Here is an example written by ChatGPT using jest to mock octokit and implementing a test against your fetchInstallationToken function in fetch-installation-token.ts.

import { fetchInstallationToken } from "./fetchInstallationToken";
import { getOctokit } from "@actions/github";
import { request } from "@octokit/request";

// mocks
jest.mock("@octokit/rest", () => {
  return {
    apps: {
      getRepoInstallation: jest.fn().mockResolvedValue({
        data: { id: 123 }
      }),
      createInstallationAccessToken: jest.fn().mockResolvedValue({
        data: { token: 'installation-token' }
      })
    }
  }
});

jest.mock("@actions/github", () => {
  return {
    getOctokit: jest.fn()
  }
});

jest.mock("@octokit/request", () => {
  return {
    defaults: jest.fn()
  }
});

// clean up after each test
afterEach(() => {
  jest.clearAllMocks();
});

// test suite
describe("fetchInstallationToken", () => {
  test("should fetch the installation token", async () => {
    const result = await fetchInstallationToken({
      appId: "123",
      githubApiUrl: new URL("https://api.github.com"),
      owner: "owner",
      repo: "repo",
      privateKey: "private-key"
    });

    expect(result).toBe("installation-token");
    expect(getOctokit).toHaveBeenCalledWith("installation-token");
    expect(request.defaults).toHaveBeenCalledWith({
      baseUrl: "https://api.github.com"
    });
  });
});

---

Instead of jest mocks, you could also use nock to intercept the REST API calls octokit makes to the Internet. Here is an example of that from ChatGPT.

import nock from "nock";
import { fetchInstallationToken } from "./fetchInstallationToken";

describe("fetchInstallationToken", () => {
  afterEach(() => {
    jest.clearAllMocks();
    nock.cleanAll();
  });

  test("should return installation token", async () => {
    const appId = "123";
    const githubApiUrl = new URL("https://api.github.com");
    const installationId = 1234;
    const owner = "octocat";
    const repo = "hello-world";
    const privateKey = "-----BEGIN RSA PRIVATE KEY-----\n...";

    const installationAccessToken = "abcdefghijklmnopqrstuvwxyz";

    nock(githubApiUrl.toString())
      .post("/app/installations/1234/access_tokens", {})
      .reply(200, {
        token: installationAccessToken,
      });

    const token = await fetchInstallationToken({
      appId,
      githubApiUrl,
      installationId,
      owner,
      privateKey,
      repo,
    });

    expect(token).toEqual(installationAccessToken);
  });
});

I strongly recommend against this solution because the purpose of the test is to test this GitHub Action for correctness, not to test octokit for correctness. The jest mock example solely tests this GitHub Action for correctness, whereas the nock example tests both the GitHub Action and octokit for correctness. If GitHub changed the implementation of their REST API and octokit changed in kind, this test would fail on passing code. I included an example with nock because it does remove the GitHub repo configuration as a variable and it is a tool worth knowing about.

---

I also like to add this to my package.json when working with jest.

{
  "scripts": {
    "test": "jest --coverage"
  },
  "jest": {
    "clearMocks": true,
    "collectCoverage": true,
    "collectCoverageFrom": [
      "**/*.js",
      "**/*.ts"
    ],
    "coveragePathIgnorePatterns": [
      "coverage",
      "dist",
      "node_modules"
    ],
    "coverageThreshold": {
      "global": {
        "branches": 100,
        "functions": 100,
        "lines": 100,
        "statements": 100
      }
    },
    "testPathIgnorePatterns": [
      "coverage",
      "dist",
      "node_modules"
    ]
  }
}

This allows you to run jest using yarn test, and requires full test coverage.

Your GitHub Action test workflow would become:

name: Test
on:
  push:
    branches-ignore:
      - main

jobs:
  test:
    name: Test
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - run: yarn install --frozen-lockfile
      - run: yarn run yarn-deduplicate --fail --strategy fewer
      - run: yarn run build
      - run: yarn run prettier --check
      - run: yarn run xo
      - run: yarn test

---

Legal notice:
This document was generated in collaboration with the 2023-01-09 version of ChatGPT from OpenAI, a machine learning algorithm or weak artificial intelligence (AI). At the time of this writing, the OpenAI terms of service agreement §3.a states:

Your Content. You may provide input to the Services (“Input”), and receive output generated and returned by the Services based on the Input (“Output”). Input and Output are collectively “Content.” As between the parties and to the extent permitted by applicable law, you own all Input, and subject to your compliance with these Terms, OpenAI hereby assigns to you all its right, title and interest in and to Output.

I release this content under the MIT license.

This notice is required in some countries.

[tibdex]

Hi,

I'm not a fan of mocking.

I want my tests to:

• make me confident that the whole system works
• not be coupled to implementation details

I thus prefer black-box testing which can either be:

• unit tests of pure functions (not needed here as the code is almost branchless and the types are enforced by TypeScript)
• integration tests of subsystems or the whole system (what Upgrade deps and add test #64 does)

To unblock you and the forks, I created #67.