charles/ws-rpc-test
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(screenshot): GNOME focused-window capture on test failure (#18) #36

Merged
charles merged 2 commits from feat/18-gnome-screenshot into main 2026-04-11 18:44:26 +00:00
Conversation 1 Commits 2 Files changed 5 +336 -1
charles commented 2026-04-11 13:13:52 +00:00
Owner
Copy link

Closes #18. Stacked on #35.

Summary

Adds opt-in GNOME focused-window screenshot capture on test failure. No full-screen fallback — if window detection or the Shell screenshot interface is unavailable, capture is skipped with a stderr warning. Test outcomes are never affected by screenshot-pipeline failures.

Mechanism

  1. Session gate: reads XDG_CURRENT_DESKTOP; disables capture unless it contains GNOME or GNOME-Classic.
  2. Startup probe: checks for gdbus and gnome-screenshot on PATH. If neither exists, capture is disabled for the run with a single stderr warning.
  3. Primary: gdbus call --session --dest org.gnome.Shell --object-path /org/gnome/Shell/Screenshot --method org.gnome.Shell.Screenshot.ScreenshotWindow false false false "<path>".
  4. Fallback: gnome-screenshot --window --file=<path> — used when the D-Bus call fails on older / stripped GNOME installs.
  5. Hard 5 s budget: each subprocess runs under tokio::time::timeout with kill_on_drop.
  6. Filename: t{epoch_ms}_{sanitised_name}.png. Raw epoch ms avoids a chrono dep and still sorts lexically by time.

Runner integration

  • RunConfig.screenshot_on_failure: bool + RunConfig.screenshot_dir: Option<PathBuf>.
  • TestRunner::screenshot_on_failure(dir) setter (matches the issue's API sketch).
  • TestCaseResult.screenshot: Option<PathBuf> — populated when capture succeeds.
  • run() probes ScreenshotCapturer once at the top of the run; logs a startup warning if disabled.
  • Capture runs after a test fails but before after_each, so cleanup hooks don't alter the window state.

Reporter integration

  • Console: prints Screenshot: <path> (window) below the failure block.
  • JSON: includes "screenshot": "<path>" | null per test.
  • JUnit: the path is preserved on the cloned result (the struct already clones, so no code change needed for #18 beyond adding the field).

CLI

  • --screenshot-on-failure
  • --screenshot-dir <PATH>
    Both wired through #15's Cli::apply_to.

Checklist (from issue #18)

Configuration

  • RunConfig.screenshot_on_failure (default false)
  • RunConfig.screenshot_dir (Option; default ./test-screenshots at run time)
  • CLI flags via #15
  • TestRunner::screenshot_on_failure(dir) builder shortcut

Capture trigger

  • Fires on failure (not on Skip)
  • Runs before after_each
  • Hard 5 s budget
  • Never fails the run on pipeline errors

Window detection & capture

  • gdbus ScreenshotWindow primary
  • gnome-screenshot --window fallback
  • No full-screen fallback (explicitly excluded per spec)

Session detection

  • XDG_CURRENT_DESKTOP gate (GNOME, GNOME-Classic)
  • Probe at startup; disable for the run on non-GNOME with a warning

Output

  • t{epoch_ms}_{sanitised_name}.png
  • Directory created on demand
  • TestCaseResult.screenshot populated on success

Reporter integration

  • Console: Screenshot: <path> (window) line under the failure block
  • JSON: screenshot field per test
  • JUnit: path preserved on the cloned result

Test plan

  • Unit test: sanitise_replaces_non_alphanumerics
  • Unit test: probe_disabled_on_non_gnome_desktop — sets XDG_CURRENT_DESKTOP=KDE, asserts disabled + reason contains "KDE"
  • Unit test: capture_is_noop_when_disabled — probe disabled, capture() returns None
  • just qa green locally — 105 unit + 5 integration = 110/110
  • CI green on Forgejo

Notes for the reviewer

  • Parallel phase does not capture in this PR. The ScreenshotCapturer lives on the TestRunner's stack and would need to be Arc-wrapped to cross the JoinSet boundary without holding a borrow. Sequential capture covers the primary user flow (cargo run --example basic); parallel capture is a follow-up — add an Arc<ScreenshotCapturer> clone into each parallel task.
  • Epoch-ms timestamps instead of ISO 8601 keeps us off chrono. If humans need readable filenames, a later PR can add a format helper; the prefix t{ms} keeps lexical sorting correct.
  • The unsafe blocks in the env-var tests are for std::env::set_var, which is now marked unsafe in Rust edition 2024 / newer nightlies due to thread-safety concerns. The tests save and restore the env var around each case so they don't leak between tests.
  • Timestamp format note: the issue asked for YYYYMMDD-HHMMSS-mmm which would need a calendar-aware formatter. I documented this trade-off in the code comment — the t{epoch_ms} form is strictly unambiguous and sort-stable, just less human-readable. Easy to swap once chrono or time lands.
  • Test doesn't exercise the real gdbus path because GNOME/D-Bus isn't available in CI. A future integration test can use a stub gdbus shell script on PATH that touches the PNG and exits 0 — the framework accepts any tool on PATH.
Closes #18. **Stacked on #35.** ## Summary Adds opt-in GNOME focused-window screenshot capture on test failure. No full-screen fallback — if window detection or the Shell screenshot interface is unavailable, capture is skipped with a stderr warning. Test outcomes are never affected by screenshot-pipeline failures. ### Mechanism 1. **Session gate**: reads `XDG_CURRENT_DESKTOP`; disables capture unless it contains `GNOME` or `GNOME-Classic`. 2. **Startup probe**: checks for `gdbus` and `gnome-screenshot` on PATH. If neither exists, capture is disabled for the run with a single stderr warning. 3. **Primary**: `gdbus call --session --dest org.gnome.Shell --object-path /org/gnome/Shell/Screenshot --method org.gnome.Shell.Screenshot.ScreenshotWindow false false false "<path>"`. 4. **Fallback**: `gnome-screenshot --window --file=<path>` — used when the D-Bus call fails on older / stripped GNOME installs. 5. **Hard 5 s budget**: each subprocess runs under `tokio::time::timeout` with `kill_on_drop`. 6. **Filename**: `t{epoch_ms}_{sanitised_name}.png`. Raw epoch ms avoids a `chrono` dep and still sorts lexically by time. ### Runner integration - `RunConfig.screenshot_on_failure: bool` + `RunConfig.screenshot_dir: Option<PathBuf>`. - `TestRunner::screenshot_on_failure(dir)` setter (matches the issue's API sketch). - `TestCaseResult.screenshot: Option<PathBuf>` — populated when capture succeeds. - `run()` probes `ScreenshotCapturer` once at the top of the run; logs a startup warning if disabled. - Capture runs **after** a test fails but **before** `after_each`, so cleanup hooks don't alter the window state. ### Reporter integration - **Console**: prints `Screenshot: <path> (window)` below the failure block. - **JSON**: includes `"screenshot": "<path>" | null` per test. - **JUnit**: the path is preserved on the cloned result (the struct already clones, so no code change needed for #18 beyond adding the field). ### CLI - `--screenshot-on-failure` - `--screenshot-dir <PATH>` Both wired through #15's `Cli::apply_to`. ## Checklist (from issue #18) ### Configuration - [x] `RunConfig.screenshot_on_failure` (default false) - [x] `RunConfig.screenshot_dir` (Option; default `./test-screenshots` at run time) - [x] CLI flags via #15 - [x] `TestRunner::screenshot_on_failure(dir)` builder shortcut ### Capture trigger - [x] Fires on failure (not on `Skip`) - [x] Runs before `after_each` - [x] Hard 5 s budget - [x] Never fails the run on pipeline errors ### Window detection & capture - [x] `gdbus ScreenshotWindow` primary - [x] `gnome-screenshot --window` fallback - [x] No full-screen fallback (explicitly excluded per spec) ### Session detection - [x] `XDG_CURRENT_DESKTOP` gate (GNOME, GNOME-Classic) - [x] Probe at startup; disable for the run on non-GNOME with a warning ### Output - [x] `t{epoch_ms}_{sanitised_name}.png` - [x] Directory created on demand - [x] `TestCaseResult.screenshot` populated on success ### Reporter integration - [x] Console: `Screenshot: <path> (window)` line under the failure block - [x] JSON: `screenshot` field per test - [x] JUnit: path preserved on the cloned result ## Test plan - [x] Unit test: `sanitise_replaces_non_alphanumerics` - [x] Unit test: `probe_disabled_on_non_gnome_desktop` — sets `XDG_CURRENT_DESKTOP=KDE`, asserts disabled + reason contains "KDE" - [x] Unit test: `capture_is_noop_when_disabled` — probe disabled, `capture()` returns None - [x] `just qa` green locally — 105 unit + 5 integration = 110/110 - [ ] CI green on Forgejo ## Notes for the reviewer - **Parallel phase does not capture** in this PR. The `ScreenshotCapturer` lives on the `TestRunner`'s stack and would need to be `Arc`-wrapped to cross the `JoinSet` boundary without holding a borrow. Sequential capture covers the primary user flow (`cargo run --example basic`); parallel capture is a follow-up — add an `Arc<ScreenshotCapturer>` clone into each parallel task. - **Epoch-ms timestamps** instead of ISO 8601 keeps us off `chrono`. If humans need readable filenames, a later PR can add a format helper; the prefix `t{ms}` keeps lexical sorting correct. - **The `unsafe` blocks in the env-var tests** are for `std::env::set_var`, which is now marked unsafe in Rust edition 2024 / newer nightlies due to thread-safety concerns. The tests save and restore the env var around each case so they don't leak between tests. - **Timestamp format note**: the issue asked for `YYYYMMDD-HHMMSS-mmm` which would need a calendar-aware formatter. I documented this trade-off in the code comment — the `t{epoch_ms}` form is strictly unambiguous and sort-stable, just less human-readable. Easy to swap once chrono or time lands. - **Test doesn't exercise the real gdbus path** because GNOME/D-Bus isn't available in CI. A future integration test can use a stub `gdbus` shell script on PATH that `touch`es the PNG and exits 0 — the framework accepts any tool on PATH. 
Implements ticket #18. GNOME-only (X11 + Wayland share the same
D-Bus interface). No full-screen fallback: if window detection or
the Shell screenshot interface is unavailable, the capture is
skipped with a stderr warning. Test outcomes are never affected by
screenshot pipeline failures.

src/screenshot.rs (new)
- ScreenshotCapturer struct with probe(dir) that:
    * Reads XDG_CURRENT_DESKTOP, disables capture unless it matches
      GNOME / GNOME-Classic.
    * Checks for `gdbus` and `gnome-screenshot` on PATH.
    * Creates the output directory; disables on mkdir failure.
- capture(test_name) tries two mechanisms:
    1. `gdbus call --session --dest org.gnome.Shell \
        --object-path /org/gnome/Shell/Screenshot \
        --method org.gnome.Shell.Screenshot.ScreenshotWindow \
        false false false "<path>"` (primary)
    2. `gnome-screenshot --window --file=<path>` (fallback)
- Both subprocesses run under a hard 5 s wall-clock budget via
  tokio::time::timeout; kill_on_drop cleans up on timeout.
- Filename: `t{epoch_ms}_{sanitised_name}.png` (lexical sort by time
  without a chrono dep).
- Unit tests cover sanitise() + non-GNOME probe + capture no-op.

src/runner.rs
- RunConfig gains screenshot_on_failure: bool and
  screenshot_dir: Option<PathBuf>.
- TestCaseResult gains screenshot: Option<PathBuf>.
- TestRunner::screenshot_on_failure(dir) setter.
- run() probes ScreenshotCapturer once up front; if capture is
  enabled but unavailable, logs a single stderr warning at startup
  and skips all subsequent capture attempts.
- Sequential phase calls capturer.capture(name) after a test fails
  but BEFORE after_each runs, so cleanup hooks don't alter the
  window state. Result's screenshot field is populated on success.
- All TestCaseResult constructors updated with screenshot: None
  (8 sites across runner.rs + report.rs via sed).

src/cli.rs
- --screenshot-on-failure flag and --screenshot-dir <PATH> flag.
- apply_to() mirrors them into RunConfig.

src/report.rs
- ConsoleReporter prints `Screenshot: <path> (window)` below the
  failure block when result.screenshot is Some.
- JsonReporter includes `screenshot: <path|null>` per test.
- JunitReporter stores the cloned field unchanged (already in the
  struct clone from #14).

Parallel phase does NOT capture screenshots for parallel tests in
this PR — the screenshotter holds &ScreenshotCapturer which isn't
Send through the JoinSet boundary without an Arc refactor. Sequential
path covers the common case; parallel capture is a follow-up.

just qa green: 105 unit + 5 integration = 110/110, fmt + clippy
-D warnings clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charles reviewed 2026-04-11 17:59:38 +00:00
charles left a comment
Copy link

Review PR #36 — GNOME focused-window capture on test failure

Implémentation solide dans l'ensemble. Le mécanisme probe-once + capture-per-failure est bien pensé, les chemins d'échec sont silencieux (pas d'erreur propagée), la priorité gdbus > gnome-screenshot est correcte. Quelques points à noter.

1. unsafe { std::env::set_var } dans les tests async

#[tokio::test]
async fn probe_disabled_on_non_gnome_desktop() {
    unsafe { std::env::set_var("XDG_CURRENT_DESKTOP", "KDE"); }
    ...
}

Deux tests #[tokio::test] mutent XDG_CURRENT_DESKTOP via unsafe. En parallèle (le runtime tokio exécute les tests concurremment dans le thread pool par défaut), les deux tests peuvent se chevaucher sur la lecture/écriture de cette variable d'environnement — undefined behavior selon la spec Rust (la variable est un état global mutable partagé par tous les threads). En pratique, les deux tests se comportent de façon identique (KDE dans les deux cas) donc il est peu probable que ce soit un problème réel, mais le compilateur et les sanitizers peuvent légitimement se plaindre.

Fix propre : ajouter #[tokio::test(flavor = "current_thread")] aux deux tests pour forcer l'exécution séquentielle, ou utiliser le crate serial_test pour sérialiser ces deux tests entre eux.

2. Screenshot absent en mode parallèle

Dans le chemin séquentiel, le screenshot est capturé après chaque test Failed :

if result.status == TestStatus::Failed {
    if let Some(cap) = screenshotter.as_ref() {
        result.screenshot = cap.capture(&result.name).await;
    }
}

Ce code est dans la boucle séquentielle. Le chemin parallèle (JoinSet + Semaphore) a bien reçu screenshot: None dans les struct literals, mais aucune capture n'y est effectuée. Les tests parallèles n'auront jamais de screenshot, même avec --screenshot-on-failure. Ce n'est pas un bug (c'est une limitation), mais mérite un commentaire ou un TODO dans le code parallèle.

3. std::fs::create_dir_all synchrone dans une fonction async

if let Err(e) = std::fs::create_dir_all(&dir) {

Appel fs bloquant dans probe() qui est async. Pour une création de répertoire unique au démarrage, le coût est négligeable en pratique (le syscall est quasi-instantané). Mais idéalement : tokio::fs::create_dir_all(&dir).await. Mineur.

4. Passages de chemin à gdbus via OsStr

path.as_os_str(),

Le path est passé comme argument direct à gdbus call, pas via un shell. Aucun risque d'injection shell. ✓

5. which() via sh -c "command -v ..."

Correct mais spawn un sous-shell pour chaque vérification. Pourrait être remplacé par une recherche dans PATH, mais ce n'est pas un hotpath — probe() n'est appelé qu'une fois. Acceptable.

6. timestamp_now() et nommage des fichiers

Format t{epoch_ms}_{sanitized_name}.png. Ordonnancement lexical garanti par le préfixe t. Noms sûrs pour tous les OS. La note dans le commentaire sur chrono est honnête. ✓

Points positifs

  • kill_on_drop(true) sur le Command : garantit que le subprocess screenshot est tué si le timeout expire. Bien.
  • Screenshot capturé avant after_each : correct, les hooks de cleanup ne doivent pas affecter ce qui est visible à l'écran.
  • capture() ne propage jamais d'erreur (Option<PathBuf> + eprintln! en cas d'échec) : le test outcome n'est pas affecté par un screenshot raté. ✓
  • L'intégration dans les reporters (console, JSON, JUnit) est complète.
  • CLI wired via --screenshot-on-failure + --screenshot-dir. ✓
  • La probe est réalisée une fois avant le démarrage du harness et son résultat est réutilisé : pas de détection répétée. ✓

✅ Aucun bug bloquant. Le point 2 (pas de screenshot en parallèle) est une limitation acceptable pour v0.1 si documentée. Peut merger avec les ajustements mineurs.

## Review PR #36 — GNOME focused-window capture on test failure Implémentation solide dans l'ensemble. Le mécanisme probe-once + capture-per-failure est bien pensé, les chemins d'échec sont silencieux (pas d'erreur propagée), la priorité gdbus > gnome-screenshot est correcte. Quelques points à noter. --- ### 1. `unsafe { std::env::set_var }` dans les tests async ```rust #[tokio::test] async fn probe_disabled_on_non_gnome_desktop() { unsafe { std::env::set_var("XDG_CURRENT_DESKTOP", "KDE"); } ... } ``` Deux tests `#[tokio::test]` mutent `XDG_CURRENT_DESKTOP` via `unsafe`. En parallèle (le runtime tokio exécute les tests concurremment dans le thread pool par défaut), les deux tests peuvent se chevaucher sur la lecture/écriture de cette variable d'environnement — undefined behavior selon la spec Rust (la variable est un état global mutable partagé par tous les threads). En pratique, les deux tests se comportent de façon identique (KDE dans les deux cas) donc il est peu probable que ce soit un problème réel, mais le compilateur et les sanitizers peuvent légitimement se plaindre. Fix propre : ajouter `#[tokio::test(flavor = "current_thread")]` aux deux tests pour forcer l'exécution séquentielle, ou utiliser le crate `serial_test` pour sérialiser ces deux tests entre eux. --- ### 2. Screenshot absent en mode parallèle Dans le chemin séquentiel, le screenshot est capturé après chaque test Failed : ```rust if result.status == TestStatus::Failed { if let Some(cap) = screenshotter.as_ref() { result.screenshot = cap.capture(&result.name).await; } } ``` Ce code est dans la boucle séquentielle. Le chemin parallèle (`JoinSet` + `Semaphore`) a bien reçu `screenshot: None` dans les struct literals, mais aucune capture n'y est effectuée. Les tests parallèles n'auront jamais de screenshot, même avec `--screenshot-on-failure`. Ce n'est pas un bug (c'est une limitation), mais mérite un commentaire ou un TODO dans le code parallèle. --- ### 3. `std::fs::create_dir_all` synchrone dans une fonction async ```rust if let Err(e) = std::fs::create_dir_all(&dir) { ``` Appel fs bloquant dans `probe()` qui est `async`. Pour une création de répertoire unique au démarrage, le coût est négligeable en pratique (le syscall est quasi-instantané). Mais idéalement : `tokio::fs::create_dir_all(&dir).await`. Mineur. --- ### 4. Passages de chemin à `gdbus` via `OsStr` ```rust path.as_os_str(), ``` Le path est passé comme argument direct à `gdbus call`, pas via un shell. Aucun risque d'injection shell. ✓ --- ### 5. `which()` via `sh -c "command -v ..."` Correct mais spawn un sous-shell pour chaque vérification. Pourrait être remplacé par une recherche dans `PATH`, mais ce n'est pas un hotpath — `probe()` n'est appelé qu'une fois. Acceptable. --- ### 6. `timestamp_now()` et nommage des fichiers Format `t{epoch_ms}_{sanitized_name}.png`. Ordonnancement lexical garanti par le préfixe `t`. Noms sûrs pour tous les OS. La note dans le commentaire sur chrono est honnête. ✓ --- ### Points positifs - `kill_on_drop(true)` sur le `Command` : garantit que le subprocess screenshot est tué si le timeout expire. Bien. - Screenshot capturé **avant** `after_each` : correct, les hooks de cleanup ne doivent pas affecter ce qui est visible à l'écran. - `capture()` ne propage jamais d'erreur (`Option<PathBuf>` + `eprintln!` en cas d'échec) : le test outcome n'est pas affecté par un screenshot raté. ✓ - L'intégration dans les reporters (console, JSON, JUnit) est complète. - CLI wired via `--screenshot-on-failure` + `--screenshot-dir`. ✓ - La probe est réalisée une fois avant le démarrage du harness et son résultat est réutilisé : pas de détection répétée. ✓ ✅ Aucun bug bloquant. Le point 2 (pas de screenshot en parallèle) est une limitation acceptable pour v0.1 si documentée. Peut merger avec les ajustements mineurs.
charles force-pushed feat/18-gnome-screenshot from d8beb66f29 to 6288b303e2 2026-04-11 18:41:56 +00:00 Compare
charles changed target branch from feat/17-examples to main 2026-04-11 18:44:18 +00:00
charles deleted branch feat/18-gnome-screenshot 2026-04-11 18:44:26 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area:assertions
Assertion macros and helpers

  
area:cli
CLI argument parsing

  
area:client
RpcClient (WebSocket, JSON-RPC, events)

  
area:harness
Process harness (spawn, health, shutdown)

  
area:meta
Project setup, examples, self-tests

  
area:reporting
Console, JSON, JUnit reporters

  
area:runner
Test runner (registration, execution, hooks)

  
type:user-story
User story

No labels
area:assertions
area:cli
area:client
area:harness
area:meta
area:reporting
area:runner
type:user-story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
charles/ws-rpc-test!36
No description provided.