Skip to content

Conversation

@addaleax
Copy link
Collaborator

In the past, we've had a number of flaky tests that followed a pattern similar to this:

  1. Start a test shell
  2. Run a command using executeLine()
  3. Make assertions about the command's output

The internal logic of executeLine() determines that it has seen the end of the command's output by looking for the subsequent prompt (similar to how a human being in front of the shell would determine that the command has finished).

However, in the above scheme, there is a race condition that can develop as follows:

  1. The test shell process starts and prints part of its initial output
  2. The executeLine() command remembers the current output position and sends the input to the process's stdin stream
  3. The test shell prints its first prompt
  4. The test reads the output from the shell and sees the prompt, assumes that the comamnd has finished
  5. The test shell finishes evaluating the input and writes the corresponding output + a new prompt
  6. The test ignores the remaining output and instead only treats the text up to the first prompt as the command output
  7. An assertion made about the output fails, as the output is not coming from the command it was supposed to come from

In order to avoid the likelihood of introducing such race conditions, this commit adds a guardrail to the input writing methods that ensures that the shell has reached some form of known state that has been awaited, such as seeing the first prompt or other specific output, before it allows writing to the process's input.

In the past, we've had a number of flaky tests that followed a pattern
similar to this:

1. Start a test shell
2. Run a command using `executeLine()`
3. Make assertions about the command's output

The internal logic of `executeLine()` determines that it has seen
the end of the command's output by looking for the subsequent prompt
(similar to how a human being in front of the shell would determine
that the command has finished).

However, in the above scheme, there is a race condition that can
develop as follows:

1. The test shell process starts and prints part of its initial output
2. The `executeLine()` command remembers the current output position
   and sends the input to the process's stdin stream
3. The test shell prints its first prompt
4. The test reads the output from the shell and sees the prompt,
   assumes that the comamnd has finished
5. The test shell finishes evaluating the input and writes the
   corresponding output + a new prompt
6. The test ignores the remaining output and instead only treats
   the text up to the first prompt as the command output
7. An assertion made about the output fails, as the output is not
   coming from the command it was supposed to come from

In order to avoid the likelihood of introducing such race conditions,
this commit adds a guardrail to the input writing methods that
ensures that the shell has reached some form of known state that has
been awaited, such as seeing the first prompt or other specific output,
before it allows writing to the process's input.
@addaleax addaleax requested a review from a team as a code owner November 28, 2025 16:50
Copilot AI review requested due to automatic review settings November 28, 2025 16:50
@addaleax addaleax added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Nov 28, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses race conditions in e2e tests by ensuring the test shell completes initialization before accepting input commands. The core issue was that tests could write input before the shell finished starting up, causing the test framework to misinterpret initial prompts as command completion signals.

Key changes:

  • Added a guardrail mechanism (_initializationKnownToBeFinished) that prevents writing input until the shell reaches a known stable state
  • Introduced requireFinishedInitialization option (defaults to true) for input methods to control this behavior
  • Refactored two test cases to explicitly bypass initialization checks where tests intentionally write input before waiting for the shell to start

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/e2e-tests/test/test-shell.ts Implements initialization tracking flag and adds requireFinishedInitialization parameter to input methods; introduces type definitions for options objects
packages/e2e-tests/test/e2e.spec.ts Updates two tests to explicitly disable initialization requirement and replaces manual exit/assertion patterns with waitForCleanOutput()
packages/e2e-tests/test/e2e-direct.spec.ts Adds explicit waitForPrompt() call before executing commands and waits for successful exit to ensure proper synchronization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}: TestShellInputOptions = {}
): void {
if (requireFinishedInitialization && !this._initializationKnownToBeFinished)
throw new Error('Wait for shell to be initialized before writing input');
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The error message should be more actionable. Consider: 'Cannot write input before shell initialization completes. Call waitForPrompt(), waitForLine(), or waitForAnyExit() first, or set requireFinishedInitialization to false.'

Suggested change
throw new Error('Wait for shell to be initialized before writing input');
throw new Error(
"Cannot write input before shell initialization completes. Call waitForPrompt(), waitForLine(), or waitForAnyExit() first, or set requireFinishedInitialization to false."
);

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any strong feelings? I imagine anybody getting this error would look up the logic where it's being thrown (i.e. the code), so I'd personally am okay with leaning towards brevity, but I also don't care too deeply

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah don't mind either way either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants