Skip to content

test(EasyTrees): let the dataset load settle before setting treeRange - #48

Merged
ms609 merged 2 commits into
cpp-searchfrom
feature/shiny-debounce-wait
Aug 4, 2026
Merged

test(EasyTrees): let the dataset load settle before setting treeRange#48
ms609 merged 2 commits into
cpp-searchfrom
feature/shiny-debounce-wait

Conversation

@ms609

@ms609 ms609 commented Aug 4, 2026

Copy link
Copy Markdown

Fixes #46

Stacked on #45 — branched from feature/ci-maxmin-windows, so it carries that
PR's two commits until #45 merges. Merge #45 first and this reduces to the one commit.
It has to be stacked: without #45 the shinytest2 job dies in Set up R dependencies
and never reaches the suite, so there is nothing to verify against.

What was wrong

The Distribution baseline recorded

trees <- WideSample(trees[1:125], 48)

for a state its own test sets to c(77, 125) two lines earlier
(test-Distribution.R:16) — and had
done so since the snapshot was first recorded on 2026-07-03. The pre-2026-07-27 version of
the file shows the same 1, 125 under the old evenly-spaced stride, so this predates the
WideSample logging change and is not MaxMin-specific.

Why now, and why not "just stale"

CI had never checked it: the job died on the unresolvable MaxMin Suggests (#43) long
before reaching the suite. With #45 in place the runner reaches the tests and produces
trees[77:125] — the value the test asks for — so the stored baseline is the wrong one.

The cause is that wait_for_idle() does not wait for a pending debounce() timer.
mod_data's nTree / treeRange watchers are debounced (aJiffy = 42 ms,
typingJiffy = 105 ms), and while that timer runs Shiny has nothing to recompute, so it
reports idle. set_inputs() + wait_for_idle() can therefore return before the watcher has
seen the value just set, and a download captured then encodes the previous state. Whether
the timer wins is machine-dependent — which is exactly why the dev box and the runner
disagreed on this one snapshot while agreeing on the other ten.

So snapshot_accept() alone would have been the wrong move: it would have re-recorded
whichever side of the race the recording machine happened to land on, leaving the test free
to flip back later.

The change

  • wait_stable() sleeps past the longest debounce window and waits for idle again, so a
    capture happens after debounced work has run rather than possibly before it.
  • The 001 baseline is replaced with the windows-latest runner's own output, taken from
    the .new artifact of run 30919105631,
    rather than a locally re-recorded file — that run took the correct path, and adopting its
    bytes keeps the baseline anchored to the platform the job runs on. Only the input line
    differs at source; the clustering conclusion, cluster count and rogue-tip lists all follow
    from the different tree subset.

What is verified, and what is not

Verified: the adopted baseline is byte-identical to what windows-latest / R 4.6.1
produced. Snapshots 002-011 already matched the locally-recorded baselines in that run
(FAIL 1, PASS 191), so cross-machine reproducibility is established for every state except
the racy one.

NOT verified locally. I could not run the suite on this box: the app subprocess aborts
with namespace 'rlang' 1.2.0 is already loaded, but >= 1.3.0 is required, a stale local
library unrelated to this change, and updating the shared default library would disrupt
concurrent sessions. So the wait_stable() change is reasoned from the debounce/idle
semantics, not measured. CI is the check here — and if the extra settling shifts any
other snapshot, the job uploads .new artifacts to adopt.

The job is continue-on-error: true, so this cannot redden the workflow either way.

Also worth knowing

WideSample() stop()s when MaxMin is absent
(R/WideSample.R:149-152) — no silent fallback — so this baseline is
only reproducible where MaxMin resolves. That makes #45 a permanent prerequisite for the
suite, not just for this PR.

R/WideSample.R sits in the unowned statistics/support-metrics cluster from #42, so no
red-team area owns this code path today.

Investigated and dropped

