Pipelines always failing for external PRs

[janpaepke]

As pointed out here https://github.com/mollie/mollie-api-node/releases/tag/untagged-fa5121ce368cb7916910, all pipelines run against PRs from forked repos now fail, since they do not have access to the API test key used for integration tests.

From the github docs:

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

In our case the test API key being hardcoded into the workflow was not meant to be a long term solution. The idea was to run the tests against the actual API locally and record the responses as snapshots. Then in the pipelines only run tests against those snapshots.

To fix this now, we can either work to implement this test-flow now or create a separate pipline workflow for (forked?) pull requests. One obvious change would be to exclude the integration tests in those.

The downside here would be that failing tests would only appear once merged.

On the other hand this would also be a problem with the above mentioned flow of generating snapshots: Any external PR could not include the integration tests for their code changes and the pipelines would also not definitively verify wether or not the changes would break existing (integration) tests. For example they might work against the snapshots but fail agains the actual API results, which might be different with the changed code.

How do we want to approach this?

[edorivai]

I propose:

1. skip integration tests that require an API key when the PR comes from a forked repo
2. ensure that forked repos merge against a different branch than master (e.g. staging or something). This way we can review and check that all non-integration tests pass. Subsequently, we can make an internal PR from staging to master, which can run integration tests using secrets from this main repo.

Alternatively: external maintainers could add their own API Key to the secrets on their forked repo. This shifts the burden to the external devs, but worth considering.

[Pimm]

The idea was to run the tests against the actual API locally and record the responses as snapshots. Then in the pipelines only run tests against those snapshots.

Indeed. It also means local tests work out-of-the-box.

In my vision, "external" contributors would, as @edorivai suggests, generate snapshots using their own test API key.

Any external PR could not include the integration tests for their code changes and the pipelines would also not definitively verify wether or not the changes would break existing (integration) tests. For example they might work against the snapshots but fail agains the actual API results, which might be different with the changed code.

I don't think I get your concern. If an "external" contributor creates a PR which changes the requests going to the Mollie API, the test will fail because the snapshots don't match (and they might have to generate new snapshots). If the existing snapshots still match, that should give us confidence that the changes don't break anything, and should still work as expected when touching the real Mollie API.

The snapshots ‒ by their nature ‒ reflect the real Mollie API, so they should give us the same confidence as a test against the real Mollie API.

Could you elaborate?

[janpaepke]

The manual workaround of closing external PRs and re-submitting them from the main repo (as done with #432 → #433 and #466 → #469) is getting tedious.

Short-term proposal: Skip integration tests for forked PRs. Something like:

- name: Run test suite
  run: yarn test ${{ github.event.pull_request.head.repo.fork && '--ignore=tests/integration' || '' }}

This would:

• Unblock external PRs immediately
• Still run unit tests (which already use NetworkMocker for mocked responses)
• Be honest about coverage (we're already not running integration tests on external PRs anyway—they just fail)

The downside is that integration tests only run after merge, but that's effectively what happens now with the manual re-submit workflow.

Long-term: Snapshot-based testing as discussed above remains the proper solution. It would make all tests deterministic, work offline, and enable external contributors to fully test their changes.

Until we have time to implement snapshots, would the skip-integration-tests workaround be acceptable?