From 9585674c9546079aa86f9943df39f700d8db331a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 10:00:31 +0200 Subject: test(mainwindow): stop the suite reading the real kernel lock table Item 61. An init() fixture gives every test its own empty lock table in a QTemporaryDir, so no test observes the machine's real sync state. The failure was never intermittent in the usual sense: 0 failures in 30 runs with no lock held, 30 in 30 with one held. It presented as three tests failing that never mention syncing, and cost three misdiagnoses. The three tests that already used the seam each restored "/proc/locks" when finished, which was itself the defect: it handed the real table to whichever test ran next, so one test opting in re-exposed all the others. Those restores are gone and cleanup() leaves the temporary path in place. noTestCanSeeTheRealLockTable guards the fixture, since a silent revert would go back to failing for reasons no assertion mentions. Verified with the lock deliberately held: 3 failures before, 119/119 after, full suite 19/19. Mutation-checked by disabling the fixture, where the guard fails first and a real test fails behind it. --- CLAUDE.md | 13 ++++ .../plans/2026-08-03-post-0.1.0-usability.md | 28 ++++++++- tests/test_mainwindow.cpp | 70 +++++++++++++++++++++- 3 files changed, 107 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d652bb6..152b9e7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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() -- cgit v1.2.3