diff --git a/context/code/command.md b/context/code/command.md index 9c71aec1..e713576d 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -592,10 +592,23 @@ the client was told nothing at all. key-path batch assumes the failed one happened. `refuse_and_stop` does it for `LoadModel` and every `ImportJsonHeadless` arm, so `load x . q` and `import-json x . q` do not quit having loaded nothing. `AddCategory` keeps its - own inline abort (gotcha 21). The item, formula and view effects only refuse: - the frame is rolled back by the refusal alone. On the key path, a failed - `AddItem`, `AddFormula` or view edit no longer stops the rest of its batch, - which is what it did before bok4. + own inline abort (gotcha 21). The item and view effects, and `AddFormula`'s + missing-category arm, only refuse: the frame is rolled back by the refusal + alone. On the key path, a failed `AddItem` or view edit no longer stops the + rest of its batch, which is what it did before bok4. +- **`AddFormula`'s parse-error arm also stops (improvise-v4hg).** It is the one + item-or-formula failure a key-path batch was found to depend on. Enter in + `FormulaEdit` is `[SetStatus, AddFormula, ChangeMode(FormulaPanel), + ClearBuffer("formula")]`, and the last two assume the commit worked. Without + the stop, `Total = (` was reported and then the editor closed and the typed + text was cleared. With it, the user stays in `FormulaEdit` with the text, and + the error is drawn beside the prompt. `fit-measure` emits `AddFormula` last, so + there is nothing after it to stop. The missing-category arm is reachable only + from a wire frame or a `:` line naming another category, and nothing in either + depends on it, so it does not stop. PR #181's review searched for an + `AddItem` case the same way and found none, so `AddItem` does not stop either. + An abort is added where a later effect depends on the failed one, never for + symmetry. In a client's frame, any refusal becomes `Rejected(MutationRefused)`. The frame is undone whole, and the client is told, which is the channel the narrated @@ -615,8 +628,17 @@ Consequences: refusal, not by `status_msg`. The frame's rollback also rolls the host's status line back. - **Every refusal site has a test that fails when it is reverted** to a - status-only write (improvise-9fpl). Most are one table, - `every_failing_model_command_refuses_its_whole_frame`. That matters because + status-only write (improvise-9fpl, improvise-0v8l). Two tables carry most of + them. `every_failing_model_command_refuses_its_whole_frame` covers the wire. + `every_refusal_site_refuses_its_failure_and_only_its_failure` covers the key + path and the `:` line, which is what the headless exit code reads. The wire + table cannot see the refusal half of a `refuse_and_stop` site, because + `Host::mutating_frame` rejects on `refused || aborted`, and the abort alone + suffices. So each `refuse_and_stop` site needs a case in both tables: + the key-path table for its refusal, and + `a_failed_load_or_import_does_not_run_the_quit_after_it` for its abort + (improvise-514o). Each key-path case has a succeeding twin, so a site + that refused unconditionally would fail too. That matters because these are exactly the lines a merge conflict could silently drop. - **Not covered:** a no-op *inside* a view that does resolve. Examples are `SetAxis` on a category the view does not have, `HideItem` of an absent @@ -3357,18 +3379,41 @@ one run in eight (measured: 46 of 400). So the arguments now include **every item of every regular category as a one-coordinate drill**, read off the parsed document. A one-coordinate drill spans every other category whole, so it matches the whole slice of records under that item, not one record picked by name. In -the demo that is four records and eight cells for each of the eight drills. The -test -also asserts that the widest candidate it actually passes matches at least four -records. That guard catches a demo with too little data, and it catches the -derived drills being dropped from the argument list. It does **not** catch the -comparison losing its wide drills some other way, because it measures what the -argument lists match, not what the built commands ordered (improvise-b9f2). The -review showed this with a rival: a `drill-into-cell` parser that refuses fewer -than two arguments sends every one-coordinate drill to the one-record cursor -fallback. The guard still reports four records, and with the store's sort -removed the law passed 38 of 300 runs. With the store's sort removed and the -parser intact, the law fails 400 of 400 runs. +the demo that is four records and eight cells for each of the eight drills. With +the store's sort removed, the law fails 400 of 400 runs. + +**The width guard measures what the comparison compared (improvise-b9f2, +improvise-btyw).** The test asserts that some `drill-into-cell` the comparison +actually built and compared froze at least four records. For each one it applies +those very effects to one scratch `App` and counts the records in its +`drill_state`. It counts them only when their `Debug` text, which carries their +order, appears in the text the comparison compared. The comparison reads `Debug` +while the scratch `App` reads the applied state, and those are two +representations. The review showed they can disagree: give `StartDrill` a +`Debug` that prints only a row count, and the frozen records are still four +while the compared text no longer holds their order. With the store's sort +removed the law then passed 300 of 300 runs, with the guard green. Requiring +the ordered records in the compared text ties the guard to what the comparison +could actually see. A first +version measured what the *argument lists* match in the store instead. That was +a parallel derivation, and the review beat it with a `drill-into-cell` parser +that refuses fewer than two arguments. Every one-coordinate drill then went to +the one-record cursor fallback, the argument lists still matched four records, +and with the store's sort removed the law passed 38 of 300 runs. + +Counting frozen records is not enough by itself. A drill over no coordinates +(any candidate with no `Cat/Item` argument, such as `["1", "1"]`) freezes +**every** record, and it reads them through `iter_cells` in insertion order, +never through the narrowing index where gotcha 18 lives. Under the parser rival +that whole-store drill still froze 16 records, and a guard counting it stayed +silent. So only a drill that **narrowed** the store counts: one that froze fewer +records than the store holds. With the parser rival in place, the guard fires on +every run ("froze 1 records"), and it fires on every run under the `Debug` rival +too ("froze 0 records"). + +What the guard still cannot see is a narrowing drill whose records are frozen in +an order that does not come from the store. It checks that the law had a wide +drill to compare, not why the drill's order would differ. **What it cannot see.** Only commands and arguments the fixture reaches. A command whose nondeterminism needs some other state, such as records mode, a diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index d21d9fb0..2d306a36 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5316,6 +5316,81 @@ before `refuse_and_stop` was applied to its arms. For `load`, the injected fault is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test then fails with "`load absent.improv . q` quit having loaded nothing". +#### improvise-514o: every `refuse_and_stop` arm + +The case list above covered `load` and the JSON cannot-read arm only. PR #181's +review reverted each `refuse_and_stop` to `app.refuse` one at a time, which +keeps the refusal and drops the abort. Five `ImportJsonHeadless` arms failed +nothing: CSV cannot-read, CSV parse, JSON parse, array path and build. The wire +table `every_failing_model_command_refuses_its_whole_frame` cannot see the +abort, because `Host::mutating_frame` rejects on `refused || aborted`, and the +refusal alone suffices. The list now has one case per arm, using the fixtures +the wire table uses, which now come from `ui::host::import_fixture_storage`. +Each case also asserts the arm's own status text, so a fixture that failed in a +different arm cannot stand in for the one it names. The same scripted sweep, +rerun after the change, fails this test at all seven sites. Before, it failed +at two. + +### ui::app::tests::a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text + +improvise-v4hg. In `FormulaEdit`, Enter on `Total = (` reported the parse error +and then did what a successful commit does: `CommitBuffer`'s +`ChangeMode(FormulaPanel)` closed the editor and the keymap's welded +`ClearBuffer("formula")` cleared the text. The user saw the error with their +formula gone. Driven through `App::handle_key` from `FormulaPanel`: `a`, the +characters, then Enter. The test failed first with "got FormulaPanel". The fix is +`refuse_and_stop` on the parse-error arm, and a `Binding::Sequence` is one +effect vec, so the stop also covers the `ClearBuffer` step. + +The mode assertion alone would be vacuous against a fix that stops the mode +change but still loses the text. That rival (clear the `formula` buffer, then +`refuse_and_stop`) was run and fails "and must keep what they typed" with +`left: None`. The control, `Total = 1`, asserts the ordinary path still stores +the formula and closes the editor, so a stop on every arm would fail it. After +the fix, the abort sweep (`refuse_and_stop` back to `app.refuse`) fails this +test at that site. + +### ui::app::tests::every_refusal_site_refuses_its_failure_and_only_its_failure + +improvise-0v8l. PR #185 split refusals (`App::refuse`, which the headless exit +code reads) from reports (`set_status`), and most of the refusal sites were +unpinned. The ticket counted 23 of 27. At gh/main `608ba6a` (after #181's wire +table), a script (`/tmp/refuse-fu/sweep.py`, not committed) reverted each of +the **32** production refusal sites in `ui/effect.rs` and `command/cmd/*.rs` +alone to a status-only write, and ran `cargo test --workspace --no-fail-fast`. +For `effect::refuse(x)` the revert is `effect::set_status(x)`. For +`app.refuse(x)` it is `app.view_state.status_msg = x.into()`. For +`refuse_and_stop` it is the status write with the abort kept. **13 of 32 failed +a test. 19 survived:** `AddCategory` at the limit, `apply_coordinate_rename`, +both `StartImportWizard` arms, the five `ImportJsonHeadless` arms the wire +table cannot see (the abort alone rejects the frame), `rename_record_coordinate`, +`CommitExport`, both `ViewNavigate` arms, both `AddRecordRow` arms, both +`EditOrDrill` arms, `SearchNavigate` and `ExecuteCommand`. + +This table has one case per survivor. Each case drives the failure through a +`:` line (`run_line`, which does what `improvise cmd` does, one batch per +segment) or through Enter in the site's own minibuffer mode. It asserts that the +batch refused and that the status names the site's own reason. Each case also +has a **succeeding twin** that must not refuse, so a site that refused +unconditionally fails too. That rival (`AddCategory` refusing on `Ok` as well) +was run and fails "its succeeding twin was refused". Rerun after the change, +the sweep catches **32 of 32**. The abort half, 10 sites before v4hg's eleventh, +went from 5 caught to 11 of 11. + +Three fixture choices. The second was found when the first run failed: the input meant to be refused was accepted. + +- **`AddCategory` past the limit** needs eleven categories and a two-segment + line. The first `add-category` fills the twelfth slot and the second is + refused, so the succeeding twin is the same fixture with one segment. +- **`add-record-row` onto an existing row** must page the category on the + view the client is actually on. Records mode switches to `_Records`, so + writing `Workbook::active_view_mut` (the document's default) changed a view + nobody was looking at, and the case "succeeded". It goes through + `App::view_mut_of(&ViewTarget::Active)` (gotcha 29). +- **The drill rename** edits record 1, not record 0, following the + cursor-at-index-0 rule in the wave agent rules. Nothing failed here: this is + a precaution, not a finding. + ### ui::effect::tests::a_drill_rows_measure_edits_follow_its_rename improvise-uo2p. `ApplyAndClearDrill` walked `pending_edits`, a `HashMap`, and @@ -5434,14 +5509,41 @@ rivals it catches: still pin the store directly. - **The derived drills dropped from the argument list** (the `all.extend(one_coordinate_drills(model))` line commented out): the test - fails before comparing anything, with "the widest drill orders 1 records". - That is the only way of losing the wide drills the guard can see. It measures - what the argument lists match, not what the built commands ordered, so it can - pass while the comparison has lost its wide drills (improvise-b9f2). Review - rival: `drill-into-cell`'s parser changed to refuse fewer than two arguments, - which sends every one-coordinate drill to the one-record cursor fallback. The - guard stays silent and, with gotcha 18 re-injected, the law passed 38 of 300 - runs. + fails with "the widest narrowing drill compared froze 1 records". +- **`drill-into-cell`'s parser changed to refuse fewer than two arguments** + (improvise-b9f2): every one-coordinate drill then goes to the one-record cursor + fallback. The first width guard measured what the argument lists match, so it + stayed silent. With gotcha 18 re-injected the law passed 38 of 300 runs in + review, and 31 of 300 here at `cfcc9bff`. The guard now applies the compared + drills' own effects to a scratch `App` and counts the records they froze. + Against this rival it fails on every run with "froze 1 records". + - A first version of the fix counted all frozen records and **still passed**. + A candidate with no `Cat/Item` argument drills the whole store (16 records), + and it reads them in insertion order through `iter_cells`, which the + injection cannot reach. So the guard counts only drills that froze fewer + records than the store holds. + - Measured at `7dfa90f1`, test binary in a loop: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + parser rival | **0 of 300** | all 300 | + | gotcha 18 alone | **0 of 300** | 0 (every failure a real divergence) | + | neither | 300 of 300 | 0 | +- **`StartDrill`'s `Debug` changed to print only its row count** + (`StartDrill(4 rows)`, improvise-btyw): the law compares effects by their + `Debug` text, and this rival takes the record order out of that text, while + the frozen `drill_state` still holds four records. At `7dfa90f1`, with gotcha + 18 re-injected, the law **passed 200 of 200** runs and the guard stayed green. + The guard now counts a drill only when its frozen records, as `Debug` prints + them, appear in the text the comparison compared. The same measurements after + the repair, 200 runs each: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + `Debug` rival | **0 of 200** | all 200 ("froze 0 records") | + | gotcha 18 + parser rival | **0 of 200** | all 200 | + | gotcha 18 alone | **0 of 200** | 0 (every failure a real divergence) | + | neither | 200 of 200 | 0 | - **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. It fails as "listed … but did not". @@ -5614,3 +5716,51 @@ store is exactly `{Clothing/Jan/Revenue = 1}`. Under the rival the clear lands on `Food/Jan/Profit`, where there is nothing to clear, and `Clothing/Jan/Profit = 5` survives. Injected alone into a full workspace run, the rival fails this test and nothing else. + +### a_fresh_sessions_slice_is_the_hosts_startup_slice + +improvise-v8ab. A fresh session's slice is built twice: `App::with_storage` +builds the host's own, and `project.rs::fresh_view` builds a client's. Nothing +asserted they agree. `tests/lockstep.rs` seeds most of its clients from +`app.view_state.clone()`, and those could not see improvise-lxg0's phantom +frame or the next drift between the two. There are three exceptions. +`switching_between_a_records_view_and_a_pivot_view_settles_both_ways` and +`a_locally_resolved_page_turn_lands_where_the_host_would_put_it` go through +`Host::connect`, and the `published_basis` helper uses `Sessions::connect`. +None of them compares the connected slice with the host's. The test connects to two documents, both opening on `Second`, one with a `Default` view +and one without. It asserts the whole `ViewState` equals the host's, not only +the back stack, so any field the two constructors disagree on fails it. The +rival is the old fold, restored in `fresh_view`. It failed on the +`["Default", "Second"]` case: the only difference was `view_back_stack: +[ViewFrame { view_name: "Default", mode: Normal }]`. + +The scope is **connect only**. A rebase deliberately resets the client's whole +slice, while `install_workbook` keeps the host's mode and panels, so equality +is not the contract there. `lockstep.rs` still copies `app.view_state`, and +that is deliberate. Its helpers seed a client *after* mutating the host (mode, +buffers, cursor), so a `Snapshot` would drop exactly the state those tests +stage. This test is what makes the copy faithful at startup. + +### a_document_opening_on_a_records_view_opens_the_host_in_records_normal + +improvise-y5eo. `App::with_storage` set `view_state.mode` to `Normal` and never +settled it against the layout. The host settles only at the end of a batch +(`settle_normal_mode` in `apply_effects`). A session's mirror settles in +`project_diff`, so `Sessions::connect` handed a client `RecordsNormal` while +the host sat in `Normal` over the same records grid. The host's first `i` then +resolved through `Normal`'s keymap and entered plain `Editing`. The fixture's +active view puts `_Index` on rows and `_Dim` on columns, which the `.improv` +writer does persist. The test asserts the starting mode, then presses `i` and +asserts the host is still in a records mode. The fix calls +`settle_normal_mode` once at the end of construction, so the startup mode +comes from the same rule every batch ends with, and so from the same +`improvise_protocol::settled_mode` a client uses. + +Rival injected: that one call removed. The whole workspace had exactly two +failures. This test failed at the mode assertion (`left: Normal, right: +RecordsNormal`), and `a_fresh_sessions_slice_is_the_hosts_startup_slice` +failed on its new records case: the client's `RecordsNormal` against the +host's `Normal`, and nothing else different. `install_workbook` needs no +change of its own, because both of its callers (`LoadModel`, +`AcceptWizardModel`) run inside `apply_effects`, which settles after the batch. + diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index cd8ff234..3304bd5e 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -719,3 +719,47 @@ every target-specific entry, whose **normal** serde_json dependency lacks The `checked.len() >= 2` assertion is the positive signal. If a cargo change renamed the metadata fields, the check would find no serde_json dependency and pass vacuously without it. + +### The re-poll a second boundary could skip — `scripts/pr-reviews-test.sh` (improvise-qnpi) + +**What broke.** "an inline comment arriving mid-wait is seen" failed once in +two identical runs of `checkers.sh` on a loaded machine. `pr-reviews --wait` +computed `deadline = now + WAIT` and then tested `now < deadline` *before* its +first re-poll. With `--wait 1`, a second boundary falling between those two +reads of the clock made the loop run zero times: exit 1 with one poll, whatever +the second poll would have shown. + +**Reproduced under load.** A loop running that fixture with one busy loop per +core failed once, as `exit 1, polls=1`, around run 52; the fixed script ran +200 times under the same load with no failure. That is evidence of the +mechanism, not proof: the window is a few milliseconds per run. + +**The case that proves it.** "a second passing before the first re-poll does +not skip it" puts a `date` on `PATH` that advances one second per read (a +loaded machine, compressed) and a no-op `sleep`. Against the old loop it fails +every time, exit 1 (22 passed, 1 failed). The fix checks the deadline *after* +each poll, so any `--wait` above 0 polls at least once more. A larger timeout +would not have helped: the case uses the shimmed clock, where only the order of +check and poll matters. + +### The harness lock that every build rewrote — `harness/build.sh` (improvise-mkbp) + +**What broke.** `harness/opfs-contract/Cargo.lock` pinned `pest 2.9.1` while +its path dependencies require `pest = "2.9.2"`, so the committed lock was +invalid for its own manifest and every cargo invocation rewrote it. + +**Why both halves.** The resolved lock is committed, and `build.sh` now starts +with `cargo metadata --locked` against it. The lock alone would repeat the bug +at the next workspace dependency bump; `--locked` alone fails today. The check +is its own step because `wasm-pack build … -- --locked` does not work: with the +stale lock restored, that version exited 0 and still rewrote the lock, because +wasm-pack re-resolves before the flag reaches `cargo build`. The standalone +check, against the same stale lock, exits 101 in seconds and leaves the file +untouched. + +**The assemble half.** `web/assemble.sh` starts with `rm -rf` of its output, +which is what keeps the deployed tree free of stale files, so it is not taught +to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html` +is served, naming the fix. Injected by running `web/assemble.sh harness/serve` +alone: 4 chromium failures, each carrying that message, where it used to fail +with no hint of the cause. diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 8f0a71e5..06135353 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -1,14 +1,14 @@ //! A command is a function of its context: two contexts built independently from //! equal data give equal effects — `context/code/command.md`, improvise-hzjo. -use std::collections::{BTreeSet, HashSet}; +use std::collections::BTreeSet; use super::core::{Cmd, CmdContext}; +use super::grid::DrillIntoCell; use super::registry::tests::candidate_args; use crate::command::key::{KeyCode, KeyModifiers}; use crate::model::Model; use crate::model::cell::{CellKey, CellValue}; -use crate::model::record::record_of; use crate::ui::app::App; use crate::ui::effect::{self, Effect}; @@ -61,27 +61,23 @@ fn one_coordinate_drills(model: &Model) -> Vec> { drills } -/// How many distinct records the widest all-`Cat/Item` candidate matches. -fn widest_drill(model: &Model, candidates: &[Vec]) -> usize { - let coordinates = |args: &Vec| -> Option> { - args.iter() - .map(|a| { - a.split_once('/') - .map(|(c, i)| (c.to_string(), i.to_string())) - }) - .collect() - }; - candidates - .iter() - .filter(|args| !args.is_empty()) - .filter_map(coordinates) - .map(|partial| { - let cells = model.data.matching_cells(&partial); - let records: HashSet = cells.iter().map(|(k, _)| record_of(k)).collect(); - records.len() - }) - .max() - .unwrap_or(0) +/// Records a compared drill froze, counted only if it narrowed the store and its +/// ordered records are in `compared`, the text the law compared (improvise-btyw). +fn records_narrowed(scratch: &mut App, effects: Vec>, compared: &str) -> usize { + let store = scratch + .model_state + .workbook + .model + .data + .live_records() + .count(); + scratch.apply_effects(effects); + let frozen = scratch + .drill_state + .as_ref() + .filter(|drill| compared.contains(&format!("{:?}", drill.records))) + .map_or(0, |drill| drill.records.len()); + if frozen < store { frozen } else { 0 } } /// The registry's candidates, plus names the demo really has: a drill or a fit @@ -108,9 +104,9 @@ fn args_the_document_names(model: &Model) -> Vec> { all } -fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option { +fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option>> { let cmd = ctx.registry.interactive(name, args, ctx).ok()?; - Some(format!("{:?}", cmd.execute(ctx))) + Some(cmd.execute(ctx)) } /// What running every registered command under `a` and under each of `others` found. @@ -120,20 +116,34 @@ struct Comparison { shown: Vec, never_built: Vec<&'static str>, compared: usize, + widest_drill: usize, } fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { let mut found = Comparison::default(); let candidates = args_the_document_names(a.model); + let drill = DrillIntoCell { + key: CellKey::new(vec![]), + }; + let mut scratch = fixture(); for name in a.registry.names().collect::>() { let before = found.compared; for args in &candidates { - let Some(left) = effects_of(name, args, a) else { + let Some(effects) = effects_of(name, args, a) else { continue; }; + let left = format!("{effects:?}"); + if name == drill.name() { + found.widest_drill = + found + .widest_drill + .max(records_narrowed(&mut scratch, effects, &left)); + } found.compared += 1; for b in others { - let right = effects_of(name, args, b).unwrap_or_default(); + let right = effects_of(name, args, b) + .map(|e| format!("{e:?}")) + .unwrap_or_default(); if left != right { found.diverged.insert(name); found @@ -157,13 +167,12 @@ fn every_command_gives_equal_effects_in_independently_built_equal_contexts() { .iter() .map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE)) .collect(); - let model = contexts[0].model; - let widest = widest_drill(model, &args_the_document_names(model)); - assert!( - widest >= 4, - "the widest drill orders {widest} records, so the store's order is nearly a coin toss" - ); let found = compare(&contexts[0], &contexts[1..]); + assert!( + found.widest_drill >= 4, + "the widest narrowing drill compared froze {} records, so the store's order is nearly a coin toss", + found.widest_drill + ); assert!( found.never_built.is_empty(), "no candidate args build these in the fixture, so the law never ran them: {:?}", diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 5ac40a44..4b878fe9 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -386,7 +386,9 @@ impl App { let view = workbook.active_view(); GridLayout::with_frozen_records(&workbook.model, view, None) }; - Self { + // A document may open on a records grid, so the first mode is settled + // by the rule every batch ends with (improvise-y5eo). + let mut app = Self { model_state: ModelState { workbook, file_path, @@ -416,7 +418,9 @@ impl App { last_reframed_elsewhere: std::collections::BTreeSet::new(), keymap_set: KeymapSet::default_keymaps(), storage, - } + }; + app.settle_normal_mode(); + app } /// This client's cursor in the active view. Absent means the origin, so @@ -2758,6 +2762,29 @@ mod tests { } } + /// improvise-y5eo: a document that opens on a records grid opens the host in + /// `RecordsNormal`. See `context/test-evidence/command.md`. + #[test] + fn a_document_opening_on_a_records_view_opens_the_host_in_records_normal() { + use crate::view::Axis; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.active_view_mut().set_axis("_Index", Axis::Row); + wb.active_view_mut().set_axis("_Dim", Axis::Column); + let mut app = App::new(wb, None); + assert!(app.layout.is_records_mode(), "setup: the grid is records"); + assert_eq!(app.view_state.mode, AppMode::RecordsNormal); + + app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE)) + .unwrap(); + assert!( + app.view_state.mode.is_records(), + "the host's first `i` left records mode: {:?}", + app.view_state.mode + ); + } + /// The settle's `RecordsNormal` arm, driven from the mode it names. The /// `:` crossing above enters the settle in `Normal` both ways — the evidence. #[test] @@ -3001,16 +3028,34 @@ mod tests { fn a_failed_load_or_import_does_not_run_the_quit_after_it() { use improvise_protocol::keymap::Keymap; - for (command, path) in [("load", "absent.improv"), ("import-json", "absent.json")] { + // One case per `refuse_and_stop` site (improvise-514o). + let cases: [(&str, &[&str], &str); 7] = [ + ("load", &["absent.improv"], "Load error"), + ("import-json", &["absent.json"], "Cannot read 'absent.json'"), + ("import-json", &["absent.csv"], "Cannot read 'absent.csv'"), + ("import-json", &["bad.csv"], "CSV error"), + ("import-json", &["bad.json"], "JSON parse error"), + ( + "import-json", + &["nested.json", "M", "nope"], + "No array at path", + ), + ("import-json", &["measures.json"], "Import error"), + ]; + for (command, args, arm) in cases { + let path = args.join(" "); let mut app = App::new(Workbook::new("T"), None); - app.storage = Box::new(crate::storage::MemoryFs::new()); + app.storage = Box::new(crate::ui::host::import_fixture_storage()); let key = KeyCode::Char('x'); let mods = KeyModifiers::NONE; let mut km = Keymap::new(); km.bind_seq( key, mods, - vec![(command, vec![path.to_string()]), ("force-quit", vec![])], + vec![ + (command, args.iter().map(|a| a.to_string()).collect()), + ("force-quit", vec![]), + ], ); let effects = { let ctx = app.cmd_context(key, mods); @@ -3020,8 +3065,8 @@ mod tests { app.apply_effects(effects); assert!( - app.last_batch_refused(), - "setup: `{command}` must actually have failed, got {:?}", + app.last_batch_refused() && app.view_state.status_msg.contains(arm), + "setup: `{command} {path}` must fail in the {arm:?} arm, got {:?}", app.view_state.status_msg ); assert_ne!( @@ -3032,6 +3077,392 @@ mod tests { } } + /// improvise-v4hg: Enter on a formula that does not parse threw the typed text + /// away. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text() { + let type_and_commit = |text: &str| { + let mut app = two_col_model(); + app.view_state.mode = AppMode::FormulaPanel; + let press = |app: &mut App, code| { + app.handle_key(KeyEvent::new(code, KeyModifiers::NONE)) + .unwrap() + }; + press(&mut app, KeyCode::Char('a')); + assert!( + matches!(app.view_state.mode, AppMode::FormulaEdit { .. }), + "setup: `a` in the formula panel opens the editor" + ); + for c in text.chars() { + press(&mut app, KeyCode::Char(c)); + } + press(&mut app, KeyCode::Enter); + app + }; + + // The control: a formula that parses is stored and the editor closes. + let ok = type_and_commit("Total = 1"); + assert_eq!(ok.view_state.mode, AppMode::FormulaPanel); + assert!( + ok.model_state + .workbook + .model + .formulas() + .iter() + .any(|f| f.target == "Total"), + "the control formula must land" + ); + + let bad = type_and_commit("Total = ("); + assert!( + bad.last_batch_refused() && bad.view_state.status_msg.contains("Formula error"), + "the parse error must be reported, got {:?}", + bad.view_state.status_msg + ); + assert!( + matches!(bad.view_state.mode, AppMode::FormulaEdit { .. }), + "a refused formula must leave the user in the editor, got {:?}", + bad.view_state.mode + ); + assert_eq!( + bad.view_state.buffers.get("formula").map(String::as_str), + Some("Total = ("), + "and must keep what they typed" + ); + } + + /// Run a `:` line the way `improvise cmd` does, one batch per segment; true + /// when any segment refused, which is what its exit code reads. + fn run_line(app: &mut App, line: &str) -> bool { + let mut refused = false; + for segment in crate::command::parse::segments(line) { + let effects = { + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + crate::command::parse::parse_segment_interactive(segment, &ctx) + .unwrap_or_else(|e| panic!("`{segment}` must parse: {e}")) + .map(|cmd| cmd.execute(&ctx)) + .unwrap_or_default() + }; + app.apply_effects(effects); + refused |= app.last_batch_refused(); + } + refused + } + + /// Commit `text` in a minibuffer mode with Enter; true when that batch refused. + fn commit_in(app: &mut App, mode: AppMode, buffer: &str, text: &str) -> bool { + app.view_state.mode = mode; + app.view_state + .buffers + .insert(buffer.to_string(), text.to_string()); + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + app.last_batch_refused() + } + + fn with_import_fixtures(mut app: App) -> App { + app.storage = Box::new(crate::ui::host::import_fixture_storage()); + app + } + + /// Eleven regular categories: one short of the limit. + fn one_category_short_of_the_limit() -> App { + let mut wb = Workbook::new("T"); + for n in 0..11 { + wb.add_category(format!("C{n}")).unwrap(); + } + App::new(wb, None) + } + + /// Stage one coordinate rename in a drill over the records fixture, then commit it. + fn commit_drill_rename(app: &mut App, to: &str) -> bool { + let records = app.layout.records.clone().expect("records mode"); + let mut pending_edits = HashMap::new(); + pending_edits.insert( + (1, crate::view::RecordCol::Category("Region".into())), + to.into(), + ); + app.drill_state = Some(DrillState { + records, + pending_edits, + }); + app.apply_effects(vec![Box::new(crate::ui::effect::ApplyAndClearDrill)]); + app.last_batch_refused() + } + + /// Records mode with `Region` paged to `page`: `add-record-row` then names + /// the row whose coordinates are exactly that page. + fn records_paged_to(app: &mut App, page: &str) { + app.model_state.workbook.add_item("Region", "West").unwrap(); + let view = app + .view_mut_of(&ViewTarget::Active) + .expect("the active view"); + view.set_axis("Region", crate::view::Axis::Page); + view.set_page_selection("Region", page); + app.rebuild_layout(); + } + + /// A grid with a regular category on None, so every cell is an aggregate. + fn aggregated_grid(with_items: bool) -> App { + let mut wb = Workbook::new("T"); + for cat in ["Row", "Col", "Hidden"] { + wb.add_category(cat).unwrap(); + if with_items { + wb.add_item(cat, "A").unwrap(); + } + } + wb.active_view_mut() + .set_axis("Hidden", crate::view::Axis::None); + let mut app = App::new(wb, None); + app.rebuild_layout(); + app + } + + /// `Region` down, `_Measure` across: `Revenue` at column 0, then `Total`, + /// which a formula owns, at column 1. + fn a_formula_column() -> App { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.active_view_mut() + .set_axis("_Measure", crate::view::Axis::Column); + let mut app = App::new(wb, None); + app.apply_effects(vec![Box::new(crate::ui::effect::AddFormula { + raw: "Total = Revenue * 2".into(), + target_category: "_Measure".into(), + })]); + app + } + + fn with_a_cell(mut app: App) -> App { + app.model_state.workbook.model.set_cell( + CellKey::new(vec![("Row".into(), "A".into()), ("Col".into(), "X".into())]), + CellValue::Number(42.0), + ); + app.rebuild_layout(); + app + } + + fn with_a_second_view(mut app: App) -> App { + app.model_state.workbook.create_view("Second"); + app + } + + fn switch_to_second(app: &mut App) { + app.apply_effects(vec![Box::new(crate::ui::effect::SwitchView( + "Second".into(), + ))]); + } + + /// improvise-0v8l: one case per refusal site no other test pinned, each with a + /// succeeding twin. Evidence: `context/test-evidence/command.md`. + #[test] + fn every_refusal_site_refuses_its_failure_and_only_its_failure() { + type Setup = fn() -> App; + type Drive = fn(&mut App) -> bool; + let cases: Vec<(&str, Setup, Drive, &str, Setup, Drive)> = vec![ + ( + "add-category past the limit", + one_category_short_of_the_limit, + |app| run_line(app, "add-category A . add-category B"), + "Maximum of 12 categories", + one_category_short_of_the_limit, + |app| run_line(app, "add-category A"), + ), + ( + "a drill commit that empties a coordinate", + records_app_for_drill, + |app| commit_drill_rename(app, ""), + crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY, + records_app_for_drill, + |app| commit_drill_rename(app, "West"), + ), + ( + "import of a JSON that does not parse", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import bad.json"), + "JSON parse error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import good.json"), + ), + ( + "import of an absent file", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import absent.json"), + "Cannot read file", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import good.json"), + ), + ( + "import-json of an absent CSV", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json absent.csv"), + "Cannot read 'absent.csv'", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json of a CSV that is not text", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json bad.csv"), + "CSV error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json of JSON that does not parse", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json bad.json"), + "JSON parse error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json at a path with no array", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json nested.json M nope"), + "No array at path", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json that builds no model", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json measures.json"), + "Import error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "a records coordinate edited to nothing", + records_model_with_two_rows, + |app| { + app.cursor_mut().selected = (1, 0); + commit_in(app, AppMode::records_editing(), "edit", "") + }, + crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY, + records_model_with_two_rows, + |app| { + app.cursor_mut().selected = (1, 0); + commit_in(app, AppMode::records_editing(), "edit", "West") + }, + ), + ( + "an export with no path", + || with_import_fixtures(two_col_model()), + |app| commit_in(app, AppMode::export_prompt(), "export", ""), + "not exported", + || with_import_fixtures(two_col_model()), + |app| commit_in(app, AppMode::export_prompt(), "export", "out.csv"), + ), + ( + "view-forward with nowhere to go", + || with_a_second_view(two_col_model()), + |app| run_line(app, "view-forward"), + "No forward view", + || with_a_second_view(two_col_model()), + |app| { + switch_to_second(app); + run_line(app, "view-back"); + run_line(app, "view-forward") + }, + ), + ( + "view-back with nowhere to go", + || with_a_second_view(two_col_model()), + |app| run_line(app, "view-back"), + "No previous view", + || with_a_second_view(two_col_model()), + |app| { + switch_to_second(app); + run_line(app, "view-back") + }, + ), + ( + "add-record-row outside records mode", + two_col_model, + |app| run_line(app, "add-record-row"), + "only works in records mode", + records_model_with_two_rows, + |app| run_line(app, "add-record-row"), + ), + ( + "add-record-row onto a row that is already there", + records_model_with_two_rows, + |app| { + records_paged_to(app, "South"); + run_line(app, "add-record-row") + }, + "already here", + records_model_with_two_rows, + |app| { + records_paged_to(app, "West"); + run_line(app, "add-record-row") + }, + ), + ( + "edit-or-drill on an aggregate grid with no cell", + || aggregated_grid(false), + |app| run_line(app, "edit-or-drill editing"), + "No cell at cursor", + || aggregated_grid(true), + |app| run_line(app, "edit-or-drill editing"), + ), + ( + "edit-or-drill on a cell a formula owns", + a_formula_column, + |app| { + app.cursor_mut().selected = (0, 1); + run_line(app, "edit-or-drill editing") + }, + "computed by a formula", + a_formula_column, + |app| { + app.cursor_mut().selected = (0, 0); + run_line(app, "edit-or-drill editing") + }, + ), + ( + "a search with no matches", + || with_a_cell(two_col_model()), + |app| { + app.view_state.search_query = "zzz".into(); + run_line(app, "search-navigate forward") + }, + "No matches", + || with_a_cell(two_col_model()), + |app| { + app.view_state.search_query = "42".into(); + run_line(app, "search-navigate forward") + }, + ), + ( + "a : line that does not parse", + two_col_model, + |app| commit_in(app, AppMode::command_mode(), "command", "no-such-command"), + ":no-such-command", + two_col_model, + |app| commit_in(app, AppMode::command_mode(), "command", "add-category Z"), + ), + ]; + for (what, fail_setup, fails, says, ok_setup, succeeds) in cases { + let mut app = fail_setup(); + let refused = fails(&mut app); + assert!( + refused && app.view_state.status_msg.contains(says), + "{what}: expected a refusal saying {says:?}, got refused={refused} {:?}", + app.view_state.status_msg + ); + let mut app = ok_setup(); + assert!( + !succeeds(&mut app), + "{what}: its succeeding twin was refused: {:?}", + app.view_state.status_msg + ); + } + } + /// A prefix is consumed whether or not the next key matches, so a stray key /// after `g` does nothing rather than acting as a fresh Normal binding. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 51ec609d..1c3f9a03 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -47,8 +47,8 @@ pub enum ViewTarget { Named(String), } -/// Refuse AND stop the batch, for a failed document swap: what follows it in -/// a `:` line assumes it happened, so `load x . q` must not quit (cz8d). +/// Refuse AND stop the batch, where what follows assumes the change happened: +/// `load x . q` must not quit, a bad formula must not close its editor (cz8d). fn refuse_and_stop(app: &mut App, why: impl Into) { app.refuse(why); app.abort_effects = true; @@ -221,9 +221,9 @@ impl Effect for AddFormula { } app.model_state.workbook.model.add_formula(formula); } - Err(e) => { - app.refuse(format!("Formula error: {e}")); - } + // Stop too: what follows it on the key path closes the editor and + // clears the buffer, which would throw the user's text away (v4hg). + Err(e) => refuse_and_stop(app, format!("Formula error: {e}")), } } fn kind(&self) -> EffectKind { diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index f31b1d08..78aac189 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -266,6 +266,27 @@ impl Host { } } +/// One file per way `ImportJsonHeadless` can fail, plus one that imports; the +/// `absent.*` paths are left unwritten. Shared by the frame and key-path tests. +#[cfg(test)] +pub(crate) fn import_fixture_storage() -> crate::storage::MemoryFs { + let storage = crate::storage::MemoryFs::new(); + let files: [(&str, &[u8]); 5] = [ + // Every field numeric, so no category is accepted and the build fails. + ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), + ("bad.json", b"{not json"), + ("nested.json", br#"{"data": [{"Region": "North"}]}"#), + // A record that is not UTF-8 is a CSV read error, not a warning. + ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), + ("good.json", br#"[{"Region": "North", "Revenue": 1}]"#), + ]; + for (name, bytes) in files { + crate::storage::Storage::write(&storage, std::path::Path::new(name), bytes) + .expect("fixture writes"); + } + storage +} + #[cfg(test)] mod tests { use std::path::{Path, PathBuf}; @@ -1946,19 +1967,7 @@ mod tests { /// A host whose storage holds one file per way `ImportJsonHeadless` can /// fail, plus nothing at the two paths that must fail to read. fn host_with_import_fixtures() -> Host { - let storage = MemoryFs::new(); - let files: [(&str, &[u8]); 4] = [ - // Every field numeric, so no category is accepted and the build fails. - ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), - ("bad.json", b"{not json"), - ("nested.json", br#"{"data": [{"Region": "North"}]}"#), - // A record that is not UTF-8 is a CSV read error, not a warning. - ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), - ]; - for (name, bytes) in files { - crate::storage::Storage::write(&storage, Path::new(name), bytes) - .expect("fixture writes"); - } + let storage = super::import_fixture_storage(); // A second view, so deleting an absent one is not refused as "the last". let mut wb = workbook(); wb.create_view("Scratch"); diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 76b227c5..8ce9e258 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -1603,6 +1603,32 @@ mod tests { } } + /// improvise-v8ab: a fresh client starts from the host's own startup slice. + /// See `context/test-evidence/command.md`. + #[test] + fn a_fresh_sessions_slice_is_the_hosts_startup_slice() { + for (names, records) in [ + (&["Default", "Second"][..], false), + (&["Main", "Second"][..], false), + (&["Main", "Second"][..], true), + ] { + let mut wb = opening_on(names, "Second"); + if records { + // `Second` opens on a records grid (improvise-y5eo). + wb.active_view_mut().set_axis("_Index", Axis::Row); + wb.active_view_mut().set_axis("_Dim", Axis::Column); + } + let app = App::new(wb, None); + assert_eq!(app.view_state.active_view, "Second", "setup: {names:?}"); + assert_eq!(app.layout.is_records_mode(), records, "setup: {names:?}"); + let (_, snapshot) = Sessions::default().connect(&app, 10, 10); + assert_eq!( + snapshot.view, app.view_state, + "{names:?}, records {records}" + ); + } + } + /// improvise-lxg0: a document swap reseeds through the same door. #[test] fn a_rebased_session_has_no_history_to_step_back_through() { diff --git a/harness/build.sh b/harness/build.sh index f4e21ffa..ed73ca64 100755 --- a/harness/build.sh +++ b/harness/build.sh @@ -9,10 +9,21 @@ # half is the *deployed* arrangement rather than `web/` itself, because a # harness that runs against a different tree from the one that ships is # testing something nobody deploys (improvise-vvl3). +# +# Re-run THIS script, never `web/assemble.sh harness/serve` alone: assembling +# starts with `rm -rf` of its output, which deletes opfs/ and the fixtures +# (improvise-mkbp). opfs.spec.js says so if it finds opfs/ missing. set -eu root=$(cd "$(dirname "$0")/.." && pwd) serve="$root/harness/serve" +# A stale opfs-contract/Cargo.lock fails HERE rather than being rewritten in +# every agent's tree (improvise-mkbp). wasm-pack cannot enforce this: it +# re-resolves the lock itself before any `-- --locked` reaches cargo build. +# Fix a failure with `cargo update -w` in harness/opfs-contract, and commit it. +cargo metadata --locked --format-version 1 \ + --manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null + "$root/web/build.sh" >/dev/null "$root/web/assemble.sh" "$serve" harness >/dev/null diff --git a/harness/opfs-contract/Cargo.lock b/harness/opfs-contract/Cargo.lock index 08a283d2..d20a9d11 100644 --- a/harness/opfs-contract/Cargo.lock +++ b/harness/opfs-contract/Cargo.lock @@ -308,9 +308,9 @@ dependencies = [ [[package]] name = "pest" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6d45aeb61b4bf818e12d4205f2466f8c4748f85f4fce0146d1c03d69d753f0ad" +checksum = "45d3aca230fad2e6f6317ca0a72724338c4960cb97168a85cdee66df4a9a21a8" dependencies = [ "memchr", "ucd-trie", @@ -318,9 +318,9 @@ dependencies = [ [[package]] name = "pest_derive" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "89cc5a242e25ed4e7704d0be240f2cfbe20a8c27e7e252d94835be93d92dc39f" +checksum = "284b60557f2c4a2e72ad3f2d34d42685a2fa4a6a61d0d2a10c0ae2a5e916c2cf" dependencies = [ "pest", "pest_generator", @@ -328,9 +328,9 @@ dependencies = [ [[package]] name = "pest_generator" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7abf21475cc3820fe4b2ca2dc2142902f67a02189f3b5b3a229f4febc01a43e5" +checksum = "1d9d1f08a115309ee99268cf85e5228e0e56aa9caf8841ec12866b6be07c3109" dependencies = [ "pest", "pest_meta", @@ -341,9 +341,9 @@ dependencies = [ [[package]] name = "pest_meta" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "adba4db388f687393c18c51348d44a41d870ca9df71a2c98172ea3035dc6936e" +checksum = "ed93ba1a9ffcca32130a5188701c81c0c49cf00d4b7c5007d5148951d743adcb" dependencies = [ "pest", ] diff --git a/harness/tests/opfs.spec.js b/harness/tests/opfs.spec.js index 5d480441..83b2e556 100644 --- a/harness/tests/opfs.spec.js +++ b/harness/tests/opfs.spec.js @@ -23,7 +23,11 @@ const EXPECTED = [ test.describe("OpfsFs in a real browser", () => { test.beforeEach(async ({ page, browserName }, testInfo) => { - await page.goto("/opfs/index.html"); + const served = await page.goto("/opfs/index.html"); + expect( + served?.ok(), + "harness/serve/opfs is missing: re-run harness/build.sh, not web/assemble.sh alone", + ).toBe(true); // The skip is conditional on the PROBE, not on the browser name, so it // cannot quietly swallow a real regression. See clients.md. diff --git a/scripts/pr-reviews b/scripts/pr-reviews index cf46427d..55de7185 100755 --- a/scripts/pr-reviews +++ b/scripts/pr-reviews @@ -9,7 +9,8 @@ usage() { usage: pr-reviews [--wait ] [--interval ] --wait wait up to this long for a review or an inline review - comment whose commit is the current head + comment whose commit is the current head; any --wait + above 0 polls at least once more after the first read (default 0 = print what exists and exit) --interval seconds between polls (default 30) @@ -157,8 +158,10 @@ report "$json" if has_head_review "$json"; then exit 0; fi +# The deadline is checked AFTER each poll: checked first, a second ticking over +# between two reads of the clock skipped the only re-poll (improvise-qnpi). deadline=$(( $(date +%s) + WAIT )) -while [ "$(date +%s)" -lt "$deadline" ]; do +while :; do sleep "$INTERVAL" json=$(fetch) if has_head_review "$json"; then @@ -166,6 +169,7 @@ while [ "$(date +%s)" -lt "$deadline" ]; do report "$json" exit 0 fi + [ "$(date +%s)" -lt "$deadline" ] || break done printf '\npr-reviews: waited %ss; nothing against the current head.\n' "$WAIT" diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index a591f760..520315fe 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -197,6 +197,26 @@ mk "$d" last.issue.json <<'EOF' EOF run_case "an inline comment arriving mid-wait is seen" 0 "$d" --wait 1 --interval 1 +# The same fixture on a clock that ticks one second per read: a loaded machine, +# compressed. A second passing between two reads must not cost the only poll. +clock="$work/clock" +mkdir -p "$clock" +cat > "$clock/date" <<'EOF' +#!/usr/bin/env bash +[ "${1:-}" = "+%s" ] || exec /bin/date "$@" +n=$(( $(cat "$CLOCK_TICKS" 2>/dev/null || echo 0) + 1 )) +echo "$n" > "$CLOCK_TICKS" +echo $(( 1000 + n )) +EOF +printf '#!/usr/bin/env bash\nexit 0\n' > "$clock/sleep" +chmod +x "$clock/date" "$clock/sleep" +rm -f "$d/.poll" "$work/ticks" +CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$d" \ + "$REVIEWS" 1 --wait 1 --interval 1 > "$work/out" 2>&1 +got=$? +[ "$got" = 0 ] && ok "a second passing before the first re-poll does not skip it" \ + || bad "a second passing before the first re-poll does not skip it" "exit $got, wanted 0" + # --------------------------------------------------------------------------- # `gh api --paginate` emits one array PER PAGE, so a thread past the page size # arrives as several concatenated documents. Passing that straight to