diff options
Diffstat (limited to 'docs')
3 files changed, 216 insertions, 64 deletions
diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index cfa119b..221b3b2 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -4841,3 +4841,55 @@ blocked emit when the condition is disabled, and it did not hang, which is the outcome item 85's trap made worth checking for. The user hand tested all three surfaces with an empty query bar and with one, including the disjunction case the parenthesising exists for. + +## 84. A config problem blocks `test_mainwindow` on a modal nobody can dismiss + +**Done 2026-08-14, unreleased.** + +**Observed (2026-08-14):** a new test in `test_mainwindow` hung with no output +and was killed at the two-minute timeout. It had configured an account section +carrying only `sent=`, with no `maildir=`. + +**Cause (verified by attaching gdb to the hung process, not inferred).** + +``` +#7 QDialog::exec() +#9 MainWindow::showWarnings ... src/mainwindow.cpp:1670 +#10 MainWindow::MainWindow ... src/mainwindow.cpp:381 +``` + +`Config::load` handled the malformed account exactly as it should, recording +"Account 'one' has no maildir; ignoring it" and carrying on. `showWarnings()` +then put every collected problem in a `QMessageBox::warning`, modal, called from +the `MainWindow` CONSTRUCTOR. Under the offscreen platform nothing can dismiss +it, so the constructor never returned. + +**The application was never wrong here.** The modal is right for a person: a +config problem should interrupt startup rather than scroll past. The defect was +that a TEST could not dismiss it, and that the failure was a silent hang rather +than an error naming its cause. + +**Fixed by splitting the function rather than suppressing the dialog.** +Suppressing it under `QTEST_MAIN` was rejected in the entry before any code was +written, because that is exactly the path `test_mainwindow` exercises and a +suppressed dialog ships the startup warning untested. Instead `showWarnings()` +became two: `applyWarnings()`, which sets the status label and stays in the +constructor, and `configProblems()`, a getter. `main.cpp` raises the dialog +after `show()`, which also gives it a visible parent to sit on. + +The warnings/problems distinction is preserved exactly, including its comment: a +keybinding the user wrote and that is being ignored interrupts startup, a notice +such as "no sync command configured" does not. + +**Verified.** 23 of 23 tests passing. The warning path has its first test, +built from the config shape that caused the original hang. Mutation checked by +putting the modal back in the constructor: the test times out at exit 124 rather +than failing, reproducing the original symptom exactly, which is the strongest +form this check could take. The user hand tested both halves of the +distinction: `archive=NotAKey` under `[keys]` surfaces the dialog over the +window, and the same line under `[general]` correctly does not, since it is not +a keybinding and nothing is broken. + +**The consequence for item 36.** That item could not add fixture-backed tests +while a malformed config could hang the constructor. It can now, which is why +84 was done first. diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index f578a3a..9240ad6 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -100,7 +100,7 @@ taking that too literally. | 33 | Status bar messages never expire | feedback | S | **done** | | 34 | No overview of the Maildir itself | information | M | **done** | | 35 | No refresh of the thread list after a sync | workflow | M | **done** 2026-08-10; the list now follows a sync on its own | -| 36 | `test_mainwindow` cannot reach the worker | testing | S | open, on demand | +| 36 | `test_mainwindow` cannot reach the worker | testing | S-M | open, specced 2026-08-14; see `specs/2026-08-14-mainwindow-worker-fixture-design.md`. No longer on demand: item 66 needs it. Smaller than it reads, `wireWorker()` already builds the worker from a config key | | 37 | The worker stalls on a tag edit made during a background sync | correctness | S | **done** | | 38 | `test_mainwindow` fails when a real sync holds the lock | testing | XS | **done** | | 39 | Thread list cannot be sorted by clicking a column header | workflow | S | **dropped** 2026-08-10; the card list has no column headers to click, and 0.13.0 shipped a sort dropdown instead | @@ -148,7 +148,7 @@ taking that too literally. | 81 | No way to turn a saved query into a tagging rule | workflow | S | **done** 2026-08-14, unreleased; see `specs/2026-08-14-query-to-rule-design.md` | | 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | **done** 2026-08-13, shipped in 0.18.0. Right-click offers Edit, Pin/Unpin and Delete | | 83 | A rule named with spaces is written to the file and dropped by every reader | defect | S | **done** 2026-08-14, unreleased. The name is sanitised into an id, save validates, a bad id loads for repair | -| 84 | A config problem blocks `test_mainwindow` on a modal nobody can dismiss | testing | S | open; measured 2026-08-14, `showWarnings()` calls `QMessageBox::warning` from the constructor | +| 84 | A config problem blocks `test_mainwindow` on a modal nobody can dismiss | testing | S | **done** 2026-08-14, unreleased. `showWarnings()` split: the status label stays in the constructor, `main.cpp` raises the modal after `show()` | | 85 | Nothing on screen can be searched for by right-clicking it | workflow | M | **done** 2026-08-14, unreleased; see `specs/2026-08-14-search-from-message-design.md`. Split from 78; rebuilt the details dialog as rows | | 86 | A right-click search can replace or narrow, but never exclude | workflow | S | **done** 2026-08-14, unreleased; see `specs/2026-08-14-exclude-from-search-design.md`. Follows 85. The `extend` bool became a `SearchMode` enum across four signatures | @@ -246,17 +246,26 @@ one did. Where a real message exposes a parser bug, hand-write a minimal `.eml` reproducing its **shape**, as `truncated.eml` and `hostile_filename.eml` already do. -**Approach.** Give `test_mainwindow` the fixture and point the window's worker at -it, so a test can select a thread and let a real `threadLoaded` arrive. +**Specced 2026-08-14: read +`specs/2026-08-14-mainwindow-worker-fixture-design.md` rather than planning from +here.** -**Do this when a defect needs it, not before** (user, 2026-08-04). Wiring it -with nothing to test proves nothing. The two defects above are already fixed; -this item is the note that the next one of its kind should be tested properly -rather than modelled. +**No longer on demand.** The entry said to do this when a defect needs it, not +before (user, 2026-08-04). Item 66 needs it now, so the deliverable is a RED +reproduction of that defect, not fixture wiring on its own. Fixing 66 is +deliberately excluded: it has never been isolated, and designing a fix beside a +hypothesis is how a wrong one gets locked in. + +**Smaller than this entry has read since 2026-08-04.** No hook has to be added +to `MainWindow`. `wireWorker()` (`src/mainwindow.cpp:1440`) already builds the +worker from `m_config.notmuchConfig()`, an ordinary config key, so a test that +writes a `qtmaildir.conf` pointing at the fixture gets a real worker through the +shipping path with nothing in `src/` changed. Mind the `[general]` prefix trap +when writing that file. **Constraint:** every existing `test_mainwindow` case constructs a bare -`MainWindow` and must keep working. The fixture is per-test, not a suite-wide -`initTestCase`, or every case pays for a `notmuch new`. +`MainWindow` and must keep working. The fixture is opt-in per test, not a +suite-wide `initTestCase`, or every case pays for a `notmuch new`. ## 40. No live filter over the current view @@ -524,60 +533,6 @@ in CLAUDE.md. **Size: S**, down from M now that item 85 has built the menus and item 81 the seeded dialog. -## 84. A config problem blocks `test_mainwindow` on a modal nobody can dismiss - -**Observed (2026-08-14):** a new test in `test_mainwindow` hung with no output -and was killed at the two-minute timeout. It had configured an account section -carrying only `sent=`, with no `maildir=`. - -**Cause (verified by attaching gdb to the hung process, not inferred).** - -``` -#7 QDialog::exec() -#9 MainWindow::showWarnings ... src/mainwindow.cpp:1670 -#10 MainWindow::MainWindow ... src/mainwindow.cpp:381 -``` - -`Config::load` handles the malformed account exactly as it should: it records -"Account 'one' has no maildir; ignoring it" and carries on -(`src/config.cpp:413-418`). `showWarnings()` then puts every collected problem -in a `QMessageBox::warning`, which is modal, and it is called from the -`MainWindow` CONSTRUCTOR. Under the offscreen platform nothing can dismiss it, -so the constructor never returns. - -**This is not a defect in the application.** The modal is deliberate and is -right for a person: a config problem should interrupt startup rather than -scroll past, and the code comment at `src/mainwindow.cpp:1661` explains which -problems qualify. A user sees the dialog and clicks OK. The defect is that a -TEST cannot, and the failure it produces is a silent hang rather than an error -naming the cause, which cost a debugging detour to identify. - -**Constraint on any fix: the modal must survive for real use.** Suppressing it -whenever `QTEST_MAIN` is linked would be the obvious move and is wrong, since -that is exactly the path `test_mainwindow` exercises and a suppressed dialog -means the startup warning ships untested. Two candidates, neither yet chosen: - -- A `MainWindow` flag, defaulting to showing the modal, that the tests set. It - makes the behaviour explicit and testable in both states. -- Collecting the problems and emitting them, with the modal raised by a caller - outside the constructor. Larger, and it separates "what is wrong" from "how - the user is told", which is the better shape if anything else ever needs the - list. - -**Related: every test that configures an account is one typo away from this.** -The suite has several, all of them currently well-formed. A malformed one does -not fail, it hangs, and a hang in CI reads as an infrastructure problem rather -than a test problem. - -**A second trap sits on top of the first and wasted as much time.** A hang -leaves the test binary running, and a later `ctest` then runs a STALE binary -while the source on disk has moved on, so the failure appears to persist after -it has been fixed and to vanish for reasons unconnected to the change. Kill any -surviving `test_mainwindow` and rebuild before concluding anything about a hang -here. - -**Size: S.** The diagnosis is the expensive part and it is already done. - ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/docs/superpowers/specs/2026-08-14-mainwindow-worker-fixture-design.md b/docs/superpowers/specs/2026-08-14-mainwindow-worker-fixture-design.md new file mode 100644 index 0000000..bb310d2 --- /dev/null +++ b/docs/superpowers/specs/2026-08-14-mainwindow-worker-fixture-design.md @@ -0,0 +1,145 @@ +# Giving `test_mainwindow` a real worker + +Resolves backlog item **36**, and produces a reproduction for item **66**. +Depends on item 84, which is already fixed: the constructor no longer raises a +modal, so a `MainWindow` built from a written config file can be constructed at +all. + +## What this is + +`test_mainwindow` gains the ability to build a `MainWindow` whose +`NotmuchWorker` is pointed at a throwaway notmuch database, so a test can select +a thread and let a real `threadLoaded` arrive. It is then used to write a +failing test for item 66. + +**The deliverable is a RED test for item 66.** Fixing that defect is not in +scope. See "Why the fix is excluded" below. + +## Why this is smaller than the backlog entry suggests + +The entry says the machinery exists and `test_mainwindow` does not use it, which +is true, and implies a hook has to be added to `MainWindow`, which is not. + +`MainWindow::wireWorker()` (`src/mainwindow.cpp:1440`) already constructs the +worker from `m_config.notmuchConfig()`, and the constructor already calls it. +`notmuch_config` is an ordinary config key read by `Config::load` +(`src/config.cpp:162`). So a test that writes a `qtmaildir.conf` carrying + +```ini +[general] +notmuch_config=/tmp/.../config +``` + +gets a `MainWindow` whose worker is already pointed at the fixture, through the +shipping code path, with no test-only setter and no `#ifdef`. Nothing in `src/` +changes. + +**Note the `[general]` trap** recorded in `CLAUDE.md`: QSettings treats a +section literally named `[general]` as its own fallback section and strips the +prefix, so the key is read as `notmuch_config`, not `general/notmuch_config`. +This is how that key went unnoticed as broken once already. Write the section as +above and read back with `Config::notmuchConfig()` to confirm before building +anything on it. + +## The helper + +Opt-in, never suite-wide. Roughly fifty existing cases construct a bare +`MainWindow` and must keep costing nothing; a suite-wide `initTestCase` would +make every one of them pay for a `notmuch new`, and shared mutable state between +cases is exactly how the `/proc/locks` bug (item 61) reached the whole suite. + +A test that wants a database calls a helper that: + +1. builds a `NotmuchFixture` (already exists, `tests/notmuchfixture.h`), +2. adds the messages that test needs and calls `index()`, +3. writes a `qtmaildir.conf` in the same `QTemporaryDir` with `notmuch_config` + pointing at the fixture's config, +4. loads it into a `Config` and returns both, so the caller constructs the + `MainWindow` and the fixture outlives it. + +**Lifetime is the trap here.** The fixture owns a `QTemporaryDir` which deletes +the tree in its destructor, and the worker holds the database open on another +thread. The fixture must outlive the `MainWindow`, so it cannot be a local in +the helper returned by value unless it is moved or heap-owned. Decide this when +writing it, and assert the database is readable after construction rather than +assuming. + +## Waiting for the worker + +The worker is on its own thread, so every assertion is asynchronous. +`QSignalSpy::wait()` against the specific signal, never a bare +`QTest::qWait(n)` with a guessed duration: a fixed sleep is a race that passes +on this machine and fails under load, and it also passes when the signal never +arrives at all, since nothing checks. + +Wait on `threadsReady` / `queryFinished` for a query, and on `threadLoaded` for +a selection. Assert the spy actually fired (`QVERIFY(spy.wait())`) before +asserting on what it delivered. + +## The item 66 reproduction + +Item 66: "selecting a thread root leaves the message pane blank until a reply +has been selected." The test: + +1. Fixture with one thread of at least two messages, the reply linked by a real + `In-Reply-To` header, since that is how `NotmuchFixture` builds a thread. +2. Run a query that returns it, wait for `queryFinished`. +3. Select the thread ROOT, the top-level row, without expanding it. +4. Wait for `threadLoaded`. +5. Assert the message pane is showing that thread. + +Assert on `MainWindow`'s own notion of what the pane holds, the accessor already +used elsewhere in `test_mainwindow` for "the pane is blanked", not on rendered +pixels. `CLAUDE.md` is explicit that rendering probes lie in repeatable ways and +that `viewport()->render()` returns blank images for several ordinary reasons; a +pixel assertion here would be measuring the probe, not the defect. + +**This test is expected to FAIL.** That is the deliverable. + +### If it passes + +A green test is a real finding, not a failure of the work. It means the defect +needs a condition the entry does not name, and the candidates in rough order of +likelihood are: it needs more than one message loaded before it manifests, it +needs the thread to be already expanded, it depends on the selection arriving +before a previous load completes, or it only appears against a large real +database and not a two-message one. + +In that case, record what was tried in the item 66 entry and stop. Do not widen +the test until it goes red: a test twisted until it fails proves nothing about +the defect the user actually sees. + +## Why the fix is excluded + +The user's decision, and the reason is the ordering. Item 66 has never been +isolated. Designing the fix in the same pass as the reproduction means designing +it while the diagnosis is still a hypothesis, which is where a wrong fix gets +locked in and then defended by the test written alongside it. A red test is a +complete, useful deliverable: it proves the defect is real, locates it, and +makes the fix verifiable when it comes. + +## Constraints + +- **Every existing `test_mainwindow` case must keep working unchanged.** The + helper is additive. +- **No test may read the real `/proc/locks`.** `init()` already points every + test at an empty lock table in its own `QTemporaryDir`, and no test restores + `"/proc/locks"` afterwards. That protection is suite-wide and this work must + not weaken it; `noTestCanSeeTheRealLockTable` fails if it is lost. +- **No personal data in fixtures.** Generated messages only, `example.org` + addresses, generic subjects. This was considered and declined explicitly on + 2026-08-04 and the repository is public. +- **A date fixture's weekday must match its date.** `Qt::RFC2822Date` validates + the two against each other, so `Thu, 14 Aug 2026` parses as INVALID because + that day is a Friday. Generate with `date -d <yyyy-mm-dd> +%A`, never from + memory. +- **A hung `test_mainwindow` leaves a stale binary** that a later `ctest` + re-runs, so a failure appears to persist after being fixed. Kill it and + rebuild before concluding anything about a hang. + +## Size + +**S–M.** The helper is small because the plumbing already exists. The +reproduction is the uncertain half: the first attempt may need several shapes +before it reproduces, and it may not reproduce at all, which is a legitimate +outcome rather than an overrun. |