Skip to content

[automated] test: Replace the stapler retry JavaScript harness with C# - #21811

Open
Ankit Jain (radical) wants to merge 3 commits into
microsoft:mainfrom
radical:csharp-infrastructure-harnesses
Open

Ankit Jain (radical) wants to merge 3 commits into
microsoft:mainfrom
radical:csharp-infrastructure-harnesses

Conversation

@radical

@radical Ankit Jain (radical) commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Description

Replace the stapler retry JavaScript harness with C#, following this review feedback.
The C# test delegated fixture setup and assertions to Node just to exercise a
Bash script, adding an unnecessary language hop and runtime dependency.

Run the real script from C# using TemporaryWorkspace and TestProcessRunner.
Small Bash stubs replace xcrun and sleep, avoiding Apple services and real
backoff waits. Six cases check recovery, nonretryable exit 68, and retry
exhaustion for both staple and validate.

Write executable stubs through ExecutableFile.WriteAsync to avoid Linux
ETXTBSY ("Text file busy", exit 126) failures when parallel tests inherit
writable descriptors. The helper writes and sets permissions in a short-lived
child rather than the test host.

Infrastructure guidance now defaults to C# tests and harnesses, with other
languages appropriate when they better suit the behavior under test.

Coverage changes

Coverage intentionally focuses on exit status, attempt count, and backoff.
Additional error/stdout cases, changing-error sequences, argument/quoting,
output-text, and temporary-file cleanup assertions are not retained.

The former per-process timeout is replaced by the existing project timeout.

Validation

  • 14 selected tests passed on macOS ARM64, with zero failures or skips.
  • Includes the existing executable-writer regression: 1,000 launches while four concurrent tasks start other processes.
  • Linux infrastructure CI passed: 2,050 tests, including all six stapler cases and both executable-writer regressions.
Local command: 14 selected tests passed on macOS ARM64
MSBUILDTERMINALLOGGER=false dotnet test --project tests/Infrastructure.Tests/Infrastructure.Tests.csproj --no-launch-profile -- --filter-class '*.MacOSStaplerRetryTests' --filter-class '*.ProcessRunnerTests' --filter-class '*.ExecutableFileTests' --filter-not-trait 'quarantined=true' --filter-not-trait 'outerloop=true' --report-trx --results-directory artifacts/pr-feedback-validation

Fixes # (issue)

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

Move stapler retry fixture setup and assertions into the C# test instead
of launching a JavaScript harness. Reuse TemporaryWorkspace and
TestProcessRunner with small Bash stubs for xcrun and sleep.

Cover connection-loss recovery, immediate rejection of a nonretryable
exit-68 error, and exhaustion after four attempts for staple and validate.
Keep the stubbed tests eligible for the existing Linux infrastructure
test lane without requiring Node or Apple services.

Focus coverage on retry classification, attempt counts, exit status, and
backoff delays rather than retaining the broader argument, output, and
cleanup checks from the JavaScript harness.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 21811

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 21811"

@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 5, 2026
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot November 5, 2026 18:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The executable test stubs can trigger a Linux ETXTBSY race in parallel tests, so they should be written using the existing ExecutableFile.WriteAsync helper.

1 open issue
What changed in this PR

Replaces the stapler retry test’s JavaScript harness with C# fixtures and assertions, removing its Node dependency while continuing to exercise the real shell script.

Changes:

  • Reuses TemporaryWorkspace and TestProcessRunner with Bash stubs for xcrun and sleep.
  • Covers recovery, nonretryable failure, and retry exhaustion for both staple and validate.
  • Narrows coverage to exit status, attempt counts, and backoff delays; removes the broader harness checks and per-process timeout.
File Description
tests/​Infrastructure.Tests/​Pipelines/​MacOSStaplerRetryTests.cs Implements six retry scenarios directly in C# using shared test helpers.
tests/​Infrastructure.Tests/​Pipelines/​macos-stapler-retry.harness.js Removes the JavaScript fixture and assertion harness.

Review effort: Balanced

Default fixture setup, process orchestration, and assertions to C# while
retaining language-native harnesses when they better suit the behavior
under test. Encourage reuse of TestProcessRunner and other existing
utilities without introducing abstractions for a single consumer.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Writing executable stubs from the test host exposes parallel Linux runs to an inherited-descriptor race that can cause ETXTBSY failures (exit 126) instead of exercising the retry scenario.

1 open issue

Review effort: Balanced

Writing executable stubs from a parallel test host can leave writable
descriptors inherited by another child process. Linux can then reject
execution with ETXTBSY (exit 126) instead of exercising retry behavior.

Use the existing ExecutableFile writer, which creates the stub and sets
its executable permissions from a short-lived child process.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot November 5, 2026 20:05
@radical Ankit Jain (radical) added area-engineering-systems infrastructure helix infra engineering repo stuff testing ☑️ and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The C# stapler retry harness migration has no active review issues.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

This branch has not been deployed

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

Labels

area-engineering-systems infrastructure helix infra engineering repo stuff testing ☑️

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants