Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
e62e12f6c6 |
@@ -133,10 +133,71 @@ status and `GET /api/v1/repos/yonlu/yellowjacket/actions/jobs/<id>/logs` for
|
|||||||
the log — and fix it. Two consecutive failed CI runs on the same cause: stop,
|
the log — and fix it. Two consecutive failed CI runs on the same cause: stop,
|
||||||
comment what you know on the PR, and leave it for a human.
|
comment what you know on the PR, and leave it for a human.
|
||||||
|
|
||||||
**Do not merge.** Comment on the issue linking the PR, leave
|
**Do not merge, and do not review your own work.** Comment on the issue
|
||||||
`Status/In Progress` on, and end the run.
|
linking the PR, leave `Status/In Progress` on, and end the run. Whether
|
||||||
|
this merges is decided by a reviewer that did not write it — see below.
|
||||||
|
A CI run is not a review: it proves the tests you wrote pass, which is
|
||||||
|
exactly the thing an author is worst placed to judge.
|
||||||
|
|
||||||
## Finally
|
## Finally
|
||||||
|
|
||||||
Report in three lines: which issue you took, what state it is in
|
Report in three lines: which issue you took, what state it is in
|
||||||
(PR open / CI green / stopped and why), and any issues you filed.
|
(PR open / CI green / stopped and why), and any issues you filed.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# The merge gate
|
||||||
|
|
||||||
|
This half is **not** run by the author. It is run against a PR by
|
||||||
|
someone who has not seen the branch before, and it decides whether the
|
||||||
|
work lands on its own or waits for a human.
|
||||||
|
|
||||||
|
A push to `main` publishes nothing here — `release.yml` is
|
||||||
|
`workflow_dispatch` only and all four publishers key on `v*` tags — so
|
||||||
|
the cost of a wrong merge is a bad commit on `main` and the time to
|
||||||
|
revert it. That is the whole reason this gate can exist. If that ever
|
||||||
|
changes, this section is void.
|
||||||
|
|
||||||
|
**Merge only when every one of these is true.** Any single no means
|
||||||
|
leave it open.
|
||||||
|
|
||||||
|
1. An independent review of the diff returns **MERGE** or **MERGE WITH
|
||||||
|
NITS**. `CHANGES NEEDED`, or a review that could not reach a verdict,
|
||||||
|
means a human looks.
|
||||||
|
2. `CI / check (pull_request)` **and** `CI / e2e (pull_request)` are
|
||||||
|
`success` on the PR's current head. Ignore the `(push)` contexts —
|
||||||
|
they are `skipped` by design and Gitea folds `skipped` into a
|
||||||
|
combined state of `pending` that never clears.
|
||||||
|
3. The PR is mergeable with no conflicts, rebased onto current `main`.
|
||||||
|
4. The diff touches **none** of: `.gitea/workflows/`, `.releaserc.yml`,
|
||||||
|
`packaging/`, `build/`, `scripts/gitea-release.sh`,
|
||||||
|
`backend/database/sql/schemas/`, `backend/database/staleshape*.go`,
|
||||||
|
`go.mod`, `go.sum`. These either publish to somewhere a mistake
|
||||||
|
cannot be taken back from, or can destroy a database that a user
|
||||||
|
cannot rebuild.
|
||||||
|
5. The issue is `Kind/Bug`, `Kind/Testing`, `Kind/Documentation` or
|
||||||
|
`Kind/Enhancement`. **A `Kind/Feature` is a design decision and is
|
||||||
|
never auto-merged**, however green it is.
|
||||||
|
6. The diff is under ~600 changed lines across under ~15 files. Past
|
||||||
|
that, "a reviewer read it" stops being a claim anyone should take on
|
||||||
|
trust.
|
||||||
|
7. The PR does not claim to have verified something no tier here can
|
||||||
|
see. A change whose evidence would have to come from a physical
|
||||||
|
device is reported, not merged.
|
||||||
|
|
||||||
|
**When it merges**, use `{"Do":"merge"}` on
|
||||||
|
`POST /api/v1/repos/yonlu/yellowjacket/pulls/<n>/merge`. Then **check
|
||||||
|
the issue actually closed** — a squash or an edited merge message drops
|
||||||
|
the `Closes` footer — and close it by hand with a comment naming the
|
||||||
|
commit if it did not. `unclaim.yml` strips `Status/In Progress` on
|
||||||
|
close; if the label is still there a minute later, strip it yourself.
|
||||||
|
|
||||||
|
**When it does not merge**, say so on the PR in one paragraph: which
|
||||||
|
condition failed and what would satisfy it. Leave the PR open, leave
|
||||||
|
the label on, and file the review's substantive findings as issues so
|
||||||
|
they are searchable rather than buried in a PR comment.
|
||||||
|
|
||||||
|
**A nit is not a blocker, and it is not free either.** A `MERGE WITH
|
||||||
|
NITS` merges, and each nit worth keeping becomes an issue. Do not fix
|
||||||
|
nits on the branch: that is a second author pass with no second review,
|
||||||
|
which is the thing this gate exists to prevent.
|
||||||
|
|||||||
@@ -262,26 +262,6 @@ real store code. A binding carries an **ID**, not a name
|
|||||||
`yellowjacket/backend/home.Service.GetShelves`), so the fake derives
|
`yellowjacket/backend/home.Service.GetShelves`), so the fake derives
|
||||||
that map from the generated tree rather than writing it down.
|
that map from the generated tree rather than writing it down.
|
||||||
|
|
||||||
**A test file does not get its own origin, so `setup.ts` clears
|
|
||||||
`localStorage` between tests.** `@vitest/browser-playwright` opens one
|
|
||||||
BrowserContext per session and runs several files in it one after
|
|
||||||
another, so everything a component persists — the track list's sort and
|
|
||||||
column widths, the cover size, `now-playing`'s scroll mode — is still
|
|
||||||
there when the next file mounts the same component. Which files share a
|
|
||||||
tab, and in what order, changes run to run, so the symptom is a spec
|
|
||||||
that fails about one test in three and passes every time it is run on
|
|
||||||
its own: #138 cost three scheduled runs, one of them a PR whose diff
|
|
||||||
held no frontend code at all. Measured on the build before the fix, a
|
|
||||||
single full run started **24** tests with storage already set. Two
|
|
||||||
things follow. The clear is safe precisely because the leak is
|
|
||||||
sequential — files in a session do not overlap, so it cannot wipe
|
|
||||||
storage a concurrently-running file is in the middle of using — and it
|
|
||||||
belongs in `setup.ts` rather than in the specs that write, because the
|
|
||||||
spec that *reads* is never the one that knows. And a spec whose
|
|
||||||
assertion depends on an order still **states that order** rather than
|
|
||||||
inheriting a default, or the next change to a default is the same
|
|
||||||
mystery again.
|
|
||||||
|
|
||||||
**`frontend/bindings/` is generated by `wails3`, not `go generate`**, so
|
**`frontend/bindings/` is generated by `wails3`, not `go generate`**, so
|
||||||
the pre-commit codegen check does not cover it. `make bindings-check`
|
the pre-commit codegen check does not cover it. `make bindings-check`
|
||||||
(~3.5 s warm, ~20 s on a cold build cache, also a pre-commit hook)
|
(~3.5 s warm, ~20 s on a cold build cache, also a pre-commit hook)
|
||||||
|
|||||||
@@ -161,15 +161,6 @@ describe('double-clicking a row in the track list', () => {
|
|||||||
localStorage.removeItem('track-list-column-widths');
|
localStorage.removeItem('track-list-column-widths');
|
||||||
|
|
||||||
el = await fixture<LitElement>('track-list', { externalTracks: LIST });
|
el = await fixture<LitElement>('track-list', { externalTracks: LIST });
|
||||||
|
|
||||||
// Say which order is being asserted rather than inheriting one.
|
|
||||||
// `restoreSortPreferences()` runs in `connectedCallback`, so the
|
|
||||||
// list opens in whatever sort was last persisted -- and
|
|
||||||
// `track-11` sorts before `track-3` by title, which is the shape
|
|
||||||
// of #138. The row below is the fixture's third track only while
|
|
||||||
// nothing is sorting the list.
|
|
||||||
(el as unknown as { sortField: string | null }).sortField = null;
|
|
||||||
|
|
||||||
el.style.display = 'block';
|
el.style.display = 'block';
|
||||||
el.style.height = '600px';
|
el.style.height = '600px';
|
||||||
await flush();
|
await flush();
|
||||||
@@ -181,10 +172,6 @@ describe('double-clicking a row in the track list', () => {
|
|||||||
const rows = shadowAll(el, '.track-row');
|
const rows = shadowAll(el, '.track-row');
|
||||||
const row = rows.find((r) => r.getAttribute('data-index') === '3');
|
const row = rows.find((r) => r.getAttribute('data-index') === '3');
|
||||||
|
|
||||||
// Stated first, so a list that is not in the order this asserts
|
|
||||||
// fails by saying so rather than as an off-by-eight file path.
|
|
||||||
expect(row?.getAttribute('data-file-path')).toBe('/music/track-3.mp3');
|
|
||||||
|
|
||||||
dblclick(row!);
|
dblclick(row!);
|
||||||
await flush();
|
await flush();
|
||||||
|
|
||||||
|
|||||||
@@ -72,23 +72,6 @@ document.body.style.margin = '0';
|
|||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
resetHarness();
|
resetHarness();
|
||||||
|
|
||||||
// A test file does not get its own origin. `@vitest/browser-playwright`
|
|
||||||
// opens one BrowserContext per session and runs several files in it,
|
|
||||||
// one after another, so everything a component persists — the track
|
|
||||||
// list's sort and column widths, the cover size, `now-playing`'s
|
|
||||||
// scroll mode — is still there when the next file mounts the same
|
|
||||||
// component. That is invisible until it is intermittent, because
|
|
||||||
// which files share a tab and in what order changes run to run: it
|
|
||||||
// cost #138 three scheduled runs, one of them a PR with no frontend
|
|
||||||
// code in it at all.
|
|
||||||
//
|
|
||||||
// Clearing here rather than in the specs that write is deliberate —
|
|
||||||
// the spec that *reads* is never the one that knows. It is safe for
|
|
||||||
// the same reason the leak exists: files in a session are
|
|
||||||
// sequential, so this cannot wipe storage a concurrent file is in
|
|
||||||
// the middle of using.
|
|
||||||
localStorage.clear();
|
|
||||||
});
|
});
|
||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
|
|||||||
Reference in New Issue
Block a user