diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index fef32888..c72fc2b7 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -3864,6 +3864,23 @@ ticket's transcript spells the route as `T r l c Esc`, which depends on `state().tiles` and walks to it, so the test pins *the route exists*, not a particular ordering the producer is free to change. +**The walks are bounded (improvise-ybum),** found in the adversarial review +of PR #256. `tile_to` and the column walk were `while` loops that pressed `l` +or `h` until an index was reached. A resolver that stopped the cursor short +never ended the loop: under the left-clamp fault a reviewer killed the binary +after 17 minutes, and in CI a hang costs the job timeout and names no test. +Both loops now go through `walk_to`. It presses at most `limit` times (the tile +count plus one, or the column count plus one), and it fails as soon as a press +leaves the index where it was, naming the stuck index and the target. + +- **Rival `left_clamp_one`** (`clamp(1, ..)` in `tile_cursor`): before the + change, the binary was still running at a 90-second `timeout` and was killed + (exit 124). After it, the test fails in 0.04s with "the tile cursor is stuck + at 1: `h` did not move it toward 0". +- **Rival `column_cap_one`** (`clamp(0, col_max.min(1))` in the grid move): + fails in 0.04s with "the column cursor is stuck at 1: `l` did not move it + toward 2", so the column walk is bounded too, not only the tile walk. + ### opening_an_editor_is_declined_off_the_grid_and_not_off_the_mode improvise-bd9o, and it exists because **the route test could not state the @@ -5495,3 +5512,42 @@ when the index does not move (gotcha 47). `SetTileCatIdx(3)` over three tiles. - The control matters: without the unclamped move first, a resolver that never moves at all would also pass the two clamped presses. + +### tab_at_the_rightmost_column_stays_there_on_both_ends + +improvise-qnry, found in the adversarial review of PR #256. Dropping the clamp +from `advanced`'s `Advance::Right` arm, so that it reads `Some((row, col + 1))`, +passed the whole workspace (1942 tests). `tab_in_an_editor_commits_and_lands_one_column_right` +sits on a centred cursor on purpose, so it cannot see the edge. Without the +clamp, Tab at the rightmost column of a pivot grid sends the selection one +column past the grid, and the editor opens on no cell. + +This is a lockstep case: the client is compared with the host's real +`CommitAndAdvance`, not with a hand-written expectation. The fixture is +`precise_host`, two rows by two columns, with the cursor at (1, 1). That is the +rightmost column at a nonzero row, so "the last column" is not "the first +cell". The test presses `i` then `Tab` and compares the cursor and the mode on +both ends after each key. Its preconditions assert that the grid is not in +records mode, where this corner declines instead, and that the editor is open +again at the end. + +- **Rival `right_unclamped`** (the ticket's fault): fails only this test across + the workspace (1942 passed, 1 failed). The client lands on `(1, 2)` with + `col_offset: 1`; the host stays on `(1, 1)`. +- improvise-3tt4's acceptance test (Tab at the rightmost column, comparing + buffers) would probably also see this fault, but it lives on PR #227's branch, + which is not in this base. This test compares the selection and does not + depend on #227. + +### a_locally_resolved_tile_cursor_agrees_with_the_host + +Extended for improvise-qnry. The test used to press `l` three times over +`tall_host`'s four tiles, starting at 0, so it never reached the upper clamp: +any rival that differed only at the last tile passed it. It now presses +`count + 1` times and asserts it ends on the last tile, so the final press is +checked against the host's real `MoveTileCursor` clamp. + +- **Rival `wraparound`** (`.rem_euclid(count)` in place of the clamp): fails + this test at press 3 (client 0, host 3), and also the unit test + `resolve::tests::the_tile_cursor_stops_on_the_last_tile`. Nothing else in + the workspace fails (1941 passed, 2 failed). diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 1ab86c87..1794d7fd 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -431,15 +431,44 @@ fn tile_to( .iter() .position(|t| t.category == category) .unwrap_or_else(|| panic!("the tile bar must carry {category}")); - while client.state().view.tile_cat_idx < at { - press(host, id, client, "l"); - } - while client.state().view.tile_cat_idx > at { - press(host, id, client, "h"); - } + let limit = client.state().tiles.tiles.len() + 1; + walk_to(host, id, client, "the tile cursor", at, limit, |c| { + c.state().view.tile_cat_idx + }); press(host, id, client, key); } +/// Press `l` or `h` until `read` gives `target`, failing on a stuck press or +/// after `limit` presses rather than hanging (improvise-ybum). Evidence file. +fn walk_to( + host: &mut Host, + id: improvise_command::ui::project::SessionId, + client: &mut Client, + what: &str, + target: usize, + limit: usize, + read: impl Fn(&Client) -> usize, +) { + for _ in 0..limit { + let now = read(client); + if now == target { + return; + } + let key = if now < target { "l" } else { "h" }; + press(host, id, client, key); + assert_ne!( + read(client), + now, + "{what} is stuck at {now}: `{key}` did not move it toward {target}" + ); + } + assert_eq!( + read(client), + target, + "{what} did not reach {target} in {limit} presses" + ); +} + /// `toggle-records-mode` is declined and the tile bar reaches records mode /// anyway (improvise-bd9o). Evidence file. #[test] @@ -472,9 +501,15 @@ fn the_tile_bar_route_into_records_mode_does_not_strand_the_user() { "the fixture must reach past the coordinate column" ); for col in 0..cols { - while client.state().view.cursor().selected.1 < col { - press(&mut host, id, &mut client, "l"); - } + walk_to( + &mut host, + id, + &mut client, + "the column cursor", + col, + cols + 1, + |c| c.state().view.cursor().selected.1, + ); let kind = format!( "{:?}", client.state().grid.cell(0, col).and_then(|c| c.id.clone()) diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index c825d2fe..5ceddccd 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -462,7 +462,8 @@ fn a_locally_resolved_tile_cursor_agrees_with_the_host() { "a one-tile fixture clamps immediately and proves nothing" ); - for press in 0..3 { + let count = client.tiles.tiles.len(); + for press in 0..=count { let event = KeyEvent::new(KeyCode::Char('l'), KeyModifiers::NONE); let KeyOutcome::Local(cmds) = resolve(&client, event) else { panic!("press {press}: the tile cursor must resolve locally"); @@ -476,7 +477,11 @@ fn a_locally_resolved_tile_cursor_agrees_with_the_host() { "press {press}: client and host disagree about the tile cursor" ); } - assert!(client.view.tile_cat_idx > 0, "the fixture moved nothing"); + assert_eq!( + client.view.tile_cat_idx, + count - 1, + "count + 1 presses must end clamped on the last tile (improvise-qnry)" + ); } // The editing path (improvise-bpq2's second half). @@ -671,6 +676,50 @@ fn a_locally_committed_edit_leaves_both_ends_in_the_same_place() { ); } +/// improvise-qnry: Tab at the rightmost column clamps there on both ends, as +/// `CommitAndAdvance` does. Evidence: `context/test-evidence/clients.md`. +#[test] +fn tab_at_the_rightmost_column_stays_there_on_both_ends() { + let window = (2, 2); + let mut app = precise_host(improvise_protocol::CellValue::Number(4.0)); + let mut client = client_ready_to_edit(&app, window); + let extent = client.grid.extent; + assert!( + !client.grid.records, + "records mode declines this corner instead" + ); + assert_eq!( + client.view.cursor().selected, + (1, extent.cols - 1), + "the cursor must start on the rightmost column at a nonzero row" + ); + + for key in ["i", "Tab"] { + let event = BrowserKey::new(key).to_key_event().unwrap(); + let KeyOutcome::Local(cmds) = resolve(&client, event) else { + panic!("{key}: the client must answer this key"); + }; + for cmd in &cmds { + client.fold(cmd); + } + reduce_key(&mut app, event); + assert_eq!( + client.view.cursor(), + app.view_state.cursor(), + "{key}: client and host disagree about the cursor" + ); + assert_eq!( + format!("{:?}", client.view.mode), + format!("{:?}", app.view_state.mode), + "{key}: client and host disagree about the mode" + ); + } + assert!( + app.view_state.mode.is_editing(), + "Tab must have reopened the editor, or the walk stopped short" + ); +} + // The second tranche (improvise-e24v). /// A document with two categories, two views and a value, so every widget the