test(pty): reuse a single shell session across PTY tests - #439
Merged
Conversation
Contributor
|
🚀 Web preview: https://preview-439.treq-9zy.pages.dev |
Core no longer spawns processes directly, so the PTY integration suite is now the dominant source of subprocess spawns in CI — and each spawned shell (PowerShell on Windows) costs multiple seconds under CI contention. Merge tests that only needed "a working shell" onto one shared session each, cutting spawns in this file from 17 down to 8: - test_session_lifecycle_and_io replaces 7 single-assertion tests (create, write, resize, utf8 output x3, set_auto_command, close, close-terminates-process). - test_echo_suppression_filtering replaces 3 filter-behavior tests, each stage resolving its filter state before the next stage runs. Tests that need genuinely separate or concurrent processes (multi-session, isolation, creation-time params) are left untouched.
remote_commit_file/remote_commit_on_branch used to clone the bare remote, checkout/add/commit/push through 5 git subprocesses per call — real spawns across ~30 call sites in the test suite. The "remote" in these tests is always a local bare repo on the same filesystem, so there's no need to clone at all: write the commit straight into its object database and refs via gix (new set_path_in_tree + gix_commit_file_on_ref), based on the branch's current tip if it exists there already. Also drops the last two non-network git subprocess calls from with_remote_create: `git remote set-head` only ever writes a local symbolic ref, so it becomes a gix ref-transaction (set_symbolic_ref, generalized from the existing set_head_branch), and the bare remote gets its own git user config so gix's repo.commit() can resolve author/ committer without falling back to global git config. The two `git push` calls remain as subprocesses — gix 0.81 has no push support — as does the real `push_branch` push-from-local-repo helper, which is unrelated to the removed clone-and-push pattern. Verified with a throwaway test asserting `git fsck` cleanliness and correct tree/commit structure against the real git binary (not part of this commit); full jj-backed test run wasn't possible in this sandbox (no jj binary available), but the 12 unrelated failures observed were all "jj: No such file or directory", including one from a test that calls remote_commit_on_branch and only failed on its later jj-dependent step.
Mirrors the earlier git-to-gix migration: TestRepo::run_jj and the direct
Command::new("jj") calls scattered across ~15 test files shelled out to the
jj CLI for operations (new, describe, commit, status, log with custom
templates, sparse list, file list, bookmark set, workspace update-stale/
forget, git init) that mostly already have jj-lib-backed equivalents in
src/jj.rs — reuse those directly instead of a subprocess round trip.
New thin wrappers in e2e_test_helpers.rs (jj_new, jj_describe, jj_commit,
jj_change_id, jj_update_stale, jj_working_copy_is_clean, jj_snapshot,
jj_sparse_patterns, jj_files_at_revision, jj_git_init, jj_set_bookmark,
jj_bookmarks_on_revision, jj_change_ids_in_revset/jj_commit_ids_in_revset,
jj_log_entries/jj_log_descriptions, jj_has_revert_hunk_for_line) delegate to
existing treq_lib::jj functions per call. Three genuinely missing read
accessors were added to src/jj.rs: jj_get_sparse_patterns,
jj_list_files_at_revision, jj_git_init_bare (jj_lib's `jj git init` without
--colocate), plus jj_log_revset_change_ids/jj_log_entries for the log-with-
custom-template call sites and list_all_workspace_names for JjVerifier's
unfiltered `jj workspace list` (list_jj_workspaces deliberately hides
`default`, which some of these tests specifically assert on).
Resolving a bookmark/branch-name revision needs a git-ref import first
(the real `jj` CLI does this on every invocation; raw jj-lib calls don't) —
handled via a best-effort jj_util_import_git_refs call in the wrappers that
take arbitrary revisions/revsets.
TestRepo::run_jj is renamed run_jj_cli and kept for exactly one purpose: a
new #[cfg(unix)] jj_lib_vs_cli_test.rs differential-tests the wrappers above
against the real CLI's own view of a repo, when jj is on PATH (skips
otherwise). Unix-only because CI doesn't reliably have the jj CLI on
Windows runners (see the earlier Windows-test-flakiness investigation).
…ture file_indexer_test.rs's setup_jj_repo builds a bare, non-colocated jj repo (no .git directory at all — jj_git_init_bare, i.e. plain jj-lib "jj git init" without --colocate). TestRepo::jj_commit wraps treq's production jj_commit, which resolves a git branch as part of treq's normal colocated-repo flow and fails with "Failed to read .git/HEAD" here since there's no .git to read. describe-then-new is what `jj commit -m` does under the hood and needs no git-branch concept, so it works on this fixture the same way the original `jj commit -m` CLI call did. Caught by actually running the test suite after the previous commit's jj CLI-to-jj-lib migration (this sandbox lacks a jj CLI, but a build+test run of the affected files was still possible and caught this one).
…etup The repo-wide lint (RUST_DIRS = src-tauri/src only) flags pub functions with no caller in production code, since tests/ isn't scanned. The 6 jj-lib accessors added for the CLI-to-jj-lib test migration had no such caller. tauri_test_bridge.rs is the established exemption for test-only pub surface, but it's gated behind the "tauri-test" feature (only built for the NAPI addon) and not linked into the plain `cargo test`/nextest crate the tests/*.rs integration tests actually compile against — moving them there would make them uncallable from tests/e2e_test_helpers.rs. So instead: add direct unit tests for each in jj.rs's own #[cfg(test)] module (src-tauri/src, real coverage, satisfies the lint honestly rather than working around it). Also fixes jj_lib_vs_cli_test.rs: several of its differential tests used TestRepo::new_without_init(), which skips core::init() entirely — no .jj directory ever gets created — so every jj-lib call failed with "There is no Jujutsu repo". This sandbox has no jj CLI to catch it locally; CI's jj-CLI-backed run surfaced it. Switched those to TestRepo::new().
Ziinc
force-pushed
the
claude/windows-tests-slow-flaky-97pv3q
branch
from
September 10, 2026 09:19
c3624a7 to
bff9e7c
Compare
import_git_refs_best_effort (the "make a git-only bookmark resolvable as a jj revision" best-effort call added to several test wrappers) called jj_util_import_git_refs, which is really reconcile_colocated_home_repo — full git-HEAD/working-copy reconciliation for the home repo, not a plain git-refs-into-bookmarks import. Harmless before, but main's rebase fixed a bug in that reconciliation's working-copy-reset condition, and running it as a side effect of every change-id/revset resolution now disturbs home-repo working-copy state before conflict-producing jj_new calls could run — core_changes_test.rs's conflict tests were resolving to no conflicts at all. Added jj_import_remaining_git_refs: the narrow git::import_refs step alone (imports branch refs into jj bookmarks, no git-HEAD/working-copy touching) — the operation the "best effort" comment actually described — and switched the test wrapper to use it instead.
Five tests failed once run against a real jj CLI (ubuntu/macos rust jobs; this sandbox has none): - jj_get_sparse_patterns: full checkout is represented as a single root pattern (internal path ""), not an empty pattern list. Test assumption was wrong; fixed the expectation, not the production accessor. - jj_import_remaining_git_refs unit test: compared the imported git-only branch against jj's own working-copy commit (@), but `jj git init --colocate` doesn't move git HEAD to jj's new empty @ until something reconciles them — a plain `git branch` (which points at HEAD) still resolves to the pre-init commit. Compare against that commit instead. - jj_lib_vs_cli_test.rs (3 tests): a jj-lib write followed immediately by a real `jj` CLI read of the same symbolic revision could disagree, because the CLI's first-ever touch on one of these repos performs its own git-HEAD/jj-@ reconciliation as a side effect (the same class of desync as above) — shifting @ between the wrapper's return value and the CLI's later read of "@". Added a settle_cli() warm-up call right after repo creation in every test, before any state gets captured for comparison, so that one-time reconciliation happens harmlessly upfront.
Collaborator
Author
|
Pushed fixes for the 5 rust-test failures from the last run (all caught once tests actually ran against a real
Two failures on the last run are not from this PR and I'm not touching them here:
Watching for the next run. Generated by Claude Code |
…atches_cli The CLI's log query for the description was itself a subprocess touch that snapshots/reconciles the colocated repo, shifting @ before the wrapper's own clean-check ran. Capture the wrapper's post-describe state before any further CLI subprocess call.
… workspace The home-repo-root colocated path triggers reconcile_colocated_home_repo on every jj_get_changed_files call, which silently checks @ back out to the stale Git HEAD when the wrapper's raw jj_new/jj_describe calls (unlike jj_commit) never exported to Git HEAD, discarding the just-described commit. Run the lifecycle against a created workspace instead, which isn't subject to that home-repo reconciliation and matches how the app actually drives these primitives.
Collaborator
Author
|
None of these files are touched by this PR (Rust-test-only: Generated by Claude Code |
Collaborator
Author
|
I looked into the CI failure on this PR (run 34487910076, attempt 2), specifically the The 5 failing tests are:
None of these live in files this PR touches — the diff only changes I also checked recent CI runs on Given that, I'm not pushing a fix here since it would be unrelated to this PR's purpose (deflaking/reusing PTY shell sessions in tests) and to keep the diff focused. Someone should file/fix these separately as a Windows path-normalization and Git-checkout-reconcile issue on Generated by Claude Code |
…slow-flaky-97pv3q # Conflicts: # src-tauri/tests/pty_tests.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Core no longer spawns processes directly, so the PTY integration suite is
now the dominant source of subprocess spawns in CI — and each spawned shell
(PowerShell on Windows) costs multiple seconds under CI contention. Merge
tests that only needed "a working shell" onto one shared session each,
cutting spawns in this file from 17 down to 8:
write, resize, utf8 output x3, set_auto_command, close,
close-terminates-process).
stage resolving its filter state before the next stage runs.
Tests that need genuinely separate or concurrent processes (multi-session,
isolation, creation-time params) are left untouched.