diff options
| -rw-r--r-- | CLAUDE.md | 13 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 28 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 70 |
3 files changed, 107 insertions, 4 deletions
@@ -273,6 +273,19 @@ letting QCompleter overwrite the field. This has been hit twice, in either class. A test that uses `setText()` passes against the bug, since `setText` does not drive a completer at all: the keys must be typed. +**No test may read the real `/proc/locks`, and restoring it after a test is a +BUG, not cleanup.** `TestMainWindow::init()` points every test at an empty lock +table in its own `QTemporaryDir`. Without that the suite observes the machine's +real sync state, so a `mailsync.sh` run makes `SyncMonitor` report a sync in +progress and tests that never mention syncing fail: measured 0 failures in 30 +runs with no lock held, 30 in 30 with one held, and it caused three separate +misdiagnoses (item 61). Reproduce with `flock /tmp/mbsync.lock -c 'sleep 60'` in +one shell and the suite in another. The three tests that observe a sync write +their own table content; none of them restores `"/proc/locks"` at the end any +more, because doing so handed the real table to the next test and re-exposed the +whole suite. `noTestCanSeeTheRealLockTable` fails if that protection is ever +lost. + **`QItemSelectionModel::currentRowChanged` is emitted BEFORE the selection model is updated.** A handler on it reading `selectedRows()` sees the *previous* selection, not the one the user just made. Verified against Qt 6.11. This produced two separate faults in one 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 656773b..26464ba 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 @@ -117,7 +117,7 @@ taking that too literally. | 58 | `message_zoom` documents a 0.5 to 3.0 range and enforces none of it | correctness | XS | **done** | | 59 | Archive and Mark all read shipped with the same icon | presentation | XS | **done** | | 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | **done**; already fixed by 5487d58, see below | -| 61 | `test_mainwindow` fails intermittently, about 1 run in 20 | testing | S | open; cause established 2026-08-11 (the user's cron sync holds the mbsync lock), fix is item 38's seam applied suite-wide | +| 61 | `test_mainwindow` fails intermittently, about 1 run in 20 | testing | S | **done** 2026-08-13; an `init()` fixture points every test at its own lock table | | 62 | No config option for the date format on a card | presentation | XS | **done** 2026-08-11 | | 63 | No way to see sent mail, and no filter for it | workflow | M | **done** 2026-08-11; see `specs/2026-08-11-sent-mail-design.md` | | 64 | The Sync button carries a mailbox icon, not a refresh one | presentation | XS | **done** 2026-08-11 | @@ -4125,6 +4125,32 @@ initially mistaken for a regression that change had introduced. **Size: S**, most of it in reproducing reliably rather than in the fix. +### Outcome (done 2026-08-13) + +`TestMainWindow::init()` builds a `QTemporaryDir` per test and points +`MainWindow::setLocksPathForTesting` at an empty file inside it, so no test +reads the real `/proc/locks`. An empty table is the honest representation of +"no sync is running"; the three tests that want to observe a sync write their +own content, as they already did. + +**The three existing users of the seam each restored `"/proc/locks"` when they +finished, and that restoration was itself a defect**: it handed the real table +back to whichever test ran next, so one test opting in re-exposed every test +after it. All three restores are removed, and `cleanup()` deliberately leaves +the path pointing at the temporary file. + +`noTestCanSeeTheRealLockTable` guards the fixture, since a suite that silently +reverts to the real table would go back to failing for reasons no assertion +mentions. + +Verified rather than assumed, using the reproduction above. With +`flock /tmp/mbsync.lock -c 'sleep 30'` held: 3 failures before +(`aRefreshDoesNotStampOverASelectionMessage`, +`anActionOnAMessageRowTagsThatMessageNotTheThread`, +`aSuccessfulCronSyncDrainsTheEditedAccounts`), 119/119 after, and the full suite +19/19 with the lock held. Mutation-checked by disabling the fixture: the guard +fails first with its diagnostic, and a real test fails behind it. + ## 62. No config option for the date format on a card **Observed (user, from the notes):** "option in config file for date format". diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 18fe8c1..4c1c5d9 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -66,6 +66,9 @@ class TestMainWindow : public QObject { Q_OBJECT private slots: + void init(); + void cleanup(); + void noTestCanSeeTheRealLockTable(); void everyKnownActionIsRegistered(); void everyRegisteredActionIsKnown(); void everyActionHasAShortcut(); @@ -184,8 +187,66 @@ private slots: void placeholderCountsDropAnUncountableQuery(); void flatModeDoesNotSurviveTheNextQuery(); void noTwoActionsShareAnIcon(); + +private: + /// Owns the throwaway lock table init() points every test at. A pointer + /// rather than a value because it is rebuilt per test, and QTemporaryDir + /// removes its directory when destroyed. + QTemporaryDir *m_lockDir = nullptr; }; +/// Item 61. Points every test at a lock table it owns, before every test. +/// +/// Without this the suite reads the real `/proc/locks`, so a `mailsync.sh` run +/// on the developer's machine makes `SyncMonitor` report a sync in progress and +/// tests that never mention syncing fail. It is not a rare race: measured 0 +/// failures in 30 runs with no lock held and 30 in 30 with one held, and it +/// cost three separate misdiagnoses before the cause was found. Reproduce with +/// `flock /tmp/mbsync.lock -c 'sleep 60'` in one shell and the suite in +/// another. +/// +/// An EMPTY file rather than a fabricated table: `SyncMonitor` reads it and +/// finds no entry, which is exactly "no sync is running". A test that wants to +/// see a sync writes its own content, which three already do. +/// +/// This also replaces the pattern those three used of restoring +/// `"/proc/locks"` when finished. That restoration was itself a defect: it +/// handed the real table back to whichever test ran next, so one test opting +/// in re-exposed all the others. +void TestMainWindow::init() +{ + m_lockDir = new QTemporaryDir; + QVERIFY(m_lockDir->isValid()); + + const QString locks = m_lockDir->filePath(QStringLiteral("locks")); + QFile file(locks); + QVERIFY(file.open(QIODevice::WriteOnly)); + file.close(); + + MainWindow::setLocksPathForTesting(locks); +} + +void TestMainWindow::cleanup() +{ + // Left pointing at the temporary path deliberately. Restoring + // "/proc/locks" here would re-expose the next test between cleanup() and + // its own init(), which is the trap this fixture exists to close. + delete m_lockDir; + m_lockDir = nullptr; +} + +void TestMainWindow::noTestCanSeeTheRealLockTable() +{ + // The guard for the fixture itself. A test that asserts on sync state + // proves nothing if the path silently reverts to /proc/locks, and this + // fails the moment init() stops being applied or someone restores the real + // table at the end of a test. + QVERIFY2(MainWindow::locksPath() != QStringLiteral("/proc/locks"), + "the suite is reading the real kernel lock table; a sync running " + "on this machine will fail unrelated tests (item 61)"); + QVERIFY(MainWindow::locksPath().startsWith(QDir::tempPath())); +} + void TestMainWindow::everyKnownActionIsRegistered() { // KeyMap::knownActions() is what loadOverrides() validates config bindings @@ -3546,7 +3607,8 @@ void TestMainWindow::theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock() QVERIFY2(action->isEnabled(), "the sync action was not re-enabled after the background sync"); - MainWindow::setLocksPathForTesting(QStringLiteral("/proc/locks")); + // No restore to "/proc/locks": init() points every test at its own + // table, and handing the real one back would re-expose the next test. } // Item 71. A confirmed tag edit arms a debounce that syncs it out, so an edit @@ -3742,7 +3804,8 @@ void TestMainWindow::autoSyncSkipsWhileABackgroundSyncIsRunning() QVERIFY2(!label->isHidden(), "a skipped automatic sync cleared the pending indicator"); - MainWindow::setLocksPathForTesting(QStringLiteral("/proc/locks")); + // No restore to "/proc/locks": init() points every test at its own + // table, and handing the real one back would re-expose the next test. } void TestMainWindow::aSuccessfulSyncRefreshesRatherThanRerunningTheQuery() @@ -3909,7 +3972,8 @@ void TestMainWindow::theStatusBarFollowsTheSyncPhase() QVERIFY2(!s.contains(QStringLiteral("status=")), qPrintable(s)); } - MainWindow::setLocksPathForTesting(QStringLiteral("/proc/locks")); + // No restore to "/proc/locks": init() points every test at its own + // table, and handing the real one back would re-expose the next test. } void TestMainWindow::anUnobservableLockTableLeavesTheSyncButtonUsable() |