|
| 1 | +# GitHub Real-Provider E2E — Test Plan |
| 2 | + |
| 3 | +Issue #57 (Agent 02). Companion to Agent 01's shared E2E harness (`e2e/providers/provider-adapter.ts`, `e2e/verifier/verifier-contract.ts`) and the Gitea (`e2e/suites/gitea.e2e.test.ts`) / GitLab (`e2e/suites/gitlab.e2e.test.ts`) suites built on it. |
| 4 | + |
| 5 | +## Architecture |
| 6 | + |
| 7 | +``` |
| 8 | +production GitHubService (src/services/github-service.ts) |
| 9 | + ↓ |
| 10 | +real GitHub sandbox repository (firstsun-dev/obsidian-sync-test) |
| 11 | + ↓ |
| 12 | +independent GitHubVerifier (raw REST API — Contents, branches, trees, commits) |
| 13 | +``` |
| 14 | + |
| 15 | +The production service is exercised unmodified; every remote assertion goes through `GitHubVerifier`, never by reading the service's own writes back through itself. |
| 16 | + |
| 17 | +- `e2e/provision/github-provision.ts` — validates the fine-grained PAT against the sandbox repo, then creates a run-specific branch off `main` (default; override with `E2E_GITHUB_BASE_BRANCH`). GitHub is hosted, so "provisioning" isolation means a fresh branch per run, not a container — teardown deletes that branch. |
| 18 | +- `e2e/verifier/github-verifier.ts` — implements the shared `RemoteVerifier` contract (`getFile`, `listFiles`, `fileMissing`) plus GitHub-specific extras used only by this suite: `getBlobMode`, `getRawEntry` (symlink target), `listCommitShas`, `getCommitMessage`. |
| 19 | +- `e2e/providers/github-adapter.ts` — wires `GitHubService` + `GitHubVerifier` into the shared `ProviderE2EAdapter` contract. |
| 20 | +- `e2e/suites/github.e2e.test.ts` — the test suite itself. |
| 21 | + |
| 22 | +## Required secrets / config |
| 23 | + |
| 24 | +Set on `firstsun-dev/git-files-sync` (this repo) for CI, or exported locally for `npm run test:e2e -- --provider github`: |
| 25 | + |
| 26 | +| Name | Kind | Purpose | |
| 27 | +|---|---|---| |
| 28 | +| `E2E_GITHUB_TOKEN` | secret | Fine-grained PAT, scoped to the sandbox repo only, **Contents: Read and write** permission (covers file reads/writes, `createCommitOnBranch`, and branch-ref create/delete for setup+teardown) | |
| 29 | +| `E2E_GITHUB_OWNER` | variable | `firstsun-dev` | |
| 30 | +| `E2E_GITHUB_REPO` | variable | `obsidian-sync-test` | |
| 31 | +| `E2E_GITHUB_BASE_BRANCH` | variable (optional) | defaults to `main` | |
| 32 | + |
| 33 | +These have been set on `firstsun-dev/git-files-sync` via `gh secret set` / `gh variable set`. |
| 34 | + |
| 35 | +## Sandbox repository assumptions |
| 36 | + |
| 37 | +- `firstsun-dev/obsidian-sync-test` is dedicated to E2E use — never a real user's repo. |
| 38 | +- It has a `main` branch with at least one commit (branch creation reads `main`'s current SHA as the fork point). |
| 39 | +- The PAT's org (`firstsun-dev`) may require admin approval of fine-grained PATs before they're usable — this blocked the first live run until approved. |
| 40 | + |
| 41 | +## Coverage |
| 42 | + |
| 43 | +Common provider contract (mirrors the Gitea/GitLab suites): |
| 44 | + |
| 45 | +- `testConnection` |
| 46 | +- create (`pushFile`, via `createCommitOnBranch`) |
| 47 | +- read (`getFile`) |
| 48 | +- update (`pushFile` with existing sha) |
| 49 | +- delete (`deleteFile`) |
| 50 | +- batch push (`pushBatch`) — asserted as exactly **one** new commit for N files, not N |
| 51 | +- rename/move (`commitBatch`) — add+delete in one commit |
| 52 | + |
| 53 | +GitHub-specific regression/behavior coverage: |
| 54 | + |
| 55 | +- symlink push (`pushSymlink`, Git Data API, blob mode `120000`) — content/target verified via the raw Contents API's `type`/`target` fields |
| 56 | +- a real GraphQL **HTTP 200 with `errors[]`** rejection, forced deterministically via a file path that collides with an existing directory (no timing dependency, unlike a stale-head race) |
| 57 | +- `expectedHeadOid` self-healing under **genuine concurrent writes** to the same branch (2 concurrent single-file pushes — see "Known limitation" below for why not higher) |
| 58 | + |
| 59 | +## Known limitation: concurrency test size |
| 60 | + |
| 61 | +`commitOnBranch`'s retry budget is fixed at 3 attempts with a 500ms×attempt backoff. Live testing showed 3+ concurrent writers to the same branch can legitimately exhaust that budget under real GitHub write latency — that's a real characteristic of the production retry budget, not a test bug, but it made the test flaky at that concurrency level. The suite uses 2 concurrent writers: the minimum that still forces a real race while staying reliably within budget. |
| 62 | + |
| 63 | +## Known limitation: GitHub API read-after-write lag |
| 64 | + |
| 65 | +GitHub's REST Contents/commits/trees endpoints were observed live to occasionally return pre-write state immediately after a mutation lands (a real propagation characteristic, not a client-side cache — plain Node `fetch` is used, no caching layer). Every verifier read that immediately follows a write in this suite polls via a local `waitFor`/`waitForContent`/`waitForMissing` helper (`e2e/suites/github.e2e.test.ts`) instead of asserting on a single read. Confirmed stable across 3+ consecutive live runs after adding this. |
| 66 | + |
| 67 | +## Production bugs found (and fixed) via this live testing |
| 68 | + |
| 69 | +Both in `STALE_HEAD_ERROR` (`src/services/github-service.ts`) — the regex `commitOnBranch` uses to decide whether a `createCommitOnBranch` failure is retryable: |
| 70 | + |
| 71 | +1. A genuine concurrent-write race reports `"Ref refs/heads/<branch> is at <oid> but expected <oid>"` — didn't match any existing pattern, so the retry never fired. |
| 72 | +2. A rename's deletion side, read immediately after the file's own create, reports `"A path was requested for deletion which does not exist as of commit oid <oid>"` — a different phrasing than the already-handled `"does not exist in tree"`, also unmatched. |
| 73 | + |
| 74 | +Both patterns were added to the regex, with regression tests in `tests/services/github-service.test.ts` reproducing the exact live wording via mocked `requestUrl` responses. |
| 75 | + |
| 76 | +## Harness gap found (and fixed): `window` is undefined under Node |
| 77 | + |
| 78 | +`GitHubService.commitOnBranch`'s retry backoff uses `window.setTimeout` — correct for Obsidian's Electron renderer (where `window` always exists), but the E2E harness runs under `environment: 'node'` (`vitest.e2e.config.ts`) for a real `fetch`, which has no `window`. Fixed with a minimal shim (`e2e/shim/window-timers.ts`, wired via `setupFiles`) that aliases `window` to `globalThis` — not Gitea/GitLab-specific, so any future provider whose production code touches `window` benefits from it too. |
| 79 | + |
| 80 | +## Running |
| 81 | + |
| 82 | +``` |
| 83 | +E2E_GITHUB_OWNER=<owner> E2E_GITHUB_REPO=<repo> E2E_GITHUB_TOKEN=<fine-grained PAT> \ |
| 84 | + npm run test:e2e -- --provider github |
| 85 | +``` |
| 86 | + |
| 87 | +Verified: 3 consecutive clean runs (10/10 tests) against `firstsun-dev/obsidian-sync-test` as of 2026-08-07. |
0 commit comments