I first suspected FetchTreeRange's echo guard, which identifies a programmatic update's
echo by arrival order and discards the first firing whose value differs from the pre-push
one — which looks like it could eat a real edit. Probing the module showed it cannot, in
practice: arming the guard is itself a reactive write, so the watcher re-fires and consumes
the guard ~42 ms after the dataset load, long before any user edit arrives. A candidate fix
that compared against the pushed value instead would have been worse — it would have
treated that stale firing as an edit and reverted the push. Both unit tests I wrote for it
passed against the unmodified module, confirming there was no defect there to guard. No
change made.

🤖 Generated with Claude Code

The Distribution baseline recorded

    trees <- WideSample(trees[1:125], 48)

for a state its own test sets to c(77, 125) two lines earlier, and the
value flipped between machines: the dev box recorded 1:125, the
windows-latest runner produced 77:125.

Neither is what the test asks for. `set_inputs(data-dataSource =
"Sun2018")` does not finish loading inside its default 4 s wait -- the run
log says so outright, "Server did not update any output values within 4
seconds" -- so the `data-treeRange` set on the next line lands while the
dataset is still loading. When the load completes, UpdateAllTrees() sees
the tree count change and calls UpdateTreeRange(c(1L, nTrees)), resetting
the range to the full span and discarding the c(77, 125) the test just
asked for. Whether that reset landed before or after the download was
captured is what varied by machine.

Waiting for the load to settle before touching treeRange makes the
ordering deterministic: the reset happens first, then the test's range is
applied and honoured.

Also make wait_stable() cover the debounce window. mod_data's nTree /
treeRange watchers are debounce()d (aJiffy = 42 ms, typingJiffy = 105 ms),
and a pending debounce timer does not make Shiny busy -- there is nothing
to recompute until it expires, so wait_for_idle() can return before the
watcher has seen the value just set. With the ordering fixed this is
load-bearing rather than defensive: the c(77, 125) thinning is triggered
through that debounced watcher, and set_inputs() cannot be relied on to
wait for it (its own 4 s wait expires with "did not update any output
values" when a set produces no output change).

The 001 baseline is provisionally the windows-latest runner's output for
the 77:125 state, from run 30919105631's uploaded .new artifact. The
input line is now correct by construction, but the clustering conclusion
and rogue-tip lists downstream of it depend on the sampled subset and so
on the RNG state, which the corrected ordering shifts -- to be confirmed
against CI, whose job uploads .new artifacts for exactly this.

Fixes #46

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ms609
ms609 force-pushed the feature/shiny-debounce-wait branch from 067b4da to e3c294d Compare August 4, 2026 16:55
@ms609

ms609 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Correcting the diagnosis in the PR description above. The wait_for_idle()/debounce gap is
real, but it is not the cause — and my first baseline choice had the two states backwards.

Actual cause

The run log is explicit about it:

set_inputs(`data-dataSource` = "Sun2018"): Server did not update any output values within 4 seconds.
    1. \-app$set_inputs(`data-dataSource` = "Sun2018") at test-Distribution.R:14:3

The dataset load does not finish inside set_inputs()' default 4 s wait, so line 16's
data-treeRange = c(77, 125) lands while Sun2018 is still loading. When the load completes,
UpdateAllTrees() sees the tree count change and calls UpdateTreeRange(c(1L, nTrees))
resetting the range to the full span and discarding what the test just asked for. Whether that
reset landed before or after the download was captured is what varied by machine.

What gave it away

My settle-wait change made CI produce 1:125 where it had produced 77:125 — the opposite of
the intended effect. More waiting cannot lose an input that has already been applied, but it can
let the load's reset land before the capture. So:

  • CI's earlier 77:125 was a premature capture, not the correct one.
  • The dev box's 1:125 was the fully-settled state.
  • Neither is what line 16 asks for, which is why re-baselining either way would not have
    fixed anything.

I had this backwards in the description above, where I called the runner's value "the value the
test asks for". It wasn't; it was the same race resolving the other way.

The change now

  • wait_stable() after the data-dataSource set, so the load's range reset happens before
    the test's range is applied, and the range is then honoured. This is the fix.
  • wait_stable() still covers the debounce window — but as a consequence of the above rather
    than the cure. With the ordering fixed, the c(77, 125) thinning is triggered through the
    debounced watcher, and set_inputs() cannot be relied on to wait for it: its own 4 s wait
    expires with "did not update any output values" whenever a set produces no output change
    (which the log also shows, on line 21).

Baseline status

The 001 baseline is provisional. Its input line is now correct by construction, but the
clustering conclusion and rogue-tip lists downstream depend on the sampled subset, hence on the
RNG state, which the corrected ordering shifts. I cannot check that locally — the app subprocess
aborts with namespace 'rlang' 1.2.0 is already loaded, but >= 1.3.0 is required, a stale local
library I am not going to update underneath concurrent sessions. The job uploads .new
artifacts for exactly this case, so if it differs I will adopt the runner's bytes.

Not a merge blocker either way: continue-on-error: true.

Not touched

test-ViewChars.R:15 and test-SearchLog.R:33 emit the same "did not update any output values"
warning on their own dataset loads, so they carry the same latent race. Their snapshots currently
pass, and adding waits there could legitimately shift them, so I have left them alone rather than
widen this PR. Worth a follow-up.

@ms609
ms609 changed the base branch from feature/ci-maxmin-windows to cpp-search August 4, 2026 16:58
@ms609 ms609 closed this Aug 4, 2026
@ms609 ms609 reopened this Aug 4, 2026
@ms609
ms609 changed the base branch from cpp-search to feature/ci-maxmin-windows August 4, 2026 17:02
@ms609 ms609 changed the title test(EasyTrees): make wait_stable() cover the debounce window, and re-baseline the racy snapshot test(EasyTrees): let the dataset load settle before setting treeRange Aug 4, 2026
Base automatically changed from feature/ci-maxmin-windows to cpp-search August 4, 2026 18:13
@ms609
ms609 enabled auto-merge August 4, 2026 18:18
@ms609
ms609 merged commit 2c59965 into cpp-search Aug 4, 2026
9 checks passed
@ms609
ms609 deleted the feature/shiny-debounce-wait branch August 4, 2026 18:20
@ms609

ms609 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Green, and the baseline needed no further change

Run 30931972326 — every job
success, including EasyTrees shinytest2 (windows-latest):

✔ |         22 | Distribution [53.2s]
Duration: 333.9 s
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 192 ]

