test: the worker's dirty-marker test could not see the widget vanishing
Re-review finding, low severity and correct. The test asserted only `!document.dirty`, and `DocumentCache::default()` is already clean — so it could not tell "told it cleared" from "never told anything", and passed under an injection that made `document_update` always return `None`. It is the test that names the browser deployment, which is the one place nothing ever sends ClientFrame::Save, so it is the only guard on that path. It now records every marker the client is told and asserts `[true, false]` for the edit. The bootstrap seeds its own clean marker first, so the sequence is cleared after the handshake and the assertion is about the edit rather than about the connect. Verified failing under both injections: dropping the worker's re-projection gives `[true]`, and stopping the widget being projected at all gives `[]`. Declined, with the reason: a test for `GridLayout::record_cell_key` returning a key for a `RecordCol::Category` column. The injection the reviewer describes would make a coordinate column project `Derived` where a formula owns that coordinate — and the reviewer's own harder case established that a formula targeting a regular category SHOULD project `Derived` there, in both modes. So the injected behaviour is not clearly a fault, the exhaustive match already forces a decision on any new `RecordCol` variant, and a test would be pinning an answer nobody has argued for. Recorded as a known gap rather than guarded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2366,3 +2366,11 @@ Fault injected: dropping the re-projection after `persist()`. The client's
|
||||
`DocumentCache` stays dirty. `MainThread` grew a `document` mirror and a
|
||||
projection dispatch for it, rather than a bespoke reach into the worker — the
|
||||
point is what a client folding the real stream ends up holding.
|
||||
|
||||
**A first version asserted only `!document.dirty`, and `DocumentCache::default()`
|
||||
is already clean** — so it could not tell "told it cleared" from "never told
|
||||
anything", and passed under an injection that stopped projecting the widget
|
||||
entirely. Found in re-review. It now records **every** marker the client is
|
||||
told and asserts `[true, false]` for the edit, with the bootstrap's own clean
|
||||
marker cleared first so the sequence is about the edit rather than the
|
||||
handshake. The stronger form fails under both injections.
|
||||
|
||||
@@ -47,6 +47,8 @@ struct MainThread {
|
||||
worker: Worker,
|
||||
grid: GridCache,
|
||||
document: improvise_protocol::DocumentCache,
|
||||
/// Every dirty marker this client has been told, in order.
|
||||
dirty_marks: Vec<bool>,
|
||||
}
|
||||
|
||||
impl MainThread {
|
||||
@@ -63,6 +65,7 @@ impl MainThread {
|
||||
worker: Worker::new(app),
|
||||
grid: GridCache::default(),
|
||||
document: improvise_protocol::DocumentCache::default(),
|
||||
dirty_marks: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -114,7 +117,10 @@ impl MainThread {
|
||||
fn apply(&mut self, projection: &Projection) {
|
||||
match projection {
|
||||
Projection::Grid(update) => self.grid.apply(update),
|
||||
Projection::Document(update) => self.document.apply(update),
|
||||
Projection::Document(update) => {
|
||||
self.document.apply(update);
|
||||
self.dirty_marks.push(update.dirty);
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
@@ -212,6 +218,7 @@ fn a_reload_over_the_same_origin_reopens_what_was_saved() {
|
||||
worker: Worker::new(app),
|
||||
grid: GridCache::default(),
|
||||
document: improvise_protocol::DocumentCache::default(),
|
||||
dirty_marks: Vec::new(),
|
||||
};
|
||||
reloaded.post(&ClientFrame::Viewport { rows: 10, cols: 4 });
|
||||
assert_eq!(
|
||||
@@ -363,6 +370,9 @@ fn the_automatic_persist_tells_the_client_the_document_is_clean() {
|
||||
let mut main = MainThread::boot(storage, &path);
|
||||
main.post(&ClientFrame::Viewport { rows: 10, cols: 4 });
|
||||
|
||||
// The bootstrap already seeded a clean marker; what this test is about is
|
||||
// what the EDIT and its persist then say.
|
||||
main.dirty_marks.clear();
|
||||
let cell = main.cell(0, 0);
|
||||
main.post(&main.caught_up(ModelCommand::SetCell {
|
||||
cell,
|
||||
@@ -373,6 +383,13 @@ fn the_automatic_persist_tells_the_client_the_document_is_clean() {
|
||||
!main.worker.app().model_state.dirty,
|
||||
"the worker must really have persisted, or this proves nothing"
|
||||
);
|
||||
// `DocumentCache::default()` is already clean, so the clean assertion alone
|
||||
// cannot tell "told it cleared" from "never told anything".
|
||||
assert_eq!(
|
||||
main.dirty_marks,
|
||||
vec![true, false],
|
||||
"the client must see the marker raised and then cleared, in one batch"
|
||||
);
|
||||
assert!(
|
||||
!main.document.dirty,
|
||||
"the client must be told the document is clean — it is showing [+] otherwise"
|
||||
|
||||
Reference in New Issue
Block a user