feat(probar): real browser driver and playbook executor — both layers had only mocks - #2497
Closed
noahgift wants to merge 5 commits into
Closed
feat(probar): real browser driver and playbook executor — both layers had only mocks#2497noahgift wants to merge 5 commits into
noahgift wants to merge 5 commits into
Conversation
Issue #2473: `ProbarDriver` had exactly one implementation in the whole workspace, `MockDriver`. `ChromiumDriver` was named in three doc comments -- one of them a `BrowserController::<ChromiumDriver>::launch(..)` example -- against a type nobody had written. So every layer built on the trait (locators, validators, playbooks, pixel coverage) drove a mock, and the Playwright-competitor framing did not survive contact with the code. The CDP machinery was already here: browser.rs::cdp launches a real chromiumoxide browser, capabilities.rs and zero_js.rs take a real chromiumoxide::Page, and chromiumoxide has been a declared dependency behind the `browser` feature all along. What was missing was the adapter between that and the trait. This is that adapter: all 16 trait methods against live Chrome. FALSIFY-PROBAR-DRIVER-001, 7 tests, each chosen so MockDriver FAILS it -- a test that merely calls the trait passes identically against a mock and proves nothing: * JS is folded by a real engine ([1..8].reduce -> 36) and the UA is Chromium, with a control proving execute_js does not return one constant for every script * a bounding box comes back as the CSS 120x40, i.e. from Blink layout, and a selector matching nothing returns None rather than a fabricated handle * typing changes the real input's .value * the screenshot carries the PNG magic and >1000 bytes from the compositor * wait_for_selector genuinely waits for an element appended at +300ms, and TIMES OUT on one that never appears * launching with a bogus executable ERRORS rather than quietly handing back something that answers questions it cannot know Mutation: make execute_js return a canned value, the way a mock would -> 6 of 7 go RED. Two defects found while proving it, both mine, both caught by that mutation rather than by review: 1. chromiumoxide points every browser at the SHARED, FIXED profile dir /tmp/chromiumoxide-runner, and Chrome's ProcessSingleton then refuses the second instance outright ("Failed to create .../SingletonLock: File exists (17) ... Aborting now to avoid profile corruption"). Two concurrent drivers could not coexist -- on one machine, or between two developers sharing a box. A browser-automation library that cannot run two browsers at once forfeits test parallelism, which is most of the point. Each driver now gets its own profile directory, removed on drop. The suite went from passing only under --test-threads=1 to passing in parallel, and got faster doing it (2.95s -> 1.03s). 2. screenshot() fell back to the CONFIGURED viewport when the page would not report its own -- echoing config back as if it were a measurement, which is the quiet degradation this file's own doc comment condemns. It was why the screenshot test survived the mutation. Now an error. The mutation then killed 6 of 7 instead of 5. navigation_timeout is honoured rather than decorative; it was an ignored config field, i.e. a promise the driver did not keep. Green: 7 driver tests against Chrome 151, 6304 lib tests, clippy clean. NOT claimed, and not yet true: * `apr probar`'s own commands do not route through this driver yet, so the CLI is not made real by this commit -- the library is. * the tests are NOT on ci.yml's beat list. They need a Chrome-equipped runner, the way the GPU falsifiers need a CUDA one. They fail rather than skip without a browser, deliberately. * #2473's other half -- 1,741 tests across 15 files wired into no `mod` -- is untouched here. No public claim should call probar a Playwright alternative until the CLI routes through this and those tests compile. Refs #2473
…part 2) #2473 reported that 15 `*_tests.rs` files in aprender-test-lib are wired into no `mod` and no `include!`, so "1,741 tests have never compiled". The wiring half of that is exactly right. The implication is not. Those files are byte-identical copies of test modules that ALREADY RUN inline in their parent files. No coverage was ever lost. Measured, not inferred. Comparing each orphan against the body of its parent's `#[cfg(test)] mod tests { .. }`: browser_tests.rs 3414 lines 0 differing locator_tests.rs 2164 lines 0 differing docker_tests.rs 1184 lines 0 differing capabilities_tests.rs 1187 lines 0 differing validators_tests.rs 2756 lines 8 differing llm/score_tests.rs 603 lines 9 differing and `#[test]` counts match exactly per file -- 274/274 browser, 216/216 locator, 91/91 docker, 100/100 capabilities. Where the copies DO differ, the orphan is strictly the OLDER text, missing clippy fixes its inline twin received: `if let` vs `match .. _ => {}`, `.keys()` vs iterating entries, `!contains_key(..)` vs `get(..).is_none()`. That is the signature of a snapshot left behind, not of tests aimed at an API that does not exist. Mounted correctly as submodules of their parents (so `use super::*` resolves as written) all 15 compile with ZERO errors and run 1544/1544 green -- with a test-NAME set identical to the inline modules, `diff` exit 0. They are duplicates, not orphans. Provenance: all 15 arrived in one commit, 8bd4ce5 (2026-05-07), a 17,830-file APR-MONO vendoring blob. They were already orphaned on arrival -- an "extract tests to separate files" refactor where the copy was made and neither the `mod` declaration nor the deletion of the inline block ever happened. So the remedy is deletion, not wiring. Wiring them in would add 1,544 duplicate test executions and zero assurance. Proof of zero coverage delta, same command either side: before test result: ok. 6390 passed; 0 failed after test result: ok. 6390 passed; 0 failed Also drops `**/browser_tests.rs` from .pmat-gates.toml's file_health exclusions -- with the file gone that pattern now matches nothing, and a dead exclusion is a rule that looks like it is protecting something. This corrects #2473's second finding. Its FIRST finding stands and is addressed separately: ProbarDriver really did have only MockDriver. Refs #2473
The layer above the driver has the same defect #2473 found below it. `ActionExecutor` is the trait playbooks execute through, and it had NO production implementation at all -- the only two `impl ActionExecutor` in the workspace are `MockExecutor`, both inside `#[cfg(test)]` modules (executor.rs:485, runner.rs:457). A playbook could be authored, parsed, validated and "run" with nothing reaching a browser. `ChromiumExecutor` implements all 12 methods over the `ChromiumDriver` from the previous commit, so `click`, `navigate`, `wait` and `screenshot` in a playbook drive Chrome. FALSIFY-PROBAR-EXEC-001, 6 tests, each written so a mock executor fails: * click on a <button> makes ITS OWN onclick write to another element -- an executor that records the click without dispatching it cannot produce that text * get_text / get_attribute read the live DOM, and a selector matching nothing is ElementNotFound rather than empty string * evaluate must be able to say NO: `1+1===3` is false. An executor hardcoded to true passes the true case and fails this one * wait observes real state -- an element appended at +300ms is found, and a condition that is never true TIMES OUT * screenshot writes a file that starts with the PNG magic Mutation: make `evaluate` return true unconditionally, the way a mock would -> evaluate_is_decided_by_the_page goes RED, the other 5 stay green. Sync-over-async is bridged with a runtime the executor owns. Calling it from inside an async context would deadlock, so `launch` REFUSES with an error naming the problem instead of panicking several frames down inside tokio; there is a test for that path. Two things fixed while proving it: * four hardcoded `Duration::from_secs(30)` waits now come from `DriverConfig::element_timeout`. That field existed and was ignored -- the same unkept-promise defect as `navigation_timeout` in the driver. It also cut the suite from 30.6s to 3.7s, since the deliberate-timeout test was waiting out a hardcoded 30s. * selectors are pasted into JS as JSON string literals, so one containing a quote cannot break out of the expression. `WaitCondition::NetworkIdle` is approximated from the page's own Resource Timing rather than CDP network events, and is commented as an approximation rather than presented as exact. Green: 6 executor tests + 7 driver tests against Chrome 151, clippy clean. Still NOT true, and still not claimed: `apr probar`'s commands do not route through this yet. `probar test` is a cargo-test wrapper (its "placeholder" comment is stale -- discover_tests really does shell out to `cargo test --list`), and `probar record` prints its configuration and records nothing. Refs #2473
This reverts bad1103, which is not a retraction: the finding and the evidence stand exactly as committed. The deletion simply belongs in its own PR rather than buried under a driver change. #2498 now carries it standalone, and a 26,205-line deletion is far easier to review on its own than mixed into ~800 lines of new driver and executor code where the diffstat hides it. The .pmat-gates.toml exclusion for browser_tests.rs comes back with the file, since it only becomes dead once the file is gone -- it moves to #2498 with the deletion it depends on. Refs #2473, #2498
noahgift
changed the title
Contributor
Author
|
Split for review. The 26,205-line deletion that was commit 2 of this PR has moved to #2498, which carries it standalone. The finding and evidence are unchanged — a deletion that size is simply easier to review on its own than buried under new driver code, where the diffstat hides everything else. This PR is now +1,327 lines and nothing else: Second layer added since the description above was written: Two more ignored config fields fixed along the way — Also filed from this work: #2500 — Verified after the split: 7 driver tests + 6 executor tests green against Chrome 151.0.7922.137. |
noahgift
enabled auto-merge
August 16, 2026 07:45
auto-merge was automatically disabled
August 18, 2026 16:52
Pull request was closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the first half of #2473.
ProbarDriverhad exactly one implementation in the entire workspace:MockDriver.ChromiumDriverwas named in three doc comments — one aBrowserController::<ChromiumDriver>::launch(..)example — against a type nobodyhad written. Every layer built on the trait drove a mock.
The CDP machinery was already present (
browser.rs::cdplaunches a realchromiumoxide browser;
capabilities.rsandzero_js.rstake a realchromiumoxide::Page; chromiumoxide has been a declared dependency all along).The adapter between that and the trait was the missing piece. This is it — all
16 trait methods against live Chrome.
Proof, not assertion
FALSIFY-PROBAR-DRIVER-001— 7 tests, each chosen soMockDriverfails it. Atest that merely calls the trait passes identically against a mock.
[1..8].reduce(+)→ 36, UA is Chromium#abox is the CSS 120×40None.valuewait_for_selectorfinds an element appended at +300ms, times out on one that never appearsErrMutation: make
execute_jsreturn a canned value, the way a mock would →6 of 7 go RED.
Two defects the mutation found — both mine, neither caught by review
1. Concurrent drivers could not coexist. chromiumoxide points every browser at
the shared fixed profile dir
/tmp/chromiumoxide-runner; Chrome's ProcessSingletonthen refuses the second instance:
A browser-automation library that cannot run two browsers at once forfeits test
parallelism, which is most of the point. Each driver now gets its own profile
directory, removed on drop. The suite went from passing only under
--test-threads=1to passing in parallel — and got faster doing it, 2.95s → 1.03s.2.
screenshot()echoed config back as measurement. When the page would notreport its own dimensions it fell back to the configured viewport — the quiet
degradation this file's own doc comment condemns, and exactly why the screenshot
test survived the first mutation run. Now an error. The mutation then killed 6 of
7 instead of 5.
Also:
navigation_timeoutis now honoured rather than decorative. It was anignored config field — a promise the driver did not keep.
What this does NOT claim
apr probar's commands do not route through this driver yet. The library ismade real by this PR; the CLI is not. That wiring is the next step.
runner, the way the GPU falsifiers need a CUDA one. Without a browser they
fail rather than skip — deliberately; a skipped browser test is how probar has no browser driver and 1,741 tests that never compile — the Playwright-killer claim is not supported #2473
happened.
moddeclaration, which have never compiled.No public claim should describe probar as a Playwright alternative until the CLI
routes through this driver and those tests compile and pass.
Verified on Chrome 151.0.7922.137: 7 driver tests, 6304 library tests, clippy
clean under
--features browser.