Skip to content

test(pty): stabilize flaky PTY interactions #361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benvinegar merged 3 commits into from
May 24, 2026
Merged

test(pty): stabilize flaky PTY interactions #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22f8723
test(pty): stabilize pager wheel scrolling
benvinegar May 24, 2026
cbe828d
test(pty): widen pager wheel retry budget
benvinegar May 24, 2026
4e6bac3
test(pty): stabilize note focus cancellation
benvinegar May 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion test/pty/notes.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,9 @@ describe("PTY notes", () => {
);
expect(whileFocused).toContain("Draft note");

await session.type("\x1b");
// Keyboard cancellation is covered above; click the explicit control here so this test can
// focus on whether app-level shortcuts are blocked only while the composer is active.
await session.click(/Cancel \(Esc\)/);
await harness.waitForSnapshot(session, (text) => !text.includes("Draft note"), 5_000);
await session.press("]");
const afterCancel = await harness.waitForSnapshot(
Expand Down
65 changes: 42 additions & 23 deletions test/pty/pager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,41 @@
import { afterEach, describe, expect, setDefaultTimeout, test } from "bun:test";
import type { Session } from "tuistory";
import { createPtyHarness } from "./harness";

const harness = createPtyHarness();

/** Give PTY-backed startup and redraws enough headroom for slower CI machines. */
setDefaultTimeout(20_000);
/** Give PTY-backed startup, redraws, and wheel retries enough headroom for slower CI machines. */
setDefaultTimeout(45_000);

afterEach(() => {
harness.cleanup();
});

/** Retry PTY wheel ticks one at a time so slow CI does not drop a whole scroll burst. */
async function scrollWheelUntil(
session: Session,
direction: "down" | "up",
predicate: (text: string) => boolean,
) {
let lastErrorMessage = `Timed out waiting for pager wheel scroll ${direction}.`;

for (let attempt = 0; attempt < 12; attempt += 1) {
if (direction === "down") {
await session.scrollDown(1);
} else {
await session.scrollUp(1);
}

try {
return await harness.waitForSnapshot(session, predicate, 700);
} catch (error) {
lastErrorMessage = error instanceof Error ? error.message : String(error);
}
}
Comment on lines +22 to +34
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Total timeout budget may still be tight under extreme CI load

Tests that call scrollWheelUntil twice (down + up) can accumulate up to 12 × 700 ms per direction = ~8.4 s each, for a combined ~16.8 s on top of the 15 s waitForText startup timeout and the 200 ms waitIdle. That's a ceiling of ~32 s against the file-wide setDefaultTimeout(20_000). In practice the predicate matches on the first or second attempt, so the tests pass; but if the PTY is slow enough that either direction needs 4–5 retries (≥ 2.8 s) alongside a slow startup, the test can still time out via the outer limit rather than the inner 700 ms window.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: test/pty/pager.test.ts
Line: 22-34

Comment:
**Total timeout budget may still be tight under extreme CI load**

Tests that call `scrollWheelUntil` twice (down + up) can accumulate up to 12 × 700 ms per direction = ~8.4 s each, for a combined ~16.8 s on top of the 15 s `waitForText` startup timeout and the 200 ms `waitIdle`. That's a ceiling of ~32 s against the file-wide `setDefaultTimeout(20_000)`. In practice the predicate matches on the first or second attempt, so the tests pass; but if the PTY is slow enough that either direction needs 4–5 retries (≥ 2.8 s) alongside a slow startup, the test can still time out via the outer limit rather than the inner 700 ms window.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Member Author

@benvinegar benvinegar May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I raised the pager PTY file timeout to 45s so the worst-case wheel retry budget has headroom instead of racing Bun's outer test timeout.

This comment was generated by Pi using GPT-5


throw new Error(lastErrorMessage);
}

describe("PTY pager", () => {
test("pager mode hides chrome and pages forward on space", async () => {
const fixture = harness.createPagerPatchFixture();
Expand Down Expand Up @@ -155,22 +181,20 @@ describe("PTY pager", () => {
expect(initial).not.toContain("before_12");

await session.waitIdle({ timeout: 200 });
await session.scrollDown(10);
const scrolled = await harness.waitForSnapshot(
const scrolled = await scrollWheelUntil(
session,
"down",
(text) => !text.includes("before_01") && text.includes("before_12"),
5_000,
);

expect(scrolled).not.toContain("View Navigate Theme Agent Help");
expect(scrolled).not.toContain("before_01");
expect(scrolled).toContain("before_12");

await session.scrollUp(10);
const restored = await harness.waitForSnapshot(
const restored = await scrollWheelUntil(
session,
"up",
(text) => text.includes("before_01") && !text.includes("before_12"),
5_000,
);

expect(restored).toContain("before_01");
Expand All @@ -196,11 +220,10 @@ describe("PTY pager", () => {
expect(initial).not.toContain("before_12");

await session.waitIdle({ timeout: 200 });
await session.scrollDown(10);
const scrolled = await harness.waitForSnapshot(
const scrolled = await scrollWheelUntil(
session,
"down",
(text) => !text.includes("before_01") && text.includes("before_12"),
5_000,
);

expect(scrolled).toContain("before_12");
Expand All @@ -226,22 +249,20 @@ describe("PTY pager", () => {
expect(initial).not.toContain("before_12");

await session.waitIdle({ timeout: 200 });
await session.scrollDown(10);
const scrolled = await harness.waitForSnapshot(
const scrolled = await scrollWheelUntil(
session,
"down",
(text) => !text.includes("before_01") && text.includes("before_12"),
5_000,
);

expect(scrolled).not.toContain("View Navigate Theme Agent Help");
expect(scrolled).not.toContain("before_01");
expect(scrolled).toContain("before_12");

await session.scrollUp(10);
const restored = await harness.waitForSnapshot(
const restored = await scrollWheelUntil(
session,
"up",
(text) => text.includes("before_01") && !text.includes("before_12"),
5_000,
);

expect(restored).toContain("before_01");
Expand Down Expand Up @@ -297,22 +318,20 @@ describe("PTY pager", () => {
expect(initial).not.toContain("before_12");

await session.waitIdle({ timeout: 200 });
await session.scrollDown(10);
const scrolled = await harness.waitForSnapshot(
const scrolled = await scrollWheelUntil(
session,
"down",
(text) => !text.includes("before_01") && text.includes("before_12"),
5_000,
);

expect(scrolled).not.toContain("View Navigate Theme Agent Help");
expect(scrolled).not.toContain("before_01");
expect(scrolled).toContain("before_12");

await session.scrollUp(10);
const restored = await harness.waitForSnapshot(
const restored = await scrollWheelUntil(
session,
"up",
(text) => text.includes("before_01") && !text.includes("before_12"),
5_000,
);

expect(restored).toContain("before_01");
Expand Down
Loading