diff --git a/.config/jp/tools/src/lib.rs b/.config/jp/tools/src/lib.rs index 940aca547..71eb06597 100644 --- a/.config/jp/tools/src/lib.rs +++ b/.config/jp/tools/src/lib.rs @@ -6,6 +6,7 @@ mod fs; mod git; mod github; mod plan; +mod swift; mod ticket; mod unix; mod util; @@ -26,6 +27,7 @@ pub async fn run(ctx: Context, t: Tool) -> util::ToolResult { s if s.starts_with("debug_jp_") => debug_jp::run(ctx, t).await, s if s.starts_with("web_") => web::run(ctx, t).await, s if s.starts_with("git_") => git::run(ctx, t).await, + s if s.starts_with("swift_") => swift::run(ctx, t).await, s if s.starts_with("unix_") => unix::run(ctx, t), s if s.starts_with("ticket_") => ticket::run(ctx, t), "plan" => plan::run(ctx, t), diff --git a/.config/jp/tools/src/swift.rs b/.config/jp/tools/src/swift.rs new file mode 100644 index 000000000..857bf9255 --- /dev/null +++ b/.config/jp/tools/src/swift.rs @@ -0,0 +1,175 @@ +//! Tools for the macOS app in `apps/macos`. +//! +//! These mirror the `cargo_*` tools: each shells out to a toolchain binary from +//! the repository root and reports diagnostics rather than raw build logs. +//! +//! Every tool that builds brings its inputs up to date first, through +//! [`prepare`], so a fresh checkout needs no setup step. + +use jp_tool::Context; + +use crate::{ + Tool, + util::{ + ToolResult, + runner::{ProcessOutput, ProcessRunner}, + truncate, unknown_tool, + }, +}; + +mod check; +mod format; +mod report; +mod test; +mod test_ui; + +use check::swift_check; +use format::swift_format; +use test::swift_test; +use test_ui::swift_test_ui; + +/// Cap for compiler diagnostics embedded in a tool result. +/// +/// `xcodebuild` repeats the failing command line in full for every error, so +/// the tail of a broken build is almost entirely noise. +const MAX_DIAGNOSTIC_BYTES: usize = 32_000; + +/// The generated Xcode project, relative to the repository root. +const PROJECT_PATH: &str = "apps/macos/JP.xcodeproj"; + +/// The `XcodeGen` manifest the project is generated from. +const PROJECT_SPEC: &str = "apps/macos/project.yml"; + +/// The directory the generated project is written into. +const PROJECT_DIR: &str = "apps/macos"; + +/// The scheme covering the app and its test bundle. +const SCHEME: &str = "JP"; + +/// Swift sources formatted and linted by these tools. +const SOURCE_PATHS: &[&str] = &[ + "apps/macos/Sources", + "apps/macos/Tests", + "apps/macos/UITests", + "apps/macos/Tools/jpdrive/Sources", + "apps/macos/Tools/jpdrive/Tests", +]; + +pub async fn run(ctx: Context, t: Tool) -> ToolResult { + match t.name.trim_start_matches("swift_") { + "check" => swift_check(&ctx, t.opt("configuration")?).await, + "test" => swift_test(&ctx, t.opt("testname")?, t.opt("target")?).await, + "test_ui" => swift_test_ui(&ctx, t.opt("tests")?).await, + // `check` is a call parameter, not a tool config option, so it is read + // with `opt` rather than `option_or`. + "format" => swift_format(&ctx, t.opt("check")?.unwrap_or(false)).await, + _ => unknown_tool(t), + } +} + +/// Build the generated inputs an Xcode build depends on. +/// +/// Returns a failure message, or `None` when both steps succeeded. +/// +/// Two things are generated rather than committed: the static library with its +/// C header, and the Xcode project. +/// Both steps are idempotent and cheap when already up to date. +/// +/// The header cannot be left to the project's own build phase. +/// Xcode scans the bridging header while planning the build, before any script +/// phase runs, so a missing header fails the scan rather than triggering the +/// phase that would have produced it. +fn prepare( + ctx: &Context, + profile: &str, + runner: &R, +) -> Result, std::io::Error> { + // Going through `just` keeps the profile-to-directory mapping and the + // cbindgen invocation defined in one place. + let ffi = runner.run("just", &["build-ffi", profile], &ctx.root)?; + if !ffi.status.is_success() { + return Ok(Some(format!( + "Building `jp_ffi` failed:\n\n```\n{}\n```", + report(&ffi, "just build-ffi") + ))); + } + + let project = match runner.run( + "xcodegen", + &["generate", "--spec", PROJECT_SPEC, "--project", PROJECT_DIR], + &ctx.root, + ) { + Ok(project) => project, + + // A binary that is not installed fails to spawn rather than running and + // exiting non-zero, so it never reaches the branch below. The runner's + // `unchecked` suppresses a bad exit status, not a failure to start. + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + return Ok(Some( + "`xcodegen` is not installed. The Xcode project is generated from \ + `apps/macos/project.yml` rather than committed, so it is needed to build the app \ + at all. Install it with `brew install xcodegen`." + .to_owned(), + )); + } + + Err(error) => return Err(error), + }; + if !project.status.is_success() { + return Ok(Some(format!( + "Generating `{PROJECT_PATH}` failed:\n\n```\n{}\n```", + report(&project, "xcodegen") + ))); + } + + Ok(None) +} + +/// The diagnostics from a process, preferring stdout and falling back to +/// stderr. +/// +/// `xcodebuild` reports compiler diagnostics on stdout and its own failures on +/// stderr; most other tools use stderr for both. +/// `label` names the program for the case where it printed nothing at all. +fn report(output: &ProcessOutput, label: &str) -> String { + let stdout = strip(&output.stdout); + if !stdout.is_empty() { + return stdout; + } + + let stderr = strip(&output.stderr); + if !stderr.is_empty() { + return stderr; + } + + format!( + "{label} exited with status {} and no diagnostics.", + output.status + ) +} + +/// Strip ANSI escapes and trim, capping the result. +fn strip(output: &str) -> String { + let stripped = strip_ansi_escapes::strip_str(output); + truncate(stripped.trim(), MAX_DIAGNOSTIC_BYTES) +} + +/// The message from a failed outcome. +/// +/// Failures come back as `Ok(Outcome::Error { .. })` rather than `Err`, so +/// tests have to reach into the outcome to assert on the message. +/// +/// # Panics +/// +/// Panics if the outcome is not an error. +#[cfg(test)] +fn error_message(outcome: jp_tool::Outcome) -> String { + match outcome { + jp_tool::Outcome::Error { message, .. } => message, + other => panic!("expected an error outcome, got {other:?}"), + } +} + +#[cfg(test)] +#[path = "swift_tests.rs"] +mod tests; diff --git a/.config/jp/tools/src/swift/check.rs b/.config/jp/tools/src/swift/check.rs new file mode 100644 index 000000000..5258d2990 --- /dev/null +++ b/.config/jp/tools/src/swift/check.rs @@ -0,0 +1,71 @@ +use jp_tool::Context; + +use super::{PROJECT_PATH, SCHEME, prepare, report, strip}; +use crate::util::{ + ToolResult, error, + runner::{DuctProcessRunner, ProcessRunner}, +}; + +pub(crate) async fn swift_check(ctx: &Context, configuration: Option) -> ToolResult { + swift_check_impl(ctx, configuration.as_deref(), &DuctProcessRunner) +} + +fn swift_check_impl( + ctx: &Context, + configuration: Option<&str>, + runner: &R, +) -> ToolResult { + let configuration = configuration.unwrap_or("Debug"); + // Cargo's profile directory for a configuration, matching `CARGO_PROFILE` in + // the Xcode project. + let profile = if configuration == "Release" { + "release" + } else { + "debug" + }; + + if let Some(failure) = prepare(ctx, profile, runner)? { + return error(failure); + } + + let output = runner.run( + "xcodebuild", + &[ + "build", + "-project", + PROJECT_PATH, + "-scheme", + SCHEME, + "-configuration", + configuration, + "-destination", + "platform=macOS", + // Nothing is run or installed, so skip signing and its keychain + // prompts. + "CODE_SIGNING_ALLOWED=NO", + "-quiet", + ], + &ctx.root, + )?; + + let diagnostics = strip(&output.stdout); + + if output.status.is_success() { + return Ok(if diagnostics.is_empty() { + "Build succeeded. No warnings or errors found." + .to_owned() + .into() + } else { + format!("```\n{diagnostics}\n```\n").into() + }); + } + + error(format!( + "Swift build failed:\n\n```\n{}\n```", + report(&output, "xcodebuild") + )) +} + +#[cfg(test)] +#[path = "check_tests.rs"] +mod tests; diff --git a/.config/jp/tools/src/swift/check_tests.rs b/.config/jp/tools/src/swift/check_tests.rs new file mode 100644 index 000000000..627c6a5de --- /dev/null +++ b/.config/jp/tools/src/swift/check_tests.rs @@ -0,0 +1,201 @@ +use jp_tool::{Action, Context}; +use pretty_assertions::assert_eq; + +use super::{super::error_message, *}; +use crate::util::runner::{ExitCode, MockProcessRunner, ProcessOutput}; + +/// The tools pass `ctx.root` straight to the runner and never touch the +/// filesystem, so a fixed path is enough. +fn ctx() -> Context { + Context { + root: "/repo".into(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + } +} + +/// Expect the two preparation steps every build runs first. +fn prepared(profile: &str) -> MockProcessRunner { + MockProcessRunner::builder() + .expect("just") + .args(&["build-ffi", profile]) + .returns_success("") + .expect("xcodegen") + .args(&[ + "generate", + "--spec", + "apps/macos/project.yml", + "--project", + "apps/macos", + ]) + .returns_success("") +} + +#[test] +fn builds_the_debug_configuration_by_default() { + let runner = prepared("debug") + .expect("xcodebuild") + .args(&[ + "build", + "-project", + "apps/macos/JP.xcodeproj", + "-scheme", + "JP", + "-configuration", + "Debug", + "-destination", + "platform=macOS", + "CODE_SIGNING_ALLOWED=NO", + "-quiet", + ]) + .returns_success(""); + + let result = swift_check_impl(&ctx(), None, &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + "Build succeeded. No warnings or errors found." + ); +} + +/// A Release build links the library from cargo's `release` directory, so the +/// preparation step has to build that profile rather than the default. +#[test] +fn builds_the_release_library_for_a_release_build() { + let runner = prepared("release") + .expect("xcodebuild") + .args(&[ + "build", + "-project", + "apps/macos/JP.xcodeproj", + "-scheme", + "JP", + "-configuration", + "Release", + "-destination", + "platform=macOS", + "CODE_SIGNING_ALLOWED=NO", + "-quiet", + ]) + .returns_success(""); + + let result = swift_check_impl(&ctx(), Some("Release"), &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + "Build succeeded. No warnings or errors found." + ); +} + +/// Diagnostics from a passing build still reach the caller: the project treats +/// warnings as errors, so anything reported here is worth reading. +#[test] +fn reports_diagnostics_from_a_passing_build() { + let runner = prepared("debug") + .expect("xcodebuild") + .returns_success("note: some advice from the compiler"); + + let result = swift_check_impl(&ctx(), None, &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + "```\nnote: some advice from the compiler\n```\n" + ); +} + +/// `xcodebuild` reports compiler diagnostics on stdout, not stderr. +#[test] +fn reports_compiler_errors_from_stdout() { + let runner = prepared("debug") + .expect("xcodebuild") + .returns(ProcessOutput { + stdout: "WorkspaceReader.swift:12:5: error: cannot find 'nope' in scope".to_owned(), + stderr: String::new(), + status: ExitCode::from_code(65), + }); + + let message = error_message(swift_check_impl(&ctx(), None, &runner).unwrap()); + + assert_eq!( + message, + "Swift build failed:\n\n```\nWorkspaceReader.swift:12:5: error: cannot find 'nope' in \ + scope\n```" + ); +} + +/// A non-zero exit with nothing on either stream still has to say something. +#[test] +fn reports_a_bare_failure() { + let runner = prepared("debug") + .expect("xcodebuild") + .returns(ProcessOutput { + stdout: String::new(), + stderr: String::new(), + status: ExitCode::from_code(70), + }); + + let message = error_message(swift_check_impl(&ctx(), None, &runner).unwrap()); + + assert_eq!( + message, + "Swift build failed:\n\n```\nxcodebuild exited with status 70 and no diagnostics.\n```" + ); +} + +/// A failure building the library stops before `xcodebuild`, which would +/// otherwise fail on a missing header and bury the real cause. +#[test] +fn stops_when_the_library_fails_to_build() { + let runner = MockProcessRunner::builder() + .expect("just") + .returns_error("error[E0425]: cannot find value `nope` in this scope"); + + let message = error_message(swift_check_impl(&ctx(), None, &runner).unwrap()); + + assert_eq!( + message, + "Building `jp_ffi` failed:\n\n```\nerror[E0425]: cannot find value `nope` in this \ + scope\n```" + ); +} + +/// xcodegen is the one tool here that is not part of the Swift toolchain, so +/// its absence gets an install hint. +/// +/// A binary that is not installed fails to *spawn*. +/// Modelling it as a command that ran and exited non-zero tests the wrong +/// branch entirely, and passes while the real case reaches an unhandled +/// `io::Error` carrying no hint. +#[test] +fn points_at_homebrew_when_xcodegen_is_missing() { + let runner = MockProcessRunner::builder() + .expect("just") + .returns_success("") + .expect("xcodegen") + .fails_to_spawn(); + + let message = error_message(swift_check_impl(&ctx(), None, &runner).unwrap()); + + assert!(message.contains("brew install xcodegen"), "got: {message}"); +} + +/// xcodegen present but refusing the manifest is a different failure, and the +/// install hint would be wrong: it is already installed. +#[test] +fn reports_what_xcodegen_said_when_it_ran_and_failed() { + let runner = MockProcessRunner::builder() + .expect("just") + .returns_success("") + .expect("xcodegen") + .returns_error("Spec parsing error: unknown target JPUITests"); + + let message = error_message(swift_check_impl(&ctx(), None, &runner).unwrap()); + + assert!( + message.contains("unknown target JPUITests"), + "got: {message}" + ); + assert!(!message.contains("brew install"), "got: {message}"); +} diff --git a/.config/jp/tools/src/swift/format.rs b/.config/jp/tools/src/swift/format.rs new file mode 100644 index 000000000..c9435b202 --- /dev/null +++ b/.config/jp/tools/src/swift/format.rs @@ -0,0 +1,60 @@ +use jp_tool::Context; + +use super::{SOURCE_PATHS, strip}; +use crate::util::{ + ToolResult, error, + runner::{DuctProcessRunner, ProcessOutput, ProcessRunner}, +}; + +pub(crate) async fn swift_format(ctx: &Context, check: bool) -> ToolResult { + swift_format_impl(ctx, check, &DuctProcessRunner) +} + +fn swift_format_impl(ctx: &Context, check: bool, runner: &R) -> ToolResult { + // `swift format lint` reports rule violations without rewriting; the default + // subcommand rewrites but does not lint. Both read `apps/macos/.swift-format`. + let mut args = if check { + vec!["format", "lint", "--strict"] + } else { + vec!["format", "--in-place"] + }; + args.extend(["--recursive", "--parallel"]); + args.extend(SOURCE_PATHS); + + // `swift format` reports on stderr; stdout carries rewritten source only when + // writing to a pipe, which `--in-place` and `lint` never do. + let ProcessOutput { stderr, status, .. } = runner.run("swift", &args, &ctx.root)?; + + if check { + let findings = strip(&stderr); + return if status.is_success() && findings.is_empty() { + Ok("Swift sources are correctly formatted.".to_owned().into()) + } else { + error(format!( + "Swift formatting or lint violations:\n\n```\n{findings}\n```" + )) + }; + } + + if !status.is_success() { + return error(format!("swift format failed: {}", strip(&stderr))); + } + + // `--in-place` is silent: it neither lists what it rewrote nor says that it + // rewrote nothing. So this reports what was formatted, and claims nothing + // about what changed — `swift_format` with `check` set is what answers that. + let diagnostics = strip(&stderr); + Ok(if diagnostics.is_empty() { + format!("Formatted {}.", SOURCE_PATHS.join(", ")).into() + } else { + format!( + "Formatted {}.\n\n```\n{diagnostics}\n```", + SOURCE_PATHS.join(", ") + ) + .into() + }) +} + +#[cfg(test)] +#[path = "format_tests.rs"] +mod tests; diff --git a/.config/jp/tools/src/swift/format_tests.rs b/.config/jp/tools/src/swift/format_tests.rs new file mode 100644 index 000000000..4f12e8094 --- /dev/null +++ b/.config/jp/tools/src/swift/format_tests.rs @@ -0,0 +1,151 @@ +use jp_tool::{Action, Context}; +use pretty_assertions::assert_eq; + +use super::{super::error_message, *}; +use crate::util::runner::{ExitCode, MockProcessRunner, ProcessOutput}; + +fn ctx() -> Context { + Context { + root: "/repo".into(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + } +} + +#[test] +fn rewrites_sources_in_place() { + let runner = MockProcessRunner::builder() + .expect("swift") + .args(&[ + "format", + "--in-place", + "--recursive", + "--parallel", + "apps/macos/Sources", + "apps/macos/Tests", + "apps/macos/UITests", + "apps/macos/Tools/jpdrive/Sources", + "apps/macos/Tools/jpdrive/Tests", + ]) + .returns_success(""); + + let result = swift_format_impl(&ctx(), false, &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + "Formatted apps/macos/Sources, apps/macos/Tests, apps/macos/UITests, \ + apps/macos/Tools/jpdrive/Sources, apps/macos/Tools/jpdrive/Tests." + ); +} + +/// `swift format --in-place` says nothing at all, whether it rewrote every file +/// or none. +/// Claiming "no files to format" from that silence would be a guess, and a +/// wrong one whenever it did rewrite something. +#[test] +fn does_not_claim_nothing_changed() { + let runner = MockProcessRunner::success(""); + + let result = swift_format_impl(&ctx(), false, &runner).unwrap(); + + assert!( + !result.unwrap_content().contains("No files"), + "the formatter cannot know whether anything changed, so it must not say" + ); +} + +/// Anything the formatter does say is passed along, since it only speaks up +/// when something is wrong. +#[test] +fn relays_what_the_formatter_reported() { + let runner = MockProcessRunner::builder() + .expect("swift") + .returns(ProcessOutput { + stdout: String::new(), + stderr: "JPApp.swift:3:1: warning: [Indentation] unexpected indentation".to_owned(), + status: ExitCode::success(), + }); + + let result = swift_format_impl(&ctx(), false, &runner).unwrap(); + + assert!( + result.unwrap_content().contains("unexpected indentation"), + "expected the formatter's own output to be relayed" + ); +} + +#[test] +fn lints_without_rewriting_in_check_mode() { + let runner = MockProcessRunner::builder() + .expect("swift") + .args(&[ + "format", + "lint", + "--strict", + "--recursive", + "--parallel", + "apps/macos/Sources", + "apps/macos/Tests", + "apps/macos/UITests", + "apps/macos/Tools/jpdrive/Sources", + "apps/macos/Tools/jpdrive/Tests", + ]) + .returns_success(""); + + let result = swift_format_impl(&ctx(), true, &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + "Swift sources are correctly formatted." + ); +} + +#[test] +fn reports_lint_violations_in_check_mode() { + let runner = MockProcessRunner::builder() + .expect("swift") + .returns(ProcessOutput { + stdout: String::new(), + stderr: "JPApp.swift:3:1: warning: [NeverForceUnwrap] do not force unwrap".to_owned(), + status: ExitCode::from_code(1), + }); + + let message = error_message(swift_format_impl(&ctx(), true, &runner).unwrap()); + + assert_eq!( + message, + "Swift formatting or lint violations:\n\n```\nJPApp.swift:3:1: warning: \ + [NeverForceUnwrap] do not force unwrap\n```" + ); +} + +/// A clean exit with findings on stderr is still a violation: `--strict` makes +/// the findings meaningful, and trusting the exit status alone would hide them. +#[test] +fn treats_findings_as_violations_even_on_a_clean_exit() { + let runner = MockProcessRunner::builder() + .expect("swift") + .returns(ProcessOutput { + stdout: String::new(), + stderr: "JPApp.swift:3:1: warning: [Indentation] unexpected indentation".to_owned(), + status: ExitCode::success(), + }); + + let message = error_message(swift_format_impl(&ctx(), true, &runner).unwrap()); + + assert!(message.contains("Indentation"), "got: {message}"); +} + +#[test] +fn reports_a_formatter_failure() { + let runner = MockProcessRunner::error("error: unknown option '--nope'"); + + let message = error_message(swift_format_impl(&ctx(), false, &runner).unwrap()); + + assert_eq!( + message, + "swift format failed: error: unknown option '--nope'" + ); +} diff --git a/.config/jp/tools/src/swift/report.rs b/.config/jp/tools/src/swift/report.rs new file mode 100644 index 000000000..d57e573d0 --- /dev/null +++ b/.config/jp/tools/src/swift/report.rs @@ -0,0 +1,665 @@ +//! Turning a test runner's output into something worth reading. +//! +//! Shared by [`swift_test`] and [`swift_test_ui`], which run different bundles +//! through different filters but report their results the same way. +//! +//! [`swift_test_ui`]: super::test_ui +//! [`swift_test`]: super::test + +use std::fs; + +use camino::{Utf8Path, Utf8PathBuf}; +use jp_tool::Context; +use serde_json::Value; + +use super::strip; +use crate::util::runner::{ProcessOutput, ProcessRunner}; + +/// How much of the raw log to show when there is no summary to show instead. +const LOG_TAIL_LINES: usize = 40; + +/// The app's unit-test bundle, hosted by the app process. +const UNIT_BUNDLE: &str = "JPTests"; + +/// The app's UI-test bundle, which drives the app from outside it. +pub(super) const UI_BUNDLE: &str = "JPUITests"; + +/// An `-only-testing` argument addressing the unit bundle. +/// +/// `-only-testing` takes `//`; the bundle alone narrows a +/// run to everything in it. +pub(super) fn unit_bundle_filter(testname: Option<&str>) -> String { + match testname { + Some(name) => format!("-only-testing:{UNIT_BUNDLE}/{name}"), + None => format!("-only-testing:{UNIT_BUNDLE}"), + } +} + +/// An `-only-testing` argument addressing one test or suite in the UI bundle. +pub(super) fn ui_bundle_filter(test: &str) -> String { + format!("-only-testing:{UI_BUNDLE}/{test}") +} + +/// The labelled summary of a run, or the message explaining why it is not one. +/// +/// A run that reported no summary is not a passing run, it is a run that did +/// nothing: every runner here can exit zero when a filter matches nothing at +/// all. +pub(super) fn outcome(output: &ProcessOutput, label: &str) -> Result { + // Scan the whole log before capping it. A non-quiet run is far longer than + // the diagnostic cap and the summary is at the end, so truncating first would + // throw away the only lines worth reading. + let log = Log::from(output); + let summary = log.summary(); + + if !output.status.is_success() { + let detail = [log.crashes(), log.failures(), summary] + .into_iter() + .filter(|section| !section.is_empty()) + .collect::>() + .join("\n"); + + if !detail.is_empty() { + return Err(format!( + "{label} tests failed:\n\n```\n{}\n```", + strip(&detail) + )); + } + + // Neither a failing test nor a summary means the run did not get far enough + // to report either. Showing the head of the log here would show the build + // starting; whatever went wrong is at the other end. + return Err(format!( + "{label} tests failed without reporting a failing test or a summary, so the run died \ + rather than failing. The end of its output:\n\n```\n{}\n```", + log.tail(LOG_TAIL_LINES) + )); + } + + if summary.is_empty() { + return Err(format!( + "{label} exited successfully but reported no test summary, so no test ran. A name \ + that matches nothing in the bundle it was pointed at will do this. The end of its \ + output:\n\n```\n{}\n```", + log.tail(LOG_TAIL_LINES) + )); + } + + // A run can cover more than one bundle and so report more than one summary. + // Labelling each keeps an unlabelled second line from reading as output + // from somewhere else. + Ok(summary + .lines() + .map(|line| format!("{label}: {line}")) + .collect::>() + .join("\n")) +} + +/// The UI test bundle's identifier, as `apps/macos/project.yml` sets it. +const UI_BUNDLE_ID: &str = "computer.jp.jean-pierre.uitests"; + +/// Where collected screenshots are put, relative to the repository root. +const SCREENSHOT_DIR: &str = "tmp/uitests"; + +/// Where the UI test runner writes its failure screenshots. +/// +/// Xcode wraps a UI test bundle in a generated, sandboxed runner app, so the +/// tests cannot write into the checkout and put their screenshots in the +/// container's temporary directory instead. +/// Nothing reports that path, so it is derived the same way the runner does: +/// from its bundle identifier, which is the test bundle's with `.xctrunner` +/// appended. +fn screenshot_source() -> Option { + let home = std::env::var("HOME").ok()?; + + Some(Utf8PathBuf::from(home).join(format!( + "Library/Containers/{UI_BUNDLE_ID}.xctrunner/Data/tmp/jp-uitests" + ))) +} + +/// Discard screenshots left by an earlier run. +pub(super) fn clear_screenshots() { + if let Some(dir) = screenshot_source() { + let _removed = fs::remove_dir_all(&dir); + } +} + +/// The file the UI tests write their failure messages to. +const FAILURE_LOG: &str = "failures.txt"; + +/// Where `xcodebuild` is told to write the result bundle for a UI run. +/// +/// Inside the checkout and under `tmp/`, so it is reachable without deriving a +/// container path and is thrown away with the rest of the scratch directory. +pub(super) const RESULT_BUNDLE: &str = "tmp/uitests/run.xcresult"; + +/// Discard the result bundle an earlier run left behind. +/// +/// `xcodebuild` refuses to write over an existing bundle, so this is not +/// tidying up: without it the second UI run in a checkout fails before it +/// starts. +pub(super) fn clear_result_bundle(root: &Utf8Path) { + let _removed = fs::remove_dir_all(root.join(RESULT_BUNDLE)); +} + +/// The file the test runner's own output is staged into. +const RUNNER_OUTPUT: &str = "StandardOutputAndStandardError.txt"; + +/// The line swift-testing writes when an expectation fails. +const ISSUE_MARKER: &str = "recorded an issue at"; + +/// What the tests reported, taken from the runner's output in the result +/// bundle. +/// +/// The bundle is written incrementally into a `Staging` directory and only +/// sealed at the end of a run, so `xcresulttool` cannot open one belonging to a +/// run that was stopped at its first failure — which is every run outside CI. +/// The staged runner output is plain text and is there either way, and it holds +/// what swift-testing printed: the failed expression, and the comment the +/// author wrote under it. +/// +/// Nothing in the test target has to cooperate. +/// A `#expect`, a `#require`, an `Issue.record` and an `XCTest` assertion all +/// arrive the same way, which is the property a convention nobody has to +/// remember gives you. +pub(super) fn collect_staged_issues(root: &Utf8Path) -> Option { + let mut issues = Vec::new(); + + for file in staged_runner_output(&root.join(RESULT_BUNDLE)) { + let Ok(text) = fs::read_to_string(&file) else { + continue; + }; + + issues.extend(issue_lines(&text)); + } + + if issues.is_empty() { + return None; + } + + Some(format!( + "\n\nWhat the tests reported:\n\n```\n{}\n```\n", + issues.join("\n") + )) +} + +/// The failed expectations in one runner log, each with the comment under it. +/// +/// A failure is one line naming where it happened and what did not hold, then +/// any number of lines carrying the author's comment. +/// The comment lines are recognized by what they are not: the runner prefixes +/// its own activity with a timestamp, and marks suites and tests with a glyph. +fn issue_lines(text: &str) -> Vec { + let mut found: Vec = Vec::new(); + let mut in_issue = false; + + for line in text.lines() { + let trimmed = line.trim(); + + if trimmed.contains(ISSUE_MARKER) { + found.push(strip(trimmed)); + in_issue = true; + continue; + } + + if !in_issue { + continue; + } + + // The runner's own activity resumes, so the comment has ended. + if trimmed.is_empty() || trimmed.starts_with("t =") { + in_issue = false; + continue; + } + + let comment = strip(trimmed); + // Source lines the runner echoes back are already in the message above. + if comment.starts_with("//") { + continue; + } + + found.push(format!(" {comment}")); + } + + found +} + +/// Every staged runner log inside a result bundle. +/// +/// The path holds two UUIDs the run picks, so the tree is walked rather than +/// spelled out. +fn staged_runner_output(bundle: &Utf8Path) -> Vec { + let mut found = Vec::new(); + let mut stack = vec![bundle.join("Staging")]; + + while let Some(directory) = stack.pop() { + let Ok(entries) = fs::read_dir(&directory) else { + continue; + }; + + for entry in entries.flatten() { + let Ok(path) = Utf8PathBuf::from_path_buf(entry.path()) else { + continue; + }; + + if path.is_dir() { + stack.push(path); + } else if path.file_name() == Some(RUNNER_OUTPUT) { + found.push(path); + } + } + } + + found +} + +/// The result bundle `xcodebuild` wrote, as `xcresulttool` reports it. +/// +/// Apple's own reader for Apple's own format, so a failing `#expect`, a +/// `#require`, an `Issue.record` and an `XCTest` assertion all arrive the same +/// way, with the comment the author wrote. +/// Nothing in the test target has to know about this, which is the point: a +/// helper the suite must remember to call is a helper a future test will +/// forget. +/// +/// Fetched once and read twice, for what failed and for what ran. +/// +/// Returns `None` when there is no readable bundle. +/// A run stopped at its first failure is killed rather than asked to stop, so +/// the bundle it was part-way through writing may not be finished — which is +/// why the caller keeps a fallback rather than relying on this alone. +pub(super) fn bundle_document(ctx: &Context, runner: &R) -> Option { + let bundle = ctx.root.join(RESULT_BUNDLE); + if !bundle.exists() { + return None; + } + + let output = runner + .run( + "xcrun", + &[ + "xcresulttool", + "get", + "test-results", + "tests", + "--path", + bundle.as_str(), + "--compact", + ], + &ctx.root, + ) + .ok()?; + + serde_json::from_str(&output.stdout).ok() +} + +/// Every test the bundle records as having run, named the way a caller names +/// one. +/// +/// A `Test Case` node's `nodeIdentifier` is its suite path and function — +/// `UISuite/ConversationListTests/clickSelects()` — which is the selector this +/// tool takes, minus the bundle prefix it adds itself. +/// So the two compare directly. +/// +/// Empty when the document says nothing this recognizes, which the caller must +/// read as "cannot tell" rather than "nothing ran": `nodeIdentifier` is +/// optional in the schema, and treating its absence as a test that did not run +/// would fail a passing suite. +pub(super) fn executed_tests(document: &Value) -> Vec { + let mut found = Vec::new(); + walk_executed(document, &mut found); + found.sort(); + found.dedup(); + found +} + +fn walk_executed(value: &Value, found: &mut Vec) { + if let Value::Array(items) = value { + for item in items { + walk_executed(item, found); + } + return; + } + + let Some(object) = value.as_object() else { + return; + }; + + if object.get("nodeType").and_then(Value::as_str) == Some("Test Case") + && let Some(id) = object.get("nodeIdentifier").and_then(Value::as_str) + { + found.push(id.to_owned()); + } + + for child in object.values() { + walk_executed(child, found); + } +} + +/// The failures the bundle records, if any. +pub(super) fn bundle_issues(document: &Value) -> Option { + let mut failures = Vec::new(); + walk_failures(document, &mut Vec::new(), &mut failures); + + if failures.is_empty() { + return None; + } + + Some(format!( + "\n\nWhat the tests reported, from {RESULT_BUNDLE}:\n\n```\n{}\n```\n", + failures.join("\n") + )) +} + +/// Collect every failure message in the tests tree, under the test that holds +/// it. +/// +/// The JSON is walked for the shapes it is known to use rather than +/// deserialized into the document's schema. +/// `xcresulttool` versions its output and has changed it between Xcode +/// releases; a walk that finds nothing degrades to a less helpful message, +/// where a failed parse would replace the failure being reported with a +/// complaint about reading it. +fn walk_failures(value: &Value, path: &mut Vec, found: &mut Vec) { + if let Value::Array(items) = value { + for item in items { + walk_failures(item, path, found); + } + return; + } + + let Some(object) = value.as_object() else { + return; + }; + + let kind = object.get("nodeType").and_then(Value::as_str).unwrap_or(""); + let name = object.get("name").and_then(Value::as_str).unwrap_or(""); + + // A failure is a node whose type says so, and whose name is the message. + if kind.contains("Failure") && !name.is_empty() { + let where_ = path.last().map_or("", String::as_str); + found.push(format!("{where_}: {name}")); + } + + // Test cases name themselves, so the innermost one seen above a failure is + // the test that recorded it. + let named = kind == "Test Case" && !name.is_empty(); + if named { + path.push(name.to_owned()); + } + + for child in object.values() { + walk_failures(child, path, found); + } + + if named { + path.pop(); + } +} + +/// The file the UI tests write the process ids of the apps they launched to. +const APP_PIDS: &str = "app.pids"; + +/// Close the apps a stopped run left running. +/// +/// Stopping a run kills `xcodebuild`, and that does not reach the app under +/// test: `testmanagerd` launched it, so it survives and sits on the screen +/// until somebody quits it. +/// +/// By process id, written by each app itself, and never by name or bundle +/// identifier — the developer's own copy of JP has both of those, and closing +/// their window because a test failed would be a poor trade for a tidy screen. +/// A `TERM` rather than a kill, so the app puts itself away as it would on +/// quit. +pub(super) fn close_leftover_apps() { + let Some(source) = screenshot_source() else { + return; + }; + + let Ok(pids) = fs::read_to_string(source.join(APP_PIDS)) else { + return; + }; + + for pid in pids.lines().map(str::trim).filter(|pid| !pid.is_empty()) { + // Most of these are already gone: a test that finished terminated its + // own app. Signalling a process that is not there fails, which is the + // answer wanted anyway. + let _signalled = std::process::Command::new("kill") + .args(["-TERM", pid]) + .status(); + } + + let _removed = fs::remove_file(source.join(APP_PIDS)); +} + +/// What the UI tests recorded about their own failures. +/// +/// Returns the empty string when they recorded nothing. +/// +/// This exists because `xcodebuild` prints the header of a swift-testing issue +/// and drops the message under it, so a failing run arrives as a column of +/// identical `Issue recorded` lines. +/// The tests write the messages themselves, and this is where they are read +/// back. +pub(super) fn collect_failures() -> String { + let Some(source) = screenshot_source() else { + return "\n\nThe tests' own failure messages could not be looked for: HOME is unset, so \ + the runner's container has no derivable path.\n" + .to_owned(); + }; + + let path = source.join(FAILURE_LOG); + let log = match fs::read_to_string(&path) { + Ok(log) if !log.trim().is_empty() => log, + + // Read and empty, or not there at all. Both mean the same thing to a + // reader and neither may be reported as silence: a run that failed while + // writing nothing here looks, without this, exactly like a run that + // failed for no stated reason. + _ => { + return format!( + "\n\nThe tests recorded no failure messages ({path} is empty or absent). A \ + failing `#expect` or `#require` writes nothing here on its own \u{2014} only the \ + helpers that call `AppUnderTest.record` do. Read the assertion at the reported \ + line, or route it through `expectAppears`, `expectTranscript` or `record` so the \ + next run says what it saw.\n" + ); + } + }; + + format!( + "\n\nWhat the tests recorded:\n\n```\n{}\n```\n", + log.trim_end() + ) +} + +/// Copy this run's screenshots into the checkout and name them. +/// +/// Returns the empty string when there are none, so a failure with nothing to +/// show reads exactly as it did before. +/// +/// Copied rather than linked to: the container directory is emptied at the +/// start of every run, and a path that stops resolving the moment the next run +/// starts is worse than no path. +pub(super) fn collect_screenshots(root: &Utf8Path) -> String { + let Some(source) = screenshot_source() else { + return String::new(); + }; + + let target = root.join(SCREENSHOT_DIR); + let mut collected = Vec::new(); + + let Ok(entries) = fs::read_dir(&source) else { + return String::new(); + }; + + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name) = name.to_str() else { continue }; + if !Utf8Path::new(name) + .extension() + .is_some_and(|ext| ext.eq_ignore_ascii_case("png")) + { + continue; + } + + if fs::create_dir_all(&target).is_err() { + return String::new(); + } + + if fs::copy(entry.path(), target.join(name)).is_ok() { + collected.push(format!("{SCREENSHOT_DIR}/{name}")); + } + } + + if collected.is_empty() { + return String::new(); + } + + collected.sort(); + format!( + "\n\nWhat was on screen when the assertions failed. A tool result is text, so attach one \ + to see it:\n\n{}\n", + collected + .iter() + .map(|path| format!("- `{path}`")) + .collect::>() + .join("\n") + ) +} + +/// Markers of a run that died rather than one that failed. +/// +/// A process that crashes reports no failing test and no summary, so without +/// these the only honest thing left to say is "something went wrong, here is +/// the log". +/// `xcodebuild` announces a dead test runner; the rest are what the runtime +/// prints on its way out. +const CRASH_MARKERS: &[&str] = &[ + "Restarting after unexpected exit", + "Fatal error", + "Crashed:", + "EXC_BAD_ACCESS", + "Test runner exited", +]; + +/// A test run's output, ready to be searched. +/// +/// Both runners split their output across both streams, so both are kept. +struct Log { + stdout: String, + stderr: String, +} + +impl Log { + fn from(output: &ProcessOutput) -> Self { + Self { + stdout: strip_ansi_escapes::strip_str(&output.stdout), + stderr: strip_ansi_escapes::strip_str(&output.stderr), + } + } + + /// Both streams, one line at a time, exactly as written. + fn raw_lines(&self) -> impl Iterator { + self.stdout.lines().chain(self.stderr.lines()) + } + + fn lines(&self) -> impl Iterator { + self.raw_lines().map(clean) + } + + /// The run summary, naming how many tests ran. + /// + /// Empty when nothing ran, which is a distinct outcome from a passing run. + fn summary(&self) -> String { + strip( + &self + .lines() + .filter(|line| is_run_summary(line)) + .collect::>() + .join("\n"), + ) + } + + /// The lines saying the run died rather than reported a failure. + fn crashes(&self) -> String { + strip( + &self + .lines() + .filter(|line| CRASH_MARKERS.iter().any(|marker| line.contains(marker))) + .collect::>() + .join("\n"), + ) + } + + /// The last `count` non-empty lines of both streams. + /// + /// The end rather than the beginning: a run with no diagnostic to show got + /// as far as it got, and the head of the log is the build starting up. + fn tail(&self, count: usize) -> String { + let lines: Vec<&str> = self.lines().filter(|line| !line.is_empty()).collect(); + let start = lines.len().saturating_sub(count); + + strip(&lines[start..].join("\n")) + } + + /// The lines naming what went wrong. + /// + /// Two spellings of a compiler error, because the two runners differ: + /// `xcodebuild` prefixes the file and line, and `swift test` reports a + /// driver-level failure such as a missing module with nothing in front of + /// it. + /// + /// Continuation lines come along with the failure they belong to. + /// swift-testing puts an issue's own message on a line of its own, under a + /// header that names only the *kind* of issue, so a filter that kept the + /// header alone would report every recorded issue as "Issue recorded" and + /// throw away the sentence explaining it. + fn failures(&self) -> String { + strip( + &self + .raw_lines() + .filter(|line| { + let cleaned = clean(line); + line.contains(ISSUE_DETAIL_MARKER) + || cleaned.contains("recorded an issue") + || cleaned.contains(": error:") + || cleaned.starts_with("error:") + }) + .map(clean) + .collect::>() + .join("\n"), + ) + } +} + +/// The character swift-testing indents an issue's message with. +const ISSUE_DETAIL_MARKER: char = '\u{21b3}'; + +/// A log line with its decoration removed. +/// +/// swift-testing prefixes each line with an SF Symbol from a private use area, +/// which is a glyph in Xcode and mojibake anywhere else. +fn clean(line: &str) -> &str { + line.trim_start_matches(|c: char| !c.is_ascii()).trim() +} + +/// Whether a line summarizes a run that executed at least one test. +/// +/// A zero count is not a summary but the absence of one: a bundle whose tests +/// are all swift-testing always draws an `Executed 0 tests` line from the +/// `XCTest` runner, and reporting that as a pass would hide every mistyped +/// filter. +fn is_run_summary(line: &str) -> bool { + // swift-testing: "Test run with 30 tests in 6 suites passed after 0.028s." + if line.contains("Test run with") { + return !line.contains("with 0 tests"); + } + + // XCTest: "Executed 20 tests, with 0 failures (0 unexpected) in 0.1 seconds" + line.contains("Executed ") && !line.contains("Executed 0 tests") +} + +#[cfg(test)] +#[path = "report_tests.rs"] +mod tests; diff --git a/.config/jp/tools/src/swift/report_tests.rs b/.config/jp/tools/src/swift/report_tests.rs new file mode 100644 index 000000000..c33e2419b --- /dev/null +++ b/.config/jp/tools/src/swift/report_tests.rs @@ -0,0 +1,103 @@ +use pretty_assertions::assert_eq; +use serde_json::json; + +use super::*; + +/// A result bundle's test tree, in the shape `xcresulttool get test-results +/// tests` documents. +/// +/// The schema is published by `xcresulttool get test-results tests --help`: +/// `testNodes` holds a `TestNode` tree, `nodeType` is a closed enum including +/// `Test Plan`, `UI test bundle`, `Test Suite` and `Test Case`, and a test +/// case's `nodeIdentifier` is its suite path and function without the bundle. +/// +/// Nested suites appear as nested `Test Suite` nodes, which is how a +/// swift-testing suite inside another one arrives. +fn document() -> Value { + json!({ + "devices": [{ "deviceName": "My Mac" }], + "testPlanConfigurations": [{ "configurationId": "1" }], + "testNodes": [{ + "name": "JP", + "nodeType": "Test Plan", + "children": [{ + "name": "JPUITests", + "nodeType": "UI test bundle", + "children": [{ + "name": "UISuite", + "nodeType": "Test Suite", + "children": [{ + "name": "ConversationListTests", + "nodeType": "Test Suite", + "children": [ + { + "name": "clickSelects()", + "nodeIdentifier": "UISuite/ConversationListTests/clickSelects()", + "nodeType": "Test Case", + "result": "Passed" + }, + { + "name": "labelsRows()", + "nodeIdentifier": "UISuite/ConversationListTests/labelsRows()", + "nodeType": "Test Case", + "result": "Passed" + } + ] + }] + }] + }] + }] + }) +} + +/// The identifiers are what the caller names a test with, so they can be +/// compared against the requested selectors without reshaping either side. +#[test] +fn reads_the_tests_that_ran() { + assert_eq!(executed_tests(&document()), vec![ + "UISuite/ConversationListTests/clickSelects()".to_owned(), + "UISuite/ConversationListTests/labelsRows()".to_owned(), + ]); +} + +/// Only `Test Case` nodes name a test that ran. +/// A suite carries a name too, and counting it would let a suite that matched +/// nothing look as though it had. +#[test] +fn ignores_every_node_that_is_not_a_test_case() { + let document = json!({ + "testNodes": [{ + "name": "UISuite", + "nodeIdentifier": "UISuite", + "nodeType": "Test Suite", + "children": [{ + "name": "Failure Message", + "nodeIdentifier": "not-a-test", + "nodeType": "Failure Message" + }] + }] + }); + + assert!(executed_tests(&document).is_empty()); +} + +/// `nodeIdentifier` is optional in the schema. +/// A test case without one cannot be matched against a request, and the caller +/// reads an empty list as "cannot tell" rather than "nothing ran". +#[test] +fn skips_a_test_case_with_no_identifier() { + let document = json!({ + "testNodes": [{ + "name": "clickSelects()", + "nodeType": "Test Case", + "result": "Passed" + }] + }); + + assert!(executed_tests(&document).is_empty()); +} + +#[test] +fn reads_nothing_from_a_document_with_no_tests() { + assert!(executed_tests(&json!({ "testNodes": [] })).is_empty()); +} diff --git a/.config/jp/tools/src/swift/test.rs b/.config/jp/tools/src/swift/test.rs new file mode 100644 index 000000000..8012cc9b2 --- /dev/null +++ b/.config/jp/tools/src/swift/test.rs @@ -0,0 +1,162 @@ +use jp_tool::Context; + +use super::{ + PROJECT_PATH, SCHEME, prepare, + report::{outcome, unit_bundle_filter}, +}; +use crate::util::{ + ToolResult, error, + runner::{DuctProcessRunner, ProcessOutput, ProcessRunner}, +}; + +/// The `SwiftPM` package holding the accessibility driver. +const DRIVE_PACKAGE: &str = "apps/macos/Tools/jpdrive"; + +/// Which test suites to run. +/// +/// The two are built and filtered by different tools, so a run has to name +/// which it means rather than passing one filter to both: a suite name that +/// addresses the app matches nothing in the package, and a run that matched +/// nothing is reported as a failure. +/// +/// The UI tests are not reachable from here at all. +/// They launch the app and take the screen for as long as they run, so they +/// belong to `swift_test_ui`, which has to be asked for them by name. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Target { + /// The app's unit-test bundle, through `xcodebuild`. + App, + /// The driver package's tests, through `swift test`. + Drive, + /// Both. + All, +} + +impl Target { + fn parse(value: Option<&str>) -> Result { + match value { + None | Some("all") => Ok(Self::All), + Some("app") => Ok(Self::App), + Some("drive") => Ok(Self::Drive), + Some("ui") => Err( + "the UI tests run through `swift_test_ui`, which takes the tests to run by name. \ + They drive the app through the screen, so a run costs about six seconds per test \ + and takes over the display." + .to_owned(), + ), + Some(other) => Err(format!( + "unknown target '{other}', expected one of: app, drive, all" + )), + } + } + + fn includes_app(self) -> bool { + self != Self::Drive + } + + fn includes_drive(self) -> bool { + self != Self::App + } +} + +pub(crate) async fn swift_test( + ctx: &Context, + testname: Option, + target: Option, +) -> ToolResult { + swift_test_impl( + ctx, + testname.as_deref(), + target.as_deref(), + &DuctProcessRunner, + ) +} + +fn swift_test_impl( + ctx: &Context, + testname: Option<&str>, + target: Option<&str>, + runner: &R, +) -> ToolResult { + let target = match Target::parse(target) { + Ok(target) => target, + Err(message) => return error(message), + }; + + let mut summaries = Vec::new(); + + // The driver package needs no Xcode project and runs in milliseconds, so it + // goes first: a failure there is reported before a minute of `xcodebuild`. + if target.includes_drive() { + let output = run_drive(ctx, testname, runner)?; + match outcome(&output, "DriveKit") { + Ok(summary) => summaries.push(summary), + Err(message) => return error(message), + } + } + + if target.includes_app() { + // Tests run against the Debug configuration, so the library they link is + // the one in cargo's `debug` directory. + if let Some(failure) = prepare(ctx, "debug", runner)? { + return error(failure); + } + + let output = run_app(ctx, testname, runner)?; + match outcome(&output, "JP") { + Ok(summary) => summaries.push(summary), + Err(message) => return error(message), + } + } + + Ok(format!("```\n{}\n```", summaries.join("\n")).into()) +} + +/// Run the driver package's tests. +fn run_drive( + ctx: &Context, + testname: Option<&str>, + runner: &R, +) -> Result { + let mut args = vec!["test", "--package-path", DRIVE_PACKAGE]; + if let Some(testname) = testname { + // `swift test` takes a regex over `Suite/test`, which is the same shape + // callers already write for the app bundle. + args.extend(["--filter", testname]); + } + + runner.run("swift", &args, &ctx.root) +} + +/// Run the app's unit-test bundle. +/// +/// The filter always names the bundle, even with no test to narrow to: the UI +/// bundle is in the same scheme, and a run that named no bundle would launch +/// the app once per UI test. +fn run_app( + ctx: &Context, + testname: Option<&str>, + runner: &R, +) -> Result { + let filter = unit_bundle_filter(testname); + + // `-quiet` is deliberately absent. It suppresses the test summary along with + // everything else, which makes a filter that matched nothing look exactly + // like a passing run. + let args = vec![ + "test", + "-project", + PROJECT_PATH, + "-scheme", + SCHEME, + "-destination", + "platform=macOS", + &filter, + ]; + + runner.run("xcodebuild", &args, &ctx.root) +} + +#[cfg(test)] +#[path = "test_tests.rs"] +mod tests; diff --git a/.config/jp/tools/src/swift/test_tests.rs b/.config/jp/tools/src/swift/test_tests.rs new file mode 100644 index 000000000..6a058fad4 --- /dev/null +++ b/.config/jp/tools/src/swift/test_tests.rs @@ -0,0 +1,480 @@ +use jp_tool::{Action, Context}; +use pretty_assertions::assert_eq; + +use super::{super::error_message, *}; +use crate::util::runner::{ExitCode, MockProcessRunner, ProcessOutput}; + +fn ctx() -> Context { + Context { + root: "/repo".into(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + } +} + +/// A swift-testing summary line, verbatim from a real run. +/// +/// The leading glyph is the SF Symbol swift-testing prefixes its output with. +const PASSING_SUMMARY: &str = + "\u{1005db} Test run with 30 tests in 6 suites passed after 0.028 seconds."; + +/// What the summary looks like once the tool has cleaned it up. +const CLEANED_SUMMARY: &str = "Test run with 30 tests in 6 suites passed after 0.028 seconds."; + +/// The driver package's own summary, so a test can tell the two runs apart. +const DRIVE_SUMMARY: &str = + "\u{1005db} Test run with 33 tests in 4 suites passed after 0.012 seconds."; + +const CLEANED_DRIVE: &str = "Test run with 33 tests in 4 suites passed after 0.012 seconds."; + +/// A bundle whose tests are all swift-testing always draws this from the +/// `XCTest` runner, whether or not anything ran. +const XCTEST_ZERO: &str = + "Executed 0 tests, with 0 failures (0 unexpected) in 0.000 (0.000) seconds"; + +/// Expect the driver package run and the two app preparation steps, in the +/// order a full run performs them. +/// +/// The package goes first because it is the cheap one, so a failure there is +/// reported before a minute of `xcodebuild`. +fn prepared() -> MockProcessRunner { + MockProcessRunner::builder() + .expect("swift") + .args(&["test", "--package-path", "apps/macos/Tools/jpdrive"]) + .returns_success(DRIVE_SUMMARY) + .expect("just") + .args(&["build-ffi", "debug"]) + .returns_success("") + .expect("xcodegen") + .returns_success("") +} + +/// Expect only the app preparation steps, for a run targeting the app alone. +fn prepared_app_only() -> MockProcessRunner { + MockProcessRunner::builder() + .expect("just") + .args(&["build-ffi", "debug"]) + .returns_success("") + .expect("xcodegen") + .returns_success("") +} + +/// The default run covers the driver package and the app's unit tests, and +/// never the UI bundle: the filter names `JPTests` even with nothing to narrow +/// to, because both bundles are in the one scheme. +#[test] +fn runs_both_targets_by_default() { + let runner = prepared() + .expect("xcodebuild") + .args(&[ + "test", + "-project", + "apps/macos/JP.xcodeproj", + "-scheme", + "JP", + "-destination", + "platform=macOS", + "-only-testing:JPTests", + ]) + .returns_success(PASSING_SUMMARY); + + let result = swift_test_impl(&ctx(), None, None, &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + format!("```\nDriveKit: {CLEANED_DRIVE}\nJP: {CLEANED_SUMMARY}\n```") + ); +} + +/// The driver package builds and runs without an Xcode project, so targeting it +/// must not drag the app's preparation steps along. +#[test] +fn the_drive_target_skips_the_app_entirely() { + let runner = MockProcessRunner::builder() + .expect("swift") + .args(&["test", "--package-path", "apps/macos/Tools/jpdrive"]) + .returns_success(DRIVE_SUMMARY); + + let result = swift_test_impl(&ctx(), None, Some("drive"), &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + format!("```\nDriveKit: {CLEANED_DRIVE}\n```") + ); +} + +#[test] +fn the_app_target_runs_only_the_unit_bundle() { + let runner = prepared_app_only() + .expect("xcodebuild") + .args(&[ + "test", + "-project", + "apps/macos/JP.xcodeproj", + "-scheme", + "JP", + "-destination", + "platform=macOS", + "-only-testing:JPTests", + ]) + .returns_success(PASSING_SUMMARY); + + let result = swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + format!("```\nJP: {CLEANED_SUMMARY}\n```") + ); +} + +/// The UI tests are not reachable from this tool at all, and asking for them +/// says where they went rather than reporting an unknown target. +#[test] +fn the_ui_target_points_at_the_other_tool() { + // Nothing may be spawned: a run that started building before refusing would + // cost a minute to say no. + let runner = MockProcessRunner::never_called(); + + let message = error_message(swift_test_impl(&ctx(), None, Some("ui"), &runner).unwrap()); + + assert!(message.contains("swift_test_ui"), "got: {message}"); +} + +/// swift-testing writes an issue's message on its own line, under a header +/// naming only the kind of issue. +/// Reporting the header alone turns every recorded issue into "Issue recorded" +/// and drops the sentence saying what went wrong. +#[test] +fn keeps_the_message_under_a_recorded_issue() { + let runner = prepared_app_only() + .expect("xcodebuild") + .returns(ProcessOutput { + stdout: "\u{1005db} Test \"selects a row\" recorded an issue at Foo.swift:12:5: Issue \ + recorded\n\u{21b3} the transcript never appeared. On screen instead: \ + /tmp/uitests/a.png\n" + .to_owned(), + stderr: String::new(), + status: ExitCode::from_code(1), + }); + + let message = error_message(swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap()); + + assert!(message.contains("recorded an issue"), "got: {message}"); + assert!( + message.contains("On screen instead: /tmp/uitests/a.png"), + "got: {message}" + ); +} + +#[test] +fn rejects_an_unknown_target() { + let message = Target::parse(Some("both")).unwrap_err(); + + assert_eq!( + message, + "unknown target 'both', expected one of: app, drive, all" + ); +} + +/// An absent target runs everything, which is what makes the tool useful +/// without the caller knowing the project has two test suites in the first +/// place. +#[test] +fn an_absent_target_means_all() { + assert_eq!(Target::parse(None).unwrap(), Target::All); + assert_eq!(Target::parse(Some("all")).unwrap(), Target::All); + assert_eq!(Target::parse(Some("app")).unwrap(), Target::App); + assert_eq!(Target::parse(Some("drive")).unwrap(), Target::Drive); +} + +/// Each runner takes its own filter syntax, so a name has to be translated for +/// whichever target it is addressed to. +#[test] +fn passes_a_filter_to_each_runner_in_its_own_form() { + let drive = MockProcessRunner::builder() + .expect("swift") + .args(&[ + "test", + "--package-path", + "apps/macos/Tools/jpdrive", + "--filter", + "Tree", + ]) + .returns_success(DRIVE_SUMMARY); + + swift_test_impl(&ctx(), Some("Tree"), Some("drive"), &drive).unwrap(); + + let app = prepared_app_only() + .expect("xcodebuild") + .args(&[ + "test", + "-project", + "apps/macos/JP.xcodeproj", + "-scheme", + "JP", + "-destination", + "platform=macOS", + "-only-testing:JPTests/WorkspaceReaderTests", + ]) + .returns_success(PASSING_SUMMARY); + + swift_test_impl(&ctx(), Some("WorkspaceReaderTests"), Some("app"), &app).unwrap(); +} + +/// A filter naming a suite in the other target matches nothing there, which is +/// the mistake the error has to name rather than passing over. +#[test] +fn a_filter_matching_nothing_in_one_target_fails_the_run() { + let runner = MockProcessRunner::builder() + .expect("swift") + .returns_success("Test run with 0 tests passed after 0.001 seconds."); + + let message = error_message( + swift_test_impl(&ctx(), Some("WorkspaceReaderTests"), Some("drive"), &runner).unwrap(), + ); + + assert!(message.contains("no test ran"), "got: {message}"); + assert!( + message.contains("matches nothing in the bundle"), + "got: {message}" + ); +} + +/// The `XCTest` runner reports zero tests for a bundle that has none of its +/// own, which says nothing about whether the swift-testing tests ran. +/// Treating it as a result would make a mistyped filter look like a pass. +#[test] +fn an_xctest_zero_count_alone_is_not_a_pass() { + let runner = prepared_app_only() + .expect("xcodebuild") + .returns_success(format!("{XCTEST_ZERO}\n** TEST SUCCEEDED **")); + + let message = + error_message(swift_test_impl(&ctx(), Some("NoSuchSuite"), Some("app"), &runner).unwrap()); + + assert!(message.contains("no test ran"), "got: {message}"); +} + +/// A real run prints both, and only the swift-testing line carries a count +/// worth reading. +#[test] +fn drops_the_xctest_zero_count_beside_a_real_summary() { + let runner = prepared_app_only() + .expect("xcodebuild") + .returns_success(format!("{XCTEST_ZERO}\n{PASSING_SUMMARY}")); + + let result = swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + format!("```\nJP: {CLEANED_SUMMARY}\n```") + ); +} + +/// A run that reported no summary is a run that did nothing. +/// Reporting it as a pass would make every filtered run worthless. +#[test] +fn a_run_with_no_summary_is_not_a_pass() { + let runner = prepared_app_only() + .expect("xcodebuild") + .returns_success("Testing started\nnote: something unrelated"); + + let message = + error_message(swift_test_impl(&ctx(), Some("NoSuchSuite"), Some("app"), &runner).unwrap()); + + assert!(message.contains("no test ran"), "got: {message}"); +} + +/// `XCTest` words its summary differently from swift-testing, and a bundle can +/// hold both. +#[test] +fn recognizes_an_xctest_summary() { + let runner = prepared_app_only().expect("xcodebuild").returns_success( + "Test Suite 'All tests' passed\n\t Executed 20 tests, with 0 failures in 1.234 seconds", + ); + + let result = swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap(); + + assert!( + result.unwrap_content().contains("Executed 20 tests"), + "expected the XCTest summary to be reported" + ); +} + +/// The raw log is thousands of lines of compiler invocations; only the lines +/// that say what ran are worth relaying. +#[test] +fn drops_build_noise_from_the_summary() { + let runner = prepared_app_only() + .expect("xcodebuild") + .returns_success(format!( + "CompileSwift normal arm64 WorkspaceReader.swift\nLd \ + JP.app/Contents/MacOS/JP\n{PASSING_SUMMARY}" + )); + + let result = swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + format!("```\nJP: {CLEANED_SUMMARY}\n```") + ); +} + +#[test] +fn reports_failing_tests() { + let runner = prepared_app_only() + .expect("xcodebuild") + .returns(ProcessOutput { + stdout: "WorkspaceReaderTests.swift:61: error: Expectation failed".to_owned(), + stderr: String::new(), + status: ExitCode::from_code(65), + }); + + let message = error_message(swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap()); + + assert_eq!( + message, + "JP tests failed:\n\n```\nWorkspaceReaderTests.swift:61: error: Expectation failed\n```" + ); +} + +/// A failing package run is labelled as such, so it is clear which suite broke +/// without reading the diagnostics. +#[test] +fn names_the_target_that_failed() { + let runner = MockProcessRunner::builder() + .expect("swift") + .returns(ProcessOutput { + stdout: "ActTests.swift:32: error: Expectation failed".to_owned(), + stderr: String::new(), + status: ExitCode::from_code(1), + }); + + let message = error_message(swift_test_impl(&ctx(), None, Some("drive"), &runner).unwrap()); + + assert!( + message.starts_with("DriveKit tests failed:"), + "got: {message}" + ); +} + +/// A crashed run reports no failing test and no summary. +/// Naming the crash beats the alternative, which was dumping the whole build +/// log and leaving the reader to find the end of it. +#[test] +fn names_a_crash_rather_than_dumping_the_log() { + let runner = MockProcessRunner::builder() + .expect("swift") + .returns(ProcessOutput { + stdout: "Building for debugging...\nRestarting after unexpected exit, crash, or test \ + timeout in ScrollMemoryTests.roundTrips()" + .to_owned(), + stderr: String::new(), + status: ExitCode::from_code(1), + }); + + let message = error_message(swift_test_impl(&ctx(), None, Some("drive"), &runner).unwrap()); + + assert_eq!( + message, + "DriveKit tests failed:\n\n```\nRestarting after unexpected exit, crash, or test timeout \ + in ScrollMemoryTests.roundTrips()\n```" + ); +} + +/// A run that died with nothing recognizable in it still has to say something, +/// and the useful end of a build log is the last of it, not the first. +#[test] +fn shows_the_end_of_an_unrecognizable_failure() { + let noise = (0..100) + .map(|index| format!("CompileSwift normal arm64 File{index}.swift")) + .collect::>() + .join("\n"); + + let runner = MockProcessRunner::builder() + .expect("swift") + .returns(ProcessOutput { + stdout: format!("{noise}\nthe last thing it printed"), + stderr: String::new(), + status: ExitCode::from_code(1), + }); + + let message = error_message(swift_test_impl(&ctx(), None, Some("drive"), &runner).unwrap()); + + assert!( + message.contains("the run died rather than failing"), + "got: {message}" + ); + assert!( + message.contains("the last thing it printed"), + "got: {message}" + ); + assert!( + !message.contains("File0.swift"), + "expected the end of the log, not the start of it" + ); +} + +/// A failing run names the test that failed and then how the run ended, and +/// both are wanted: the first says what to fix, the second says how much broke. +#[test] +fn reports_the_failing_test_and_the_summary() { + let runner = prepared_app_only() + .expect("xcodebuild") + .returns(ProcessOutput { + stdout: "CompileSwift normal arm64\n\u{1005df} Test \"orders an empty list\" recorded \ + an issue at TimestampTests.swift:88\n\u{1005db} Test run with 30 tests in 6 \ + suites failed after 0.03 seconds." + .to_owned(), + stderr: String::new(), + status: ExitCode::from_code(65), + }); + + let message = error_message(swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap()); + + assert_eq!( + message, + "JP tests failed:\n\n```\nTest \"orders an empty list\" recorded an issue at \ + TimestampTests.swift:88\nTest run with 30 tests in 6 suites failed after 0.03 \ + seconds.\n```" + ); +} + +/// A failure in the cheap suite stops the run before the expensive one, so the +/// diagnostics arrive in seconds rather than after a full app build. +#[test] +fn a_package_failure_stops_before_the_app_build() { + let runner = MockProcessRunner::builder() + .expect("swift") + .returns(ProcessOutput { + stdout: String::new(), + stderr: "error: no such module 'Testing'".to_owned(), + status: ExitCode::from_code(1), + }); + + let message = error_message(swift_test_impl(&ctx(), None, None, &runner).unwrap()); + + assert!( + message.starts_with("DriveKit tests failed:"), + "got: {message}" + ); +} + +/// The bridging header has to exist before `xcodebuild` plans the build, so a +/// failed library build stops the run rather than producing a confusing "file +/// not found" from the header scan. +#[test] +fn stops_when_the_library_fails_to_build() { + let runner = MockProcessRunner::builder() + .expect("just") + .returns_error("error: could not compile `jp_ffi`"); + + let message = error_message(swift_test_impl(&ctx(), None, Some("app"), &runner).unwrap()); + + assert!( + message.starts_with("Building `jp_ffi` failed:"), + "got: {message}" + ); +} diff --git a/.config/jp/tools/src/swift/test_ui.rs b/.config/jp/tools/src/swift/test_ui.rs new file mode 100644 index 000000000..65ee3c9d7 --- /dev/null +++ b/.config/jp/tools/src/swift/test_ui.rs @@ -0,0 +1,315 @@ +//! `swift_test_ui` — run named UI tests against the macOS app. +//! +//! Separate from [`swift_test`] because a UI test is a different kind of thing +//! to run. +//! It launches the app, takes the screen for as long as it drives it, and costs +//! seconds rather than milliseconds. +//! Running the suite is a job for CI; running two tests you just wrote is a job +//! for this. +//! +//! Which is why the tests to run are required rather than optional. +//! There is no spelling of this tool that means "run all of them" — reaching +//! for that is how a red-green loop turns into a minute per iteration. +//! +//! [`swift_test`]: super::test + +use jp_tool::Context; + +use super::{ + PROJECT_PATH, SCHEME, prepare, + report::{ + RESULT_BUNDLE, UI_BUNDLE, bundle_document, bundle_issues, clear_result_bundle, + clear_screenshots, close_leftover_apps, collect_failures, collect_screenshots, + collect_staged_issues, executed_tests, outcome, ui_bundle_filter, + }, +}; +use crate::util::{ + ToolResult, error, + runner::{DuctProcessRunner, ProcessOutput, ProcessRunner, Stopped}, +}; + +/// The line `xcodebuild` prints when a swift-testing expectation fails. +/// +/// Watched for rather than waited on: the run is stopped the moment one +/// appears, so a broken app costs one test's worth of time instead of the whole +/// suite's. +const FAILURE_MARKER: &str = "recorded an issue"; + +/// Whether this process is running under continuous integration. +/// +/// CI wants every result from one run, because nobody is sitting there to run +/// it again; a person at a keyboard wants the first failure as fast as +/// possible. +/// Same suite, opposite priorities, so the environment decides. +fn under_ci() -> bool { + is_ci(std::env::var("CI").ok().as_deref()) +} + +/// Whether `value` — the `CI` variable, or `None` when it is unset — means +/// CI. +/// +/// Split out so the rule can be checked without writing to the process +/// environment. +/// `set_var` is unsafe because another thread may be reading, and `under_ci` is +/// reached by every test that runs the tool, so a test that set `CI` would be +/// racing them. +fn is_ci(value: Option<&str>) -> bool { + value.is_some_and(|value| !value.is_empty()) +} + +pub(crate) async fn swift_test_ui(ctx: &Context, tests: Option>) -> ToolResult { + swift_test_ui_impl(ctx, tests.as_deref(), &DuctProcessRunner) +} + +fn swift_test_ui_impl( + ctx: &Context, + tests: Option<&[String]>, + runner: &R, +) -> ToolResult { + let Some(tests) = tests.filter(|tests| !tests.is_empty()) else { + let mut message = "`tests` is required: name the suites or tests to run. Every one of \ + these launches the app and drives it through the screen, so there is \ + deliberately no way to ask for all of them; that is CI's job, through \ + `just test-app-ui`." + .to_owned(); + + // Asked of the bundle rather than read off a list someone maintains: + // a list in a document rots, and the one place that cannot is the + // bundle itself. + // + // Prepared first because enumeration reads the generated Xcode project, + // which is gitignored and produced by `prepare`. Reaching enumeration + // before it leaves the list unavailable on a fresh checkout, which is + // exactly where a caller most needs to be told what there is. A + // preparation failure needs no reporting of its own: enumeration then + // fails too, and the naming convention below covers both. + let names = match prepare(ctx, "debug", runner) { + Ok(None) => enumerate(ctx, runner).unwrap_or_default(), + _ => Vec::new(), + }; + + if names.is_empty() { + message.push_str( + "\n\nA name is a whole type path with a swift-testing function's trailing `()`, \ + such as `UISuite/ConversationListTests/clickSelects()`.", + ); + } else { + message.push_str("\n\nWhat there is to run:\n\n```\n"); + message.push_str(&names.join("\n")); + message.push_str("\n```\n"); + } + + return error(message); + }; + + if let Some(failure) = prepare(ctx, "debug", runner)? { + return error(failure); + } + + // Emptied before the run so what is collected afterwards belongs to it and + // not to the run before. + clear_screenshots(); + clear_result_bundle(&ctx.root); + + let (output, stopped) = run(ctx, tests, runner)?; + + // What the runner itself printed, in the order the sources can be trusted. + // + // A sealed bundle is the richest, and only a run that finished has one. The + // staged runner output covers the rest — every run stopped at its first + // failure — and holds the same expectations and comments as plain text. The + // log the tests keep themselves is last, and needed only for a failure + // recorded before the runner wrote anything. + let document = bundle_document(ctx, runner); + let reported = document + .as_ref() + .and_then(bundle_issues) + .or_else(|| collect_staged_issues(&ctx.root)) + .unwrap_or_else(collect_failures); + let detail = reported + &collect_screenshots(&ctx.root); + + if stopped.is_yes() { + close_leftover_apps(); + + return error(format!( + "Stopped the run at the first failure, so the tests after it did not run. Set `CI=1` \ + to let a run finish and report everything.{detail}" + )); + } + + let summary = match outcome(&output, "JP") { + Ok(summary) => summary, + Err(message) => return error(message + &detail), + }; + + // `xcodebuild` unions the `-only-testing` selectors and ignores any that + // match nothing, so a run naming one real test and one typo passes with the + // typo unmentioned. The bundle is the only record of what actually ran. + let executed = document.as_ref().map(executed_tests).unwrap_or_default(); + let missed = unmatched(tests, &executed); + if !missed.is_empty() { + return error(format!( + "These names matched no test, and `xcodebuild` passed over them rather than \ + failing:\n\n```\n{}\n```\n\nWhat did run:\n\n```\n{summary}\n```", + missed.join("\n") + )); + } + + Ok(format!("```\n{summary}\n```").into()) +} + +/// The requested names that nothing in `executed` answers to. +/// +/// A name is either a whole test (`Suite/test()`) or a suite standing for +/// everything beneath it, so a name is matched by an identifier that equals it +/// or continues it after a `/`. +/// +/// Empty when `executed` is, which is deliberate: an unreadable bundle means +/// the question cannot be answered, and answering "none of them ran" would fail +/// a suite that passed. +fn unmatched(requested: &[String], executed: &[String]) -> Vec { + if executed.is_empty() { + return Vec::new(); + } + + requested + .iter() + .filter(|name| { + !executed.iter().any(|id| { + id == *name + || id + .strip_prefix(name.as_str()) + .is_some_and(|rest| rest.starts_with('/')) + }) + }) + .cloned() + .collect() +} + +/// Every test in the UI bundle, as `xcodebuild` reports them. +/// +/// Costs a build, which is why it is only reached when a caller named nothing +/// and the run is not going to happen anyway. +/// +/// The JSON is walked for `identifier` keys rather than deserialized into the +/// document's shape: this is an error path, the shape has changed between Xcode +/// releases, and a parse failure here should cost the caller a less helpful +/// message rather than a second failure on top of the first. +fn enumerate(ctx: &Context, runner: &R) -> Result, std::io::Error> { + let output = runner.run( + "xcodebuild", + &[ + "test", + "-project", + PROJECT_PATH, + "-scheme", + SCHEME, + "-destination", + "platform=macOS", + "-enumerate-tests", + "-test-enumeration-style", + "flat", + "-test-enumeration-format", + "json", + ], + &ctx.root, + )?; + + let Some(start) = output.stdout.find('{') else { + return Ok(Vec::new()); + }; + + let Ok(json) = serde_json::from_str::(&output.stdout[start..]) else { + return Ok(Vec::new()); + }; + + let mut identifiers = Vec::new(); + collect_identifiers(&json, &mut identifiers); + + // `-enumerate-tests` reports the whole scheme whatever `-only-testing` + // says, so the unit bundle is in there too. The prefix comes off because + // this tool puts it back on. + let prefix = format!("{UI_BUNDLE}/"); + let mut names: Vec = identifiers + .iter() + .filter_map(|id| id.strip_prefix(&prefix)) + .map(str::to_owned) + .collect(); + + names.sort(); + names.dedup(); + + Ok(names) +} + +/// Gather every `identifier` string anywhere in `value`. +fn collect_identifiers(value: &serde_json::Value, into: &mut Vec) { + match value { + serde_json::Value::Object(map) => { + for (key, child) in map { + if key == "identifier" + && let Some(name) = child.as_str() + { + into.push(name.to_owned()); + } + collect_identifiers(child, into); + } + } + serde_json::Value::Array(items) => { + for item in items { + collect_identifiers(item, into); + } + } + _ => {} + } +} + +/// Run the named tests, stopping at the first failure unless under CI. +/// +/// One `-only-testing` argument each, which `xcodebuild` unions. +/// +/// `xcodebuild` passes over a name matching nothing: it runs whichever +/// selectors do match and exits zero, so a call naming one real test and one +/// typo would report the real one as a pass and say nothing about the typo. +/// The caller compares the result bundle's executed tests against what was +/// asked for, the bundle being the only record of which it is. +/// +/// Stopped from out here because nothing inside can do it. swift-testing has no +/// cancellation, and a test that exits its own process makes `xcodebuild` +/// relaunch the runner, finish the remaining tests, and report the whole thing +/// as passing. +fn run( + ctx: &Context, + tests: &[String], + runner: &R, +) -> Result<(ProcessOutput, Stopped), std::io::Error> { + let filters: Vec = tests.iter().map(|test| ui_bundle_filter(test)).collect(); + + let mut args = vec![ + "test", + "-project", + PROJECT_PATH, + "-scheme", + SCHEME, + "-destination", + "platform=macOS", + // Written where the report can read it back. Without this the bundle + // lands in derived data under a timestamped name, which would have to be + // scraped out of the log. + "-resultBundlePath", + RESULT_BUNDLE, + ]; + args.extend(filters.iter().map(String::as_str)); + + if under_ci() { + return Ok((runner.run("xcodebuild", &args, &ctx.root)?, Stopped::No)); + } + + runner.run_until("xcodebuild", &args, &ctx.root, &|line| { + line.contains(FAILURE_MARKER) + }) +} + +#[cfg(test)] +#[path = "test_ui_tests.rs"] +mod tests; diff --git a/.config/jp/tools/src/swift/test_ui_tests.rs b/.config/jp/tools/src/swift/test_ui_tests.rs new file mode 100644 index 000000000..a8381d3b5 --- /dev/null +++ b/.config/jp/tools/src/swift/test_ui_tests.rs @@ -0,0 +1,220 @@ +use jp_tool::{Action, Context}; +use pretty_assertions::assert_eq; + +use super::{super::error_message, *}; +use crate::util::runner::{ExitCode, MockProcessRunner, ProcessOutput}; + +fn ctx() -> Context { + Context { + root: "/repo".into(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + } +} + +/// A swift-testing summary line, verbatim from a real run. +const PASSING_SUMMARY: &str = + "\u{1005db} Test run with 2 tests in 2 suites passed after 11.2 seconds."; + +const CLEANED_SUMMARY: &str = "Test run with 2 tests in 2 suites passed after 11.2 seconds."; + +/// Expect the two preparation steps every Xcode run performs first. +fn prepared() -> MockProcessRunner { + MockProcessRunner::builder() + .expect("just") + .args(&["build-ffi", "debug"]) + .returns_success("") + .expect("xcodegen") + .returns_success("") +} + +#[test] +fn runs_each_named_test() { + let runner = prepared() + .expect("xcodebuild") + .args(&[ + "test", + "-project", + "apps/macos/JP.xcodeproj", + "-scheme", + "JP", + "-destination", + "platform=macOS", + "-resultBundlePath", + "tmp/uitests/run.xcresult", + "-only-testing:JPUITests/UISuite/ConversationListTests/clickSelects()", + "-only-testing:JPUITests/UISuite/ConversationListTests/labelsRows()", + ]) + .returns_success(PASSING_SUMMARY); + + let tests = [ + "UISuite/ConversationListTests/clickSelects()".to_owned(), + "UISuite/ConversationListTests/labelsRows()".to_owned(), + ]; + let result = swift_test_ui_impl(&ctx(), Some(&tests), &runner).unwrap(); + + assert_eq!( + result.unwrap_content(), + format!("```\nJP: {CLEANED_SUMMARY}\n```") + ); +} + +/// The whole point of the tool being separate: there is no way to ask it for +/// every UI test, because every one of them launches the app. +#[test] +fn refuses_to_run_without_names() { + let runner = prepared().expect("xcodebuild").returns_success( + r#"{"values":[{"enabledTests":[ + {"identifier":"JPUITests/UISuite/ConversationListTests/labelsRows()"}, + {"identifier":"JPTests/ConversationRefTests/exportsAsAURI()"} + ]}]}"#, + ); + + let message = error_message(swift_test_ui_impl(&ctx(), None, &runner).unwrap()); + + assert!(message.contains("`tests` is required"), "got: {message}"); + assert!(message.contains("just test-app-ui"), "got: {message}"); + + // Asked of the bundle, so the list cannot drift from what is really there. + // Named as this tool takes them: no bundle prefix, because it adds one. + assert!( + message.contains("\nUISuite/ConversationListTests/labelsRows()"), + "got: {message}" + ); + + // `-enumerate-tests` reports the whole scheme, and the unit tests are not + // this tool's to run. + assert!(!message.contains("ConversationRefTests"), "got: {message}"); +} + +/// The list is a nicety on a path that has already failed. +/// An `xcodebuild` too old to enumerate, or a shape this cannot read, costs the +/// caller the naming convention instead of a second failure. +#[test] +fn refuses_helpfully_when_enumeration_fails() { + let runner = prepared() + .expect("xcodebuild") + .returns_error("unknown option -enumerate-tests"); + + let message = error_message(swift_test_ui_impl(&ctx(), None, &runner).unwrap()); + + assert!(message.contains("`tests` is required"), "got: {message}"); + assert!(message.contains("trailing `()`"), "got: {message}"); +} + +/// An empty list is the same request as no list, and gets the same answer +/// rather than an `xcodebuild` run with no filter — which would run the whole +/// bundle, the one outcome this tool exists to prevent. +#[test] +fn refuses_to_run_with_an_empty_list() { + let runner = prepared().expect("xcodebuild").returns_success(""); + + let message = error_message(swift_test_ui_impl(&ctx(), Some(&[]), &runner).unwrap()); + + assert!(message.contains("`tests` is required"), "got: {message}"); +} + +/// A run is stopped from outside, so the marker that stops it has to be a line +/// `xcodebuild` really prints. +/// This is one, verbatim. +#[test] +fn recognizes_the_line_that_stops_a_run() { + let line = + "\u{1005db} Test \"selects a row\" recorded an issue at Foo.swift:12:5: Issue recorded"; + + assert!(line.contains(FAILURE_MARKER)); +} + +/// CI has nobody waiting on it and wants every result from the one run it gets, +/// so it opts out of stopping. +/// +/// Checked against the value rather than by setting `CI`: every test that runs +/// the tool reaches `under_ci`, so writing the variable here would race them. +#[test] +fn ci_is_the_variable_set_to_something() { + assert!(is_ci(Some("1"))); + assert!(is_ci(Some("true"))); +} + +/// The app's own Xcode scheme sets `CI` to an empty string, which must not read +/// as being under CI. +#[test] +fn an_empty_ci_variable_is_not_being_under_ci() { + assert!(!is_ci(Some(""))); +} + +#[test] +fn an_unset_ci_variable_is_not_being_under_ci() { + assert!(!is_ci(None)); +} + +/// The whole point of the check: `xcodebuild` unions the selectors and ignores +/// one that matches nothing, so without this a typo alongside a real name is a +/// passing run that quietly did half the work. +#[test] +fn names_the_requested_tests_that_never_ran() { + let executed = ["UISuite/ConversationListTests/clickSelects()".to_owned()]; + let requested = [ + "UISuite/ConversationListTests/clickSelects()".to_owned(), + "UISuite/ConversationListTests/clickSelcts()".to_owned(), + ]; + + assert_eq!(unmatched(&requested, &executed), vec![ + "UISuite/ConversationListTests/clickSelcts()".to_owned() + ]); +} + +/// A suite stands for everything under it, so naming one is answered by any +/// test beneath it rather than by an identifier equal to it. +#[test] +fn a_suite_is_matched_by_the_tests_inside_it() { + let executed = [ + "UISuite/ConversationListTests/clickSelects()".to_owned(), + "UISuite/ConversationListTests/labelsRows()".to_owned(), + ]; + + assert!(unmatched(&["UISuite/ConversationListTests".to_owned()], &executed).is_empty()); + assert!(unmatched(&["UISuite".to_owned()], &executed).is_empty()); +} + +/// A prefix that stops mid-name is not a suite of that name. +/// Matching on the bare string would let `ConversationList` answer for +/// `ConversationListTests`. +#[test] +fn a_partial_name_is_not_a_match() { + let executed = ["UISuite/ConversationListTests/clickSelects()".to_owned()]; + + assert_eq!( + unmatched(&["UISuite/ConversationList".to_owned()], &executed), + vec!["UISuite/ConversationList".to_owned()] + ); +} + +/// An unreadable bundle cannot answer the question, and answering "none of them +/// ran" would fail a suite that passed. +#[test] +fn nothing_is_unmatched_when_nothing_is_known() { + let requested = ["UISuite/ConversationListTests/clickSelects()".to_owned()]; + + assert!(unmatched(&requested, &[]).is_empty()); +} + +/// A failing run names the screenshots the tests left behind, so what was on +/// screen can be looked at rather than guessed. +#[test] +fn reports_a_failure_with_its_summary() { + let runner = prepared().expect("xcodebuild").returns(ProcessOutput { + stdout: "\u{1005db} Test \"selects a row\" recorded an issue at Foo.swift:12:5: Issue \ + recorded\n" + .to_owned(), + stderr: String::new(), + status: ExitCode::from_code(1), + }); + + let tests = ["UISuite/ConversationListTests/clickSelects()".to_owned()]; + let message = error_message(swift_test_ui_impl(&ctx(), Some(&tests), &runner).unwrap()); + + assert!(message.contains("recorded an issue"), "got: {message}"); +} diff --git a/.config/jp/tools/src/swift_tests.rs b/.config/jp/tools/src/swift_tests.rs new file mode 100644 index 000000000..b018d96a6 --- /dev/null +++ b/.config/jp/tools/src/swift_tests.rs @@ -0,0 +1,91 @@ +use jp_tool::{Action, Context}; +use pretty_assertions::assert_eq; + +use super::*; +use crate::util::runner::{ExitCode, MockProcessRunner, ProcessOutput}; + +fn ctx() -> Context { + Context { + root: "/repo".into(), + action: Action::Run, + access: None, + workspace_id: "test".into(), + conversation_id: "test".into(), + } +} + +/// Both generated inputs are built, in the order the Xcode build needs them: +/// the header must exist before the project is asked to scan it. +#[test] +fn prepare_builds_the_library_then_the_project() { + let runner = MockProcessRunner::builder() + .expect("just") + .args(&["build-ffi", "debug"]) + .returns_success("") + .expect("xcodegen") + .args(&[ + "generate", + "--spec", + "apps/macos/project.yml", + "--project", + "apps/macos", + ]) + .returns_success(""); + + assert_eq!(prepare(&ctx(), "debug", &runner).unwrap(), None); +} + +/// A failed library build short-circuits: the mock would panic on drop if +/// xcodegen had been expected and never run, which is what proves the ordering. +#[test] +fn prepare_stops_at_the_first_failure() { + let runner = MockProcessRunner::builder() + .expect("just") + .returns_error("boom"); + + let failure = prepare(&ctx(), "debug", &runner).unwrap(); + + assert_eq!( + failure, + Some("Building `jp_ffi` failed:\n\n```\nboom\n```".to_owned()) + ); +} + +#[test] +fn report_prefers_stdout() { + let output = ProcessOutput { + stdout: "from stdout".to_owned(), + stderr: "from stderr".to_owned(), + status: ExitCode::from_code(1), + }; + + assert_eq!(report(&output, "tool"), "from stdout"); +} + +/// Most tools report on stderr; only `xcodebuild` puts diagnostics on stdout. +#[test] +fn report_falls_back_to_stderr() { + let output = ProcessOutput { + stdout: String::new(), + stderr: "from stderr".to_owned(), + status: ExitCode::from_code(1), + }; + + assert_eq!(report(&output, "tool"), "from stderr"); +} + +/// Silence plus a non-zero status is still worth reporting, so the caller is +/// not left with an empty code block. +#[test] +fn report_names_the_program_when_it_said_nothing() { + let output = ProcessOutput { + stdout: String::new(), + stderr: String::new(), + status: ExitCode::from_code(70), + }; + + assert_eq!( + report(&output, "xcodebuild"), + "xcodebuild exited with status 70 and no diagnostics." + ); +} diff --git a/.config/jp/tools/src/util/runner.rs b/.config/jp/tools/src/util/runner.rs index a373bf6b9..dfdc240f4 100644 --- a/.config/jp/tools/src/util/runner.rs +++ b/.config/jp/tools/src/util/runner.rs @@ -1,7 +1,46 @@ //! Generic process runner abstraction for dependency injection in tests. +use std::{ + io::{BufRead as _, BufReader}, + process::Command, + thread, + time::{Duration, Instant}, +}; + use camino::Utf8Path; -use duct::cmd; +use duct::{ReaderHandle, cmd}; + +/// How long an interrupted process gets to clean up before it is killed. +const INTERRUPT_GRACE: Duration = Duration::from_secs(5); + +/// How often to check whether it has finished unwinding. +const INTERRUPT_POLL: Duration = Duration::from_millis(50); + +/// Stop `handle` the way Ctrl-C would, and kill it if that is not enough. +/// +/// `SIGINT` rather than an outright kill because a process that spawned others +/// is the only one that knows how to stop them. +/// `xcodebuild` interrupted tears down its test session, which is what stops +/// the app a UI test was driving; killed outright it leaves that app running on +/// the screen. +fn interrupt(handle: &ReaderHandle) { + for pid in handle.pids() { + let _sent = Command::new("kill") + .args(["-INT", &pid.to_string()]) + .status(); + } + + let deadline = Instant::now() + INTERRUPT_GRACE; + while Instant::now() < deadline { + if matches!(handle.try_wait(), Ok(Some(_))) { + return; + } + + thread::sleep(INTERRUPT_POLL); + } + + let _killed = handle.kill(); +} /// The exit code of a process. #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize)] @@ -146,6 +185,42 @@ pub trait ProcessRunner { working_dir: &Utf8Path, opts: &RunnerOpts<'_>, ) -> Result; + + /// Run `program`, killing it as soon as a line of its output satisfies + /// `stop`. + /// + /// Both streams are merged, because a caller watching for something has no + /// way to interleave two captures after the fact. + /// + /// The default runs to completion and reports that it stopped nothing, so a + /// runner that cannot stream still answers correctly — only later than a + /// caller would like. + fn run_until( + &self, + program: &str, + args: &[&str], + working_dir: &Utf8Path, + _stop: &dyn Fn(&str) -> bool, + ) -> Result<(ProcessOutput, Stopped), std::io::Error> { + let output = self.run(program, args, working_dir)?; + + Ok((output, Stopped::No)) + } +} + +/// Whether a run was cut short or reached its own end. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Stopped { + /// The process was killed because a line matched. + Yes, + /// The process ended on its own. + No, +} + +impl Stopped { + pub const fn is_yes(self) -> bool { + matches!(self, Self::Yes) + } } /// Production implementation that uses duct to run actual external processes. @@ -222,6 +297,59 @@ impl ProcessRunner for DuctProcessRunner { status: ExitCode::from(output.status), }) } + + fn run_until( + &self, + program: &str, + args: &[&str], + working_dir: &Utf8Path, + stop: &dyn Fn(&str) -> bool, + ) -> Result<(ProcessOutput, Stopped), std::io::Error> { + let handle = cmd(program, args) + .dir(working_dir) + .unchecked() + .stderr_to_stdout() + .reader()?; + + let mut reader = BufReader::new(&handle); + let mut collected = String::new(); + let mut line = String::new(); + let mut stopped = Stopped::No; + + loop { + line.clear(); + // Lossy for the same reason the captured path is: one stray byte + // must not discard the rest of the output. + let mut bytes = Vec::new(); + if reader.read_until(b'\n', &mut bytes)? == 0 { + break; + } + line.push_str(&String::from_utf8_lossy(&bytes)); + collected.push_str(&line); + + if stop(&line) { + stopped = Stopped::Yes; + interrupt(&handle); + break; + } + } + + // A killed process has no status of its own worth reporting, and the + // caller already knows it was killed. + let status = match handle.try_wait() { + Ok(Some(output)) => ExitCode::from(output.status), + _ => ExitCode::from(None), + }; + + Ok(( + ProcessOutput { + stdout: collected, + stderr: String::new(), + status, + }, + stopped, + )) + } } #[cfg(test)] @@ -234,7 +362,16 @@ use std::{ struct Expectation { program: String, args: Option>, - output: ProcessOutput, + + /// What running the command does: what it printed, or the kind of error + /// spawning it produced. + /// + /// A binary that is not installed fails to spawn, which is a different + /// outcome from one that ran and exited non-zero and reaches different code + /// in the caller. + /// `ErrorKind` rather than `io::Error` because an expectation is stored and + /// `io::Error` is not `Clone`. + result: Result, } #[cfg(test)] @@ -346,10 +483,29 @@ impl ExpectationBuilder { /// Set the output to return. pub fn returns(self, output: ProcessOutput) -> MockProcessRunner { + self.returns_result(Ok(output)) + } + + /// Fail to spawn the command, as a binary that is not installed does. + /// + /// Distinct from [`returns_error`], which models a command that ran and + /// exited non-zero. + /// A caller that handles the two differently cannot be tested with the + /// other one. + /// + /// [`returns_error`]: Self::returns_error + pub fn fails_to_spawn(self) -> MockProcessRunner { + self.returns_result(Err(std::io::ErrorKind::NotFound)) + } + + fn returns_result( + self, + result: Result, + ) -> MockProcessRunner { self.expectations.lock().unwrap().push_back(Expectation { program: self.program, args: self.args, - output, + result, }); MockProcessRunner { @@ -412,7 +568,7 @@ impl ProcessRunner for MockProcessRunner { } } - Ok(expectation.output) + expectation.result.map_err(std::io::Error::from) } } diff --git a/.jp/config/personas/dev.toml b/.jp/config/personas/dev.toml index 8035d43ef..fbb1284f0 100644 --- a/.jp/config/personas/dev.toml +++ b/.jp/config/personas/dev.toml @@ -12,6 +12,7 @@ extends = [ "../skill/edit-files.toml", "../skill/git-reading.toml", "../skill/rust-development.toml", + "../skill/swift-development.toml", "../skill/github-reader.toml", "../skill/unix.toml", "../skill/coding.toml", diff --git a/.jp/config/personas/rfd-implementor.toml b/.jp/config/personas/rfd-implementor.toml index 1315ec014..27acb64a2 100644 --- a/.jp/config/personas/rfd-implementor.toml +++ b/.jp/config/personas/rfd-implementor.toml @@ -10,6 +10,7 @@ extends = [ "../skill/edit-files.toml", "../skill/git-reading.toml", "../skill/rust-development.toml", + "../skill/swift-development.toml", "../skill/github-reader.toml", "../skill/project-discourse.toml", "../skill/unix.toml", diff --git a/.jp/config/skill/swift-development.toml b/.jp/config/skill/swift-development.toml new file mode 100644 index 000000000..44fd81f2c --- /dev/null +++ b/.jp/config/skill/swift-development.toml @@ -0,0 +1,246 @@ +[[assistant.system_prompt_sections]] +tag = "swift_development_skill" +title = "Skill: Swift Development" +content = """\ +You have been given the Swift development skill. You are an expert at writing Swift code for the \ +native macOS app in `apps/macos`, which reads JP conversations through the `jp_ffi` C ABI. The \ +following tools help you with this: + +- swift_check: Build the app, reporting compiler diagnostics. +- swift_test: Run the fast suites — the app's unit tests and the `jpdrive` package. +- swift_test_ui: Run named UI tests, which drive the app through the screen. Stops at the first \ +failure unless `CI` is set. +- swift_format: Format the app's Swift sources, or report violations without rewriting. + +Each build tool brings its own inputs up to date first (the `jp_ffi` static library, its generated \ +C header, and the Xcode project), so no setup step is needed. + +The Xcode project is generated from `apps/macos/project.yml` and is not committed. Change the \ +manifest, never the generated `.xcodeproj`. +""" + +[conversation.tools] +swift_check = { enable = true, run = "unattended", result = "unattended", style.inline_results = "off", style.results_file_link = "off" } +swift_test = { enable = true, run = "unattended", result = "unattended", style.inline_results = "full", style.results_file_link = "off" } +swift_test_ui = { enable = true, run = "ask", result = "unattended", style.inline_results = "full", style.results_file_link = "off" } +swift_format = { enable = true, run = "unattended", result = "unattended", style.inline_results = "off", style.results_file_link = "off" } + +[[assistant.instructions]] +title = "Swift Project Conventions" +items = [ + """\ + The app is held to the same bar as the Rust code: strict compiler modes, warnings as errors, \ + and tests for every behavior. There is no "it's only the UI" exemption.\ + """, + """\ + Swift 6 language mode with complete concurrency checking is on, and warnings are errors. Do \ + not silence a diagnostic with `@unchecked Sendable`, `nonisolated(unsafe)`, or an `@preconcurrency` \ + import without saying in a comment why the code is actually safe.\ + """, + """\ + Add doc comments (`///`) for every type, method, and property, and clarify non-obvious logic \ + with inline comments (`//`). The audience is a reader who has never seen the FFI boundary.\ + """, + """\ + Tests live in `apps/macos/Tests`, use swift-testing (`import Testing`, `@Test`, `#expect`), and \ + reach app-internal types through `@testable import JP`.\ + """, + """\ + UI tests live in `apps/macos/UITests` and run in their own process, so `@testable import JP` \ + is not available and must not be reached for. Anything checkable in-process belongs in \ + `apps/macos/Tests`, where it runs in milliseconds instead of costing an app launch.\ + """, + """\ + No test may touch the *system* pasteboard. A debug build copies wherever `JP_DEBUG_PASTEBOARD` \ + names, so a test points the app at a private pasteboard and reads that back; \ + `ClipboardPolicyTests` fails on any spelling of the general one.\ + """, + """\ + Anything a UI test needs the app to do differently goes through `DebugState`, gated on \ + `#if DEBUG` and off unless an environment variable says otherwise — the pasteboard and \ + animations both work this way. A release build must have no way to reach the test behaviour.\ + """, + """\ + A new animation goes through `DebugState.animated(_:)`. XCUITest waits for the app to stop \ + moving before every action it synthesizes, so an animation is time added to every test that \ + triggers one, and one lever turning them all off is worth keeping whole.\ + """, + """\ + Wait on a condition, never on the clock, and never through `waitForExistence`: it reports an \ + element about a second after it appears, whatever the element. `AppUnderTest.wait(for:)` asks \ + again instead, which finds it in under 100ms, and the timeout (one second) is the price of a \ + failure rather than of a pass.\ + """, + """\ + A UI suite shares one launched app through `.sharedApp(...)`, because launching costs about \ + four seconds and the work under test costs milliseconds. Leave the app as you found it, or \ + order the suite so what one test leaves is what the next expects. A test needing an untouched \ + app launches its own and says why.\ + """, + """\ + Run `swift_format` and `swift_check` before considering a change done, and `swift_test` before \ + considering it correct. A change to the UI suite also needs the tests you touched run by name \ + through `swift_test_ui`; the whole bundle is CI's job.\ + """, + """\ + Anything published through `@FocusedValue` or `focusedSceneValue` must compare equal to itself \ + between renders. A struct of closures never does, and the App-body loop that follows re-renders \ + the whole scene continuously: menus lose the items AppKit injects, and the entire app turns \ + sluggish.\ + """, + """\ + `accessibilityIdentifier` does not reach the `NSMenuItem` SwiftUI bridges a menu button to, \ + on the button or on its label, so every item in a menu reports the same selector name and is \ + addressable only by title.\ + """, + """\ + A `RawRepresentable` silently inherits behaviour from its raw value: `Codable` that encodes \ + through `rawValue`, which recurses forever if `rawValue` uses `JSONEncoder`, and `==` that \ + compares raw values, which is unstable when the raw value is JSON holding a dictionary. Encode \ + a separate private `Codable` struct, and write `==` out by hand.\ + """, +] + +[[assistant.instructions]] +title = "Swift Code Style Rules" +description = """\ +Hard rules for Swift in this project. The goal is code that reasons like the Rust it calls into: \ +ownership, errors, and concurrency all visible at the use site. Violating any of these is a bug.\ +""" +items = [ + """\ + **Use typed throws.** Write `throws(WorkspaceError)`, not bare `throws`. A caller should know \ + what can go wrong from the signature. Give a `do` block an explicit `do throws(E)` when its \ + `catch` needs the concrete type rather than `any Error`.\ + """, + """\ + **Model unique ownership with `~Copyable`.** A type that owns a resource requiring exactly one \ + release (an FFI handle, a file descriptor) is a noncopyable struct with a `deinit`, not a class. \ + That makes a double free a compile error instead of a crash.\ + """, + """\ + **Never force unwrap or force try.** No `!`, no `try!`, no implicitly unwrapped optionals. Use \ + `guard let`, `if let`, or a typed throw. The formatter enforces this; do not work around it.\ + """, + """\ + **Spell existentials `any P`.** The `ExistentialAny` upcoming feature is on, so a witness table \ + lookup is visible at the use site. Prefer a generic parameter when the concrete type is known.\ + """, + """\ + **Copy owned data across the FFI boundary.** Read the bytes out of a C pointer, hand the pointer \ + straight back to the library that allocated it, and never let a pointer, a lock guard, or a \ + borrow outlive the call that produced it.\ + """, + """\ + **Keep FFI calls off the main thread.** The Rust side takes locks and touches the filesystem. \ + Wrap reads in a detached task and return `Sendable` values from it.\ + """, + """\ + **Treat Swift mirrors of Rust types as a contract.** A `Decodable` struct mirroring a Rust \ + payload has no compiler checking it agrees. Pin the exact JSON in a test on both sides, and \ + ignore unknown keys so a field added in Rust does not break an older app.\ + """, + """\ + **Use early exits.** `guard` for preconditions at the top of a function rather than nesting the \ + body inside an `if`.\ + """, +] + +[[assistant.instructions.examples]] +good = """\ +init(path: String) throws(WorkspaceError) { + guard let handle = jp_workspace_open(path) else { + throw Self.lastError() + } + self.handle = handle +}\ +""" +bad = """\ +init(path: String) throws { + self.handle = jp_workspace_open(path)! +}\ +""" +reason = """\ +The typed throw tells the caller what can fail; the force unwrap turns a reported error into a \ +crash and discards the message the library left behind.\ +""" + +[[assistant.instructions.examples]] +good = """\ +struct WorkspaceReader: ~Copyable { + private let handle: OpaquePointer + + deinit { jp_workspace_close(handle) } +}\ +""" +bad = """\ +final class WorkspaceReader { + private let handle: OpaquePointer + + deinit { jp_workspace_close(handle) } +}\ +""" +reason = """\ +Noncopyable gives one owner and one release, checked at compile time. The class relies on ARC and \ +lets a second reference outlive the close.\ +""" + +[[assistant.instructions.examples]] +good = """\ +private static func take(_ raw: UnsafeMutablePointer) -> Data { + defer { jp_string_free(raw) } + return Data(bytes: raw, count: strlen(raw)) +}\ +""" +bad = """\ +private static func take(_ raw: UnsafeMutablePointer) -> String { + return String(cString: raw) +}\ +""" +reason = """\ +The good version copies the bytes and returns the allocation to the library that made it. The bad \ +version leaks, because Rust frees what Rust allocates.\ +""" + +[[assistant.instructions.examples]] +good = """\ +let read = Task.detached { () -> Result<[ConversationSummary], WorkspaceError> in + do throws(WorkspaceError) { + let reader = try WorkspaceReader(path: path) + return .success(try reader.conversations()) + } catch { + return .failure(error) + } +} +return await read.value\ +""" +bad = """\ +return await Task.detached { + do { + return .success(try WorkspaceReader(path: path).conversations()) + } catch { + return .failure(error) + } +}.value\ +""" +reason = """\ +The explicit closure return type and `do throws(E)` give `catch` a concrete error. Without them the \ +binding is `any Error` and needs a downcast.\ +""" + +[[assistant.instructions.examples]] +good = """\ +/// When the conversation was last activated, as RFC 3339 text. +/// +/// Deliberately unparsed: the Rust side emits fractional seconds whenever the +/// stored timestamp has them, which `JSONDecoder`'s `.iso8601` strategy rejects. +let lastActivatedAt: String\ +""" +bad = """\ +@available(*, deprecated) +let lastActivatedAt: Date\ +""" +reason = """\ +The comment records the constraint that makes the odd-looking type correct. A future reader would \ +otherwise "fix" it to `Date` and break every real workspace.\ +""" diff --git a/.jp/mcp/tools/swift/check.toml b/.jp/mcp/tools/swift/check.toml new file mode 100644 index 000000000..23a8701ac --- /dev/null +++ b/.jp/mcp/tools/swift/check.toml @@ -0,0 +1,26 @@ +[conversation.tools.swift_check] +enable = false +run = "unattended" +source = "local" +command = "just serve-tools {{context}} {{tool}}" +summary = "Build the macOS app in `apps/macos`, validating that the Swift code compiles. The project treats warnings as errors, so anything reported is a failure. Builds the `jp_ffi` static library and regenerates the Xcode project first, so no setup step is needed." + +examples = """ +```json +{} +``` +Builds the Debug configuration. + +```json +{"configuration": "Release"} +``` +""" + +[conversation.tools.swift_check.style] +inline_results = "full" +results_file_link = "off" +parameters = "function_call" + +[conversation.tools.swift_check.parameters.configuration] +summary = "Xcode build configuration to build. Defaults to `Debug`." +type = "string" diff --git a/.jp/mcp/tools/swift/format.toml b/.jp/mcp/tools/swift/format.toml new file mode 100644 index 000000000..67b717141 --- /dev/null +++ b/.jp/mcp/tools/swift/format.toml @@ -0,0 +1,27 @@ +[conversation.tools.swift_format] +enable = false +run = "unattended" +source = "local" +command = "just serve-tools {{context}} {{tool}}" +summary = "Format the macOS app's Swift sources with `swift format`, using `apps/macos/.swift-format`. Set `check` to report violations without rewriting anything." + +examples = """ +```json +{} +``` +Rewrites files in place. + +```json +{"check": true} +``` +Reports violations and fails if any are found. +""" + +[conversation.tools.swift_format.style] +inline_results = "full" +results_file_link = "off" +parameters = "function_call" + +[conversation.tools.swift_format.parameters.check] +summary = "Report formatting and lint violations instead of rewriting files. Defaults to `false`." +type = "boolean" diff --git a/.jp/mcp/tools/swift/test.toml b/.jp/mcp/tools/swift/test.toml new file mode 100644 index 000000000..7a09fccac --- /dev/null +++ b/.jp/mcp/tools/swift/test.toml @@ -0,0 +1,41 @@ +[conversation.tools.swift_test] +enable = false +run = "unattended" +source = "local" +command = "just serve-tools {{context}} {{tool}}" +summary = "Run the fast Swift test suites: the macOS app's unit tests, and the `jpdrive` accessibility driver package. Unit tests link the `jp_ffi` static library and call it, so a failure there can mean either the Swift or the FFI boundary is broken. Builds the library and regenerates the Xcode project first, so no setup step is needed. The driver package needs neither, and runs first because it is fast. The app's UI tests are deliberately out of reach here — they launch the app and take over the screen; use `swift_test_ui` and name the ones you want." + +examples = """ +```json +{} +``` +Runs everything. + +```json +{"target": "drive"} +``` +Runs the driver package only, skipping the Xcode build entirely. + +```json +{"target": "app", "testname": "WorkspaceReaderTests"} +``` +Runs one suite of the app's unit tests. + +```json +{"target": "drive", "testname": "Tree"} +``` +Runs one suite of the driver package. +""" + +[conversation.tools.swift_test.style] +inline_results = "full" +results_file_link = "off" +parameters = "function_call" + +[conversation.tools.swift_test.parameters.testname] +summary = "Restrict the run to a suite (`SuiteName`) or a single test (`SuiteName/testName()`). Runs everything when unspecified. A nested suite is named by its whole type path, and a swift-testing function keeps its trailing `()`. Pair this with `target`: a name that matches nothing in one of the suites fails the run, and a suite name almost never exists in both." +type = "string" + +[conversation.tools.swift_test.parameters.target] +summary = "Which suites to run: `app` for the macOS app's unit tests, `drive` for the `jpdrive` accessibility driver package, or `all` for both. Defaults to `all`." +type = "string" diff --git a/.jp/mcp/tools/swift/test_ui.toml b/.jp/mcp/tools/swift/test_ui.toml new file mode 100644 index 000000000..b988562ff --- /dev/null +++ b/.jp/mcp/tools/swift/test_ui.toml @@ -0,0 +1,33 @@ +[conversation.tools.swift_test_ui] +enable = false +run = "unattended" +source = "local" +command = "just serve-tools {{context}} {{tool}}" +summary = "Run named UI tests against the macOS app. Each one drives the app through the screen, so a run takes over the display and cannot be used while the machine is busy. Names are required and there is no way to ask for the whole bundle: run the two or three you are working on, and leave the full suite to CI (`just test-app-ui`). The run stops at the first failure and closes the app it left behind; set `CI=1` to let it finish and report everything. Builds the library and regenerates the Xcode project first, so no setup step is needed. A failure reports what the tests recorded and copies their screenshots into `tmp/uitests/`." + +examples = """ +```json +{"tests": ["UISuite/ConversationListTests/clickSelects()"]} +``` +Runs one test. This is the red-green loop: about six seconds. + +```json +{"tests": ["UISuite/ConversationListTests/clickSelects()", "UISuite/ConversationListTests/arrowKeysMoveSelection()"]} +``` +Runs two, in one launch of `xcodebuild`. + +```json +{"tests": ["UISuite/ConversationListTests"]} +``` +Runs a whole suite. Costs a launch of the app per test in it, so reach for +this when finishing a section rather than while iterating. +""" + +[conversation.tools.swift_test_ui.style] +inline_results = "full" +results_file_link = "off" +parameters = "function_call" + +[conversation.tools.swift_test_ui.parameters.tests] +summary = "The suites or tests to run, as `SuiteName` or `SuiteName/testName()`. Required, and at least one. A nested suite is named by its whole type path (`UISuite/ConversationListTests`), and a swift-testing function keeps its trailing `()`. A name matching nothing fails the run rather than being passed over, so a typo is reported instead of quietly shrinking it. Pass an empty list to be told what there is, asked of the built bundle." +type = "array" diff --git a/justfile b/justfile index 2266443f1..8638e4321 100644 --- a/justfile +++ b/justfile @@ -365,46 +365,109 @@ open-app: build-app # runs those. [group('test')] [macos] -test-app: gen-app (build-ffi "debug") - xcodebuild test -project apps/macos/JP.xcodeproj -scheme JP \ - -destination platform=macOS -only-testing:JPTests -quiet +test-app FILTER="": gen-app (build-ffi "debug") + @just _xctest JPTests "" {{quote(FILTER)}} -# Run every one of the macOS app's UI tests. +# Run the macOS app's UI tests, or the ones FILTER names. # -# Takes over the screen for the length of the run. This is the CI job; while -# writing a test, run it by name through the `swift_test_ui` tool instead, which -# stops at the first failure. +# Takes over the screen for the length of the run. # -# Every test runs here even after one fails, which is what `CI` means to that -# tool and what a run nobody is watching should do. +# just test-app-ui +# just test-app-ui UISuite/PointerCursorTests +# just test-app-ui 'UISuite/PointerCursorTests/showsResizeCursorOverTheDivider()' # -# The result bundle is written into the checkout rather than left in derived -# data, so a failing run leaves its evidence somewhere a reader or a CI artifact -# step can reach without deriving a container path. `swift_test_ui` writes to -# the same place for the same reason. +# A single test needs its parentheses, and quoting to keep the shell off them. +# +# With no filter this is the CI job: every test runs even after one fails, which +# is what a run nobody is watching should do. Naming a test drops that, so an +# iterating run stops at the first failure. [group('test')] [macos] -test-app-ui: gen-app (build-ffi "debug") +test-app-ui FILTER="": gen-app (build-ffi "debug") #!/usr/bin/env sh set -eu - # Not tidying up: `xcodebuild` refuses to write over an existing bundle, so - # without this the second run in a checkout fails before it starts. - rm -rf tmp/uitests/run.xcresult - mkdir -p tmp/uitests + # CI only for the unattended whole-suite run, where finishing and reporting + # everything beats stopping early. A named run is someone iterating, and they + # want the first failure. + if [ -n "{{FILTER}}" ]; then + just _xctest JPUITests "" {{quote(FILTER)}} + else + just _xctest JPUITests 1 "" + fi + +# Run one of the app's test bundles, reporting what failed. +# +# `-quiet` keeps a passing run to a couple of lines, and also suppresses the +# failure summary — a failing run prints `** TEST FAILED **` and nothing else, +# which is not something anyone can act on. The result bundle is what makes the +# failure readable, so it is always written and read back when the run fails. +# +# It is written inside the checkout rather than left in derived data, so a +# failing run leaves its evidence where a reader or a CI artifact step can reach +# it without deriving a container path. +# +# FILTER narrows the run to `Suite`, `Suite/test()`, or a nested +# `Outer/Inner/test()`. The names are the *type* names, not the display names a +# `@Suite("...")` or `@Test("...")` gives them; a nested suite needs its whole +# path, and a single test needs its parentheses. +# +# A filter matching nothing is reported rather than passing quietly — which is +# what it does on its own, since a run that tested nothing still exits zero. +[private] +[macos] +_xctest BUNDLE CI FILTER="": + #!/usr/bin/env sh + set -eu - # Captured rather than propagated, so the bundle is still reported on the - # failing run — which is the only run anybody opens it for. - status=0 - CI=1 xcodebuild test -project apps/macos/JP.xcodeproj -scheme JP \ - -destination platform=macOS -only-testing:JPUITests \ - -resultBundlePath tmp/uitests/run.xcresult -quiet || status=$? + target="{{BUNDLE}}" + if [ -n "{{FILTER}}" ]; then + target="{{BUNDLE}}/{{FILTER}}" + fi + + result="apps/macos/.build/{{BUNDLE}}.xcresult" + rm -rf "$result" + + # Exported only when asked for, never as an empty string: the tests ask + # whether `CI` is set, not what it says, so `CI=""` would read as CI. + if [ -n "{{CI}}" ]; then + export CI="{{CI}}" + fi + + if xcodebuild test \ + -project apps/macos/JP.xcodeproj -scheme JP \ + -destination platform=macOS -only-testing:"$target" \ + -resultBundlePath "$result" -quiet + then + # `xcodebuild` exits zero when a filter matches nothing, so a run that + # tested nothing looks exactly like one that passed. Read the count out of + # the summary rather than grepping its prose, which is how the first + # version of this check missed exactly the case it was written for. + ran=$(xcrun xcresulttool get test-results summary --path "$result" 2>/dev/null | + jq -r '.totalTestCount // 0') + + if [ "$ran" -eq 0 ]; then + echo "No test matched '$target'." >&2 + echo >&2 + echo "Names are type names, not the display names in @Suite(\"...\") or" >&2 + echo "@Test(\"...\"). A nested suite needs its full path, and a single" >&2 + echo "test needs its parentheses:" >&2 + echo >&2 + echo " UISuite/PointerCursorTests" >&2 + echo " UISuite/PointerCursorTests/showsResizeCursorOverTheDivider()" >&2 + exit 1 + fi - if [ -d tmp/uitests/run.xcresult ]; then - echo "result bundle: tmp/uitests/run.xcresult" >&2 + exit 0 fi - exit $status + echo >&2 + echo "--- $target failures ---------------------------------------------" >&2 + xcrun xcresulttool get test-results summary --path "$result" >&2 || true + echo >&2 + echo "Full results: $result" >&2 + + exit 1 # Format the macOS app's Swift sources. [group('fmt')]