Skip to content

SLVS-2936 Increase VSTest timeouts - #6701

Closed
georgii-borovinskikh-sonarsource wants to merge 8 commits into
masterfrom
gb/vstest-timeout
Closed

SLVS-2936 Increase VSTest timeouts#6701
georgii-borovinskikh-sonarsource wants to merge 8 commits into
masterfrom
gb/vstest-timeout

Conversation

@georgii-borovinskikh-sonarsource

Copy link
Copy Markdown
Member

Part of

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title Increase VSTest timeouts SLVS-2936 Increase VSTest timeouts May 4, 2026
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented May 4, 2026

Copy link
Copy Markdown

SLVS-2936

@sonar-review-alpha

sonar-review-alpha Bot commented May 4, 2026

Copy link
Copy Markdown

Summary

Increases VSTest timeouts to reduce flakiness in CI runs, and modernizes the test execution approach.

Key changes:

  • VSTest timeout environment variables doubled: VSTEST_TESTHOST_SHUTDOWN_TIMEOUT increased from 30s to 60s
  • Added VSTEST_CONNECTION_TIMEOUT: 60000 to handle connection-level timeouts
  • Migrated test command from vstest.console.exe with explicit DLL patterns to dotnet test with solution file
  • Added --blame-crash flag to capture diagnostic data on test failures
  • Added artifact upload step to persist test results for failed runs
  • Changed async execution pattern in test helper to use ConfigureAwait(false).GetAwaiter().GetResult()
  • Disabled a test with "todo" comment (RoslynAnalysisHttpServerTest.Dispose_StopsServer)

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues
0 New dependency risks

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

What reviewers should know

For reviewers:

  • The primary motivation is timeout reliability—tests were likely timing out at 30s. Verify this was confirmed in recent CI failures before merge.
  • The switch from vstest.console.exe to dotnet test is a breaking change in how tests run. Check that all test discovery and execution patterns remain equivalent (e.g., filters, settings, coverage collection).
  • The ConfigureAwait(false).GetAwaiter().GetResult() change in NoOpThreadHandler prevents potential deadlocks in synchronous test code wrapping async methods. This is a subtle but important fix.
  • The ignored Dispose_StopsServer test has a "todo" comment—ensure there's a tracking issue for re-enabling it and it's not a blocker for this PR.
  • The artifact upload for test results is new—verify the path TestResults/** matches what dotnet test --results-directory produces.

  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

sonar-review-alpha[bot]

This comment was marked as outdated.

@sonar-review-alpha sonar-review-alpha Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One real issue with the artifact upload step that undermines the diagnostic value of this change.

🗣️ Give feedback

Comment thread .github/workflows/build.yml Outdated

- name: Upload test results artifact
uses: actions/upload-artifact@v4
if: success() || failure()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

success() || failure() excludes cancelled(). If the GitHub Actions job hits its runner-level timeout and is cancelled — which is the most common outcome when a test hangs beyond vstest's own VSTEST_TESTHOST_SHUTDOWN_TIMEOUT — the blame artifacts are never uploaded. That's precisely the scenario this step exists to diagnose.

Change to always() so the artifacts are captured regardless of how the job ends.

Suggested change
if: success() || failure()
if: always()
  • Mark as noise

sonar-review-alpha[bot]

This comment was marked as outdated.

@sonar-review-alpha sonar-review-alpha Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The prior issue from the previous review cycle is still open — see below.

🗣️ Give feedback

serverStarter2.MockedLogger.Received(1).LogVerbose(Arg.Any<MessageLevelContext>(), Resources.HttpRequestFailed, Arg.Is<string>(x => x.Contains(exceptionMessage)));
}

[Ignore] // todo

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Ignore] with // todo leaves no trail. This test verifies a meaningful contract — that Dispose() actually stops the HTTP listener. Without a linked tracking issue or a description of why it's failing, this has a high chance of sitting ignored indefinitely.

MSTest's [Ignore] accepts a message: [Ignore("SLVS-XXXX: brief description of failure")]. Please link the ticket.

  • Mark as noise

@sonar-review-alpha sonar-review-alpha Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both previously flagged issues remain unresolved. The artifact upload step still uses if: success() || failure() (no fix for the cancellation gap), and [Ignore] // todo on Dispose_StopsServer still has no tracking reference.

The new commit moves the artifact upload step earlier in the job (before "Report Test Results") and replaces --blame with --blame-crash. Neither change addresses either open issue.

🗣️ Give feedback

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant