Skip to content

Commit 8fe0300

Browse files
committed
Unit-test the conversion toolchain, and fix the four defects that found (MTE-5766, MTE-5827)
The toolchain had no tests. Its verdicts are what we decide "done" on, and the way we checked them was to run a tool, read its output, and judge whether it looked right — which is how two confidently wrong diagnoses survived review. Both were in the gates written to prevent exactly that. 44 tests, stdlib unittest, no device or Firefox checkout needed: ps/lsof/adb/git are stubbed and the edge cases build synthetic batch dirs. The efftriage rule tests instead run against REAL labelled conversion runs, now checked in under tests/fixtures/corpus with a labels.json manifest. That distinction is the point: hand-written traces only exercise the shapes their author imagined, so they go green while the tool mismatches real output — a false green in the suite meant to prevent false greens. The corpus also existed only as batch dirs in one person's conversion-runs/, unversioned and impossible to run in CI, which is a poor home for the campaign's main diagnostic asset. Four defects, three of them in code already reviewed by eye and pronounced fine: - efftriage diagnosed GREEN runs. Traces normally carry non-fatal [ERR] lines (nav-graph polling, tolerated absence checks) and the rules scanned them whatever the outcome, so a passing run was reported as an absence-assertion failure. Two labelled runs in our own corpus were affected. Still triaged on purpose: a retry-pass, where attempt 1 did fail and clean=false means flaky-not-done, and a crash, since crash mode is where `outcome: pass` is itself the lie (MTE-5822). - efftriage blamed the harness for a typo: a nonexistent batch dir came back as gotcha A24, sending the reader to check effpretty resolution and pull dumps off a device. It also tracebacked. - effdoctor named the wrong queue — the failure it exists to prevent. effwatch is launched by a relative path, resolved with abspath against effdoctor's cwd instead of the watcher's, so from a Firefox checkout it reported a queue under firefox/tae-conversion/ that has never existed and told us to use it. Requests there are never consumed and never error. Now reads the watcher's own cwd (/proc, then lsof); a computed queue that is not on disk is a FAIL and an unreadable cwd is "undetermined", because declining to answer beats answering wrongly. - effdoctor's self-exclusion skipped any ps line containing the substring "effdoctor", so a watcher under a directory with that name was invisible. Unnoticed watchers are the whole point of the tool. Recorded as known gaps rather than papered over: rules T3/T4/T5 have no labelled example in the 53-run corpus, so they are unvalidated rather than known-good, and 3 of 28 failures still get no diagnosis. Both are asserted, so the lists cannot go stale in either direction.
1 parent 716f701 commit 8fe0300

35 files changed

Lines changed: 7302 additions & 6 deletions

File tree

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
name: tae-conversion-tests
2+
3+
on:
4+
workflow_dispatch:
5+
push:
6+
paths:
7+
- "tae-conversion/**"
8+
- ".github/workflows/tae-conversion-tests.yml"
9+
pull_request:
10+
paths:
11+
- "tae-conversion/**"
12+
- ".github/workflows/tae-conversion-tests.yml"
13+
14+
jobs:
15+
test:
16+
name: Run unit tests
17+
runs-on: ubuntu-latest
18+
19+
strategy:
20+
fail-fast: false
21+
matrix:
22+
python-version: ["3.9", "3.12"]
23+
24+
steps:
25+
- name: Checkout code
26+
uses: actions/checkout@v7
27+
28+
- name: Set up Python
29+
uses: actions/setup-python@v7
30+
with:
31+
python-version: ${{ matrix.python-version }}
32+
33+
# No dependency install step: the eff* tools are stdlib-only, and the tests
34+
# need no device, no emulator, no Firefox checkout and no network. The rule
35+
# tests run against real conversion runs checked in under
36+
# tae-conversion/tests/fixtures/corpus; everything that shells out to ps,
37+
# lsof, adb or git is stubbed.
38+
- name: Run tests
39+
run: python -m unittest discover -s tae-conversion/tests -p '*tests.py' -v
40+
41+
# ResourceWarnings are errors here: an unclosed file in a tool that is run
42+
# hundreds of times a session is a real leak, and warning noise in the output
43+
# is what makes people stop reading it.
44+
- name: Run tests with warnings as errors
45+
run: python -W error::ResourceWarning -m unittest discover -s tae-conversion/tests -p '*tests.py'