Previously FAIL 1 | WARN 1 | PASS 191. No shinytest2-snapshots artifact was produced, i.e. no
.new file: the baseline matched byte-for-byte, downstream clustering lines included.

The two Server did not update any output values warnings that named this test — the Sun2018
load and the data-treeRange = c(1, 125) set — are both gone. The ones that remain are
test-ViewChars.R:15 and test-SearchLog.R:33, which this PR deliberately does not touch.

Why the provisional baseline turned out exact

I flagged it as uncertain because the sampled subset depends on RNG state, which the reordering
shifts. It matched for a principled reason rather than luck: the number of sample.int() draws
before the capture is the same either way. Both orderings thin twice — once for the full span at
load (1:125, 48) and once for the test's range (77:125, 48) — so the seed sequence, and hence
the subset, is identical. The premature capture and the settled capture differ only in when the
download was taken relative to the clobber, not in what had been computed.

That also retires the caveat in the PR description: the baseline is no longer provisional.

Full matrix

sense-check (ubuntu-24.04-arm, release), windows-latest (release), ubuntu-24.04 (4.1),
ubuntu-24.04-arm (devel), macOS-latest (release), macos-15-intel (release) — all success.

Note this run also carries #53's branches: ["**"], merged in from cpp-search, which is what
lets a PR based on feature/ci-maxmin-windows trigger R-CMD-check at all: the previous
branches: ["*"] does not match a base containing /, so with this PR stacked the workflow was
silently not running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shinytest2 test-Distribution.R snapshot is stale, and the diff changes the analytical conclusion (WideSample/MaxMin path)

1 participant