summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDanilo M. <danix@danix.xyz>2026-08-06 19:32:22 +0200
committerDanilo M. <danix@danix.xyz>2026-08-06 19:32:22 +0200
commiteef7f6cc1b2f845d2ca1eba53336f64597c49e62 (patch)
tree61a2168834a623c1afa5e898996f8ae036e44d36
parentddb4ac26b7fb553c9349401dda4510636c67cc9b (diff)
downloadqtmaildir-eef7f6cc1b2f845d2ca1eba53336f64597c49e62.tar.gz
qtmaildir-eef7f6cc1b2f845d2ca1eba53336f64597c49e62.zip
test: stop two tests depending on the machine they run on
Both are the same class of defect: a test that reads real machine state and so passes or fails on circumstance rather than on the code under test. Item 38. Every MainWindow a test builds constructed its SyncMonitor on the live /proc/locks, so a window observed the machine's actual sync state and the sync-button assertion failed whenever the user's cron sync happened to be running. Cron fires every ten minutes and a run lasts ~35s, which is roughly 6% of runs, and it read as flakiness. SyncMonitor already took an injectable locks path for exactly this; MainWindow did not expose it. It does now, as a test seam rather than a config key: /proc/locks is not something a user would set, and a wrong value silently disables background sync detection instead of failing loudly. The monitor is still constructed and started, per the item's own constraint. Only the table it reads is redirected, to an empty file in the test's own temporary directory. Item 46. uiStateSurvivesARestart asserted a 940px width, and the offscreen platform reports an 800x800 screen. restoreGeometry() clamps to the available area, so the width came back as 798 while the 620 height, which fits, restored untouched. That asymmetry was the tell that persistence was fine and the test was wrong. The size is now 640x560 and carries no meaning beyond differing from the default. Verified by reproducing the original conditions rather than by waiting for them: the suite run under flock -n /tmp/mbsync.lock fails item 38's assertion with the seam bypassed and passes with it in place, and item 46 now passes under offscreen where it failed. One dud mutation is recorded in the backlog, writing an unparseable line into the injected lock table does not fail the test, because lockHeldIn() correctly finds no lock in it. Suite: 15/15 offscreen with the lock held, and green on Wayland except the pre-existing querycompleter screenshot flake, which fails to grab under Wayland and passes offscreen.
-rw-r--r--docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md38
-rw-r--r--src/mainwindow.cpp17
-rw-r--r--src/mainwindow.h12
-rw-r--r--tests/test_mainwindow.cpp22
4 files changed, 85 insertions, 4 deletions
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 0186bfa..dd6a8ca 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
@@ -85,7 +85,7 @@ taking that too literally.
| 35 | No refresh of the thread list after a sync | workflow | M | open |
| 36 | `test_mainwindow` cannot reach the worker | testing | S | open, on demand |
| 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 | open |
+| 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 | open |
| 40 | No live filter over the current view | workflow | M | open |
| 41 | A message whose HTML body carries a `Content-Id` renders blank | correctness | S | open |
@@ -93,7 +93,7 @@ taking that too literally.
| 43 | No "Mark all read" for the current view | workflow | S | open |
| 44 | No way to manage the filters applied at sync time | workflow | ? | open, unspecified |
| 45 | Two Sync buttons on the main window | discoverability | XS | open |
-| 46 | `uiStateSurvivesARestart` fails under the offscreen platform | testing | XS | open |
+| 46 | `uiStateSurvivesARestart` fails under the offscreen platform | testing | XS | **done** |
Sizes are rough: XS under an hour, S a sitting, M a session.
@@ -1917,6 +1917,30 @@ are unaffected either way; it is only the construction-time state that leaks in.
construction-time state IS the behaviour under test for this case, and a window
that never polls would pass the assertion for the wrong reason.
+### Outcome (done)
+
+`MainWindow::setLocksPathForTesting()` / `locksPath()` give the window the seam
+`SyncMonitor` already had, and the test points it at an empty file in its own
+`QTemporaryDir` so construction observes no sync. The constraint above is
+respected: the monitor is still constructed and still started, it simply reads a
+lock table the test controls.
+
+**A test seam, deliberately not a config key.** `/proc/locks` is not something a
+user would ever set, and a wrong value fails silently by disabling background
+sync detection rather than loudly. A `[general]` key was considered and rejected
+for that reason.
+
+**The override is process-wide and is reset at the end of the test**, since the
+`QTemporaryDir` holding the file is destroyed with it; leaving it set would
+point every later window at a path that no longer exists.
+
+**Verified by reproducing the original failure rather than waiting for cron.**
+Running the suite under `flock -n /tmp/mbsync.lock` fails the assertion exactly
+as reported when the seam is bypassed, and passes with it in place. The first
+mutation attempted was a dud worth recording: writing `MUTANT` into the injected
+lock table does not fail the test, because it is not a parseable `/proc/locks`
+line and `lockHeldIn()` correctly finds no lock in it.
+
## 39. Thread list cannot be sorted by clicking a column header
**Observed (user, 2026-08-05):** "left pane columns order by clicking on the
@@ -2207,6 +2231,16 @@ the property under test (the size that went in comes back out) is exact.
the machine they run on, and both were found by running the suite in a context
its author had not tried rather than by reading it.
+### Outcome (done)
+
+The asserted size is now 640x560, which fits the offscreen platform's 800x800
+screen. Nothing in `MainWindow` changed: the persistence was never broken, only
+the test's choice of a window wider than the smallest screen it runs against.
+
+Verified both ways round, since this one passed on Wayland throughout: 45 of 45
+under offscreen where it previously failed, and still green on the real
+platform.
+
## Deferred, unsized, or split out
Items noted while triaging but not part of the original list. Same numbering
diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index 7c7b801..75549d3 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -83,6 +83,21 @@ QString MainWindow::uiStatePath()
return base + QStringLiteral("/qtmaildir/uistate.conf");
}
+namespace {
+/// Overridden only by setLocksPathForTesting(); "/proc/locks" in every real run.
+QString g_locksPath = QStringLiteral("/proc/locks");
+} // namespace
+
+void MainWindow::setLocksPathForTesting(const QString &path)
+{
+ g_locksPath = path;
+}
+
+QString MainWindow::locksPath()
+{
+ return g_locksPath;
+}
+
void MainWindow::restoreUiState()
{
QSettings state(uiStatePath(), QSettings::IniFormat);
@@ -465,7 +480,7 @@ void MainWindow::buildUi()
// every ten minutes, so mail arrives and tags change while the window sits
// idle, and until now nothing here noticed.
m_syncMonitor = new SyncMonitor(SyncMonitor::defaultLockPath(),
- QStringLiteral("/proc/locks"), this);
+ locksPath(), this);
connect(m_syncMonitor, &SyncMonitor::stateChanged,
this, &MainWindow::onExternalSyncStateChanged);
m_syncMonitor->start();
diff --git a/src/mainwindow.h b/src/mainwindow.h
index 22c8187..96b27f4 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -104,6 +104,18 @@ public:
/// preserve comments or key order).
static QString uiStatePath();
+ /// Kernel lock table every MainWindow's SyncMonitor watches, "/proc/locks"
+ /// unless a test overrides it.
+ ///
+ /// A test seam, deliberately NOT a config key: /proc/locks is not something
+ /// a user would ever set, and a wrong value silently disables background
+ /// sync detection rather than failing loudly. Without this every window a
+ /// test builds observes the machine's real sync state, so a test asserting
+ /// on the sync button fails whenever the user's cron sync happens to be
+ /// running (item 38).
+ static void setLocksPathForTesting(const QString &path);
+ static QString locksPath();
+
protected:
void closeEvent(QCloseEvent *event) override;
diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index c409d1a..6a4f903 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -233,7 +233,11 @@ void TestMainWindow::uiStateSurvivesARestart()
QStandardPaths::setTestModeEnabled(true);
QFile::remove(MainWindow::uiStatePath());
- const QSize resized(940, 620);
+ // Must fit the smallest screen this ever runs against: the offscreen
+ // platform reports 800x800, and restoreGeometry() clamps to the available
+ // area, so a 940px width came back as 798 and failed only under offscreen.
+ // The number carries no meaning beyond differing from the default size.
+ const QSize resized(640, 560);
{
const Config config;
MainWindow window(config);
@@ -995,6 +999,17 @@ void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock()
s.setValue(QStringLiteral("sync/command"), QStringLiteral("/bin/true"));
}
+ // An empty lock table, so construction observes no sync. Against the real
+ // /proc/locks this assertion fails whenever the user's cron sync happens to
+ // be running: cron fires every ten minutes and a run lasts ~35s, so roughly
+ // 6% of runs landed inside one and the failure looked like flakiness.
+ const QString locks = dir.filePath(QStringLiteral("locks"));
+ {
+ QFile f(locks);
+ QVERIFY(f.open(QIODevice::WriteOnly));
+ }
+ MainWindow::setLocksPathForTesting(locks);
+
Config config;
config.load(conf);
MainWindow window(config);
@@ -1014,6 +1029,11 @@ void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock()
SyncMonitor::State::Idle));
QVERIFY2(button->isEnabled(),
"the sync button was not re-enabled after the background sync");
+
+ // The override is process-wide, and QTemporaryDir takes the file with it at
+ // the end of this scope: leaving it set would point every later window at a
+ // path that no longer exists.
+ MainWindow::setLocksPathForTesting(QStringLiteral("/proc/locks"));
}
void TestMainWindow::anUnobservableLockTableLeavesTheSyncButtonUsable()