‎tae-conversion/CHANGELOG.md‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,41 @@ verdict semantics, changed flags. `git log` has the rest.
2525
resolving it against the caller's cwd yields a directory that never existed. A computed queue that is not on
2626
disk is reported as a failure, and an unreadable cwd as "undetermined": naming the wrong queue confidently is
2727
worse than declining to name one. (MTE-5766)
28+
- **Unit tests** (`tae-conversion/tests/`, 44 of them) and a CI job, where previously the toolchain had none.
29+
Run them with `python -m unittest discover -s tae-conversion/tests -p '*tests.py'`; no device, emulator,
30+
Firefox checkout or network needed, since `ps`/`lsof`/`adb`/`git` are stubbed. The efftriage rule tests run
31+
against **real labelled conversion runs** checked in under `tests/fixtures/corpus` with a `labels.json`
32+
manifest: hand-written traces only test the shapes their author imagined, so they pass while the tool
33+
mismatches real output — a false green in the suite meant to prevent false greens. That corpus previously
34+
existed only as batch dirs in one person's `conversion-runs/`, unversioned and unrunnable in CI. Writing
35+
these found four defects, three of them in code that had already been reviewed by eye and pronounced fine.
36+
- `ALT_TOOLS` env var on `effdoctor`, so the alt-checkout divergence check works for checkouts that are not at
37+
`~/Workspace/ui-test-modernization`.
2838
- `VERSION` + this changelog, and `--version` on all 12 tools. Version lookup resolves symlinks, so it stays
2939
correct when a tool is invoked through another checkout's `tools/` dir. (MTE-5770 slice 1)
3040
- Docs: lessons **K13** (ad surfaces are testable offline by faking the app-services client at the seam
3141
app-services itself uses in its unit tests) and gotchas **A37–A43**.
3242

3343
### Fixed
44+
- `efftriage.py` — **a green run was handed a failure diagnosis.** Traces routinely carry non-fatal `[ERR]`
45+
lines (nav-graph polling, tolerated absence checks), and the rules scanned them regardless of outcome, so a
46+
passing run was reported as an absence-assertion failure. Two of the campaign's own labelled runs were
47+
affected. A passing single-attempt run is no longer triaged; its non-fatal error count is reported as a note
48+
instead. Deliberately still triaged: a **retry-pass**, where attempt 1 really did fail and `clean=false`
49+
means flaky-not-done (now labelled `PASSED ONLY ON RETRY`), and a **crash**, because crash mode is where
50+
`outcome: pass` is itself the lie (MTE-5822). (MTE-5827)
51+
- `efftriage.py` — a nonexistent batch dir was diagnosed as gotcha A24 ("run-report.txt is missing"), sending
52+
the reader to check effpretty resolution and pull dumps off a device on account of a mistyped path. It is now
53+
reported as `no-such-batch`, and no longer tracebacks with a `KeyError`.
54+
- `effdoctor.py` — **named the wrong queue, which is the failure it exists to prevent.** effwatch is normally
55+
launched by a relative path, and that path was resolved with `os.path.abspath` — against effdoctor's cwd
56+
rather than the watcher's. Run from a Firefox checkout it reported a live queue under
57+
`firefox/tae-conversion/`, a directory that has never existed, and advised queueing into it; a request
58+
dropped there is never consumed and never errors. The watcher's own cwd is now read (`/proc` first, `lsof`
59+
fallback). A computed queue that is not on disk is a FAIL, and an unreadable cwd is reported as undetermined
60+
with the command to run: declining to answer beats answering wrongly. (MTE-5766)
61+
- `effdoctor.py` — the self-exclusion filter skipped every `ps` line containing the substring `effdoctor`, so
62+
a watcher running from a directory whose name contained it was invisible. Now skips only this process.
3463
- `effverify.py` — **a crashed test was scored as passed**, with `clean=true` and `failed_total=0`. Only
3564
`failed:` markers and the gradle status line were read, and a crash emits neither (`"gradle": null` was the
3665
tell). Now also reads the report's `FAILURES (n of m)` header and `CRASH:` lines, and refuses to report
@@ -47,6 +76,12 @@ verdict semantics, changed flags. `git log` has the rest.
4776
a `NameError` that is now fixed; gotcha **A24** now carries its root cause.
4877

4978
### Known gaps
79+
- `efftriage` rules **T3, T4 and T5 have no labelled example** anywhere in the 53-run corpus they were
80+
hardened against, so they are unvalidated rather than known-good — a rule that has never fired on a real
81+
failure may not match the shape it was written for at all. Listed in `UNVALIDATED_RULES` in the tests, which
82+
fail if the list goes stale in either direction.
83+
- 3 of the 28 failed runs in the corpus get **no diagnosis** (`gap-*` fixtures). They are checked in as
84+
tripwires: add a rule that explains one and the test fails, which is the reminder to record the win.
5085
- Mechanized coverage is not yet reported. `effgates --coverage` plus a `Gate:` line on every doc entry is
5186
MTE-5830; the `effcheck` verb × selector-strategy matrix is MTE-5828; `effparity` (legacy-vs-port assertion
5287
parity, gating lesson G1) is MTE-5829.

0 commit comments

Comments
 (0)