Skip to content

.NET: Fix .NET Copilot integration tests for SDK v1.0.0#6424

Merged
giles17 merged 3 commits into
mainfrom
dotnet-ghcp-int-tests
Jun 10, 2026
Merged

.NET: Fix .NET Copilot integration tests for SDK v1.0.0#6424
giles17 merged 3 commits into
mainfrom
dotnet-ghcp-int-tests

Conversation

@giles17

@giles17 giles17 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the .NET GitHub Copilot integration tests to work reliably with SDK v1.0.0 and prepares them for CI.

Changes

  • Runtime skip: Replace hard [Fact(Skip = ...)] with Assert.Skip when COPILOT_GITHUB_TOKEN is not set (matches Python pattern)
  • CI-ready traits: Add [Trait("Category", "Integration")] for filtering in CI workflows
  • Fix FunctionTool test: Use explicit SessionConfig with Tools, OnPermissionRequest, and SystemMessage (custom tools require permission approval)
  • Session cleanup: Create explicit sessions in all tests and call DeleteSessionAsync after each (matches Python pattern)
  • Disable RemoteMcp: Mark with [Trait("Category", "IntegrationDisabled")] (requires OAuth flow not available in CI)
  • Code cleanup: Remove unused System.Diagnostics import, simplify skip logic

Testing

All 8 enabled integration tests pass locally with COPILOT_GITHUB_TOKEN set:

dotnet test tests\Microsoft.Agents.AI.GitHub.Copilot.IntegrationTests -c Release -f net9.0 --filter-not-trait "Category=IntegrationDisabled"

Related

- Remove hard-skip in favor of runtime Assert.Skip when COPILOT_GITHUB_TOKEN is not set
- Add [Trait("Category", "Integration")] for CI filtering
- Fix FunctionTool test: use explicit SessionConfig with Tools, OnPermissionRequest, and SystemMessage
- Mark RemoteMcp test as IntegrationDisabled (requires OAuth flow)
- Create explicit sessions in all tests and delete after each (cleanup)
- Remove unused System.Diagnostics import
- Simplify SkipIfCopilotNotConfigured to only check env var

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 9, 2026 19:35
@moonbox3 moonbox3 added the .NET label Jun 9, 2026
@github-actions github-actions Bot changed the title Fix .NET Copilot integration tests for SDK v1.0.0 .NET: Fix .NET Copilot integration tests for SDK v1.0.0 Jun 9, 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.

Pull request overview

Updates the .NET GitHub Copilot integration tests to align with the Copilot SDK v1.0.0 behavior and make the suite CI-filterable and more reliable by moving from compile-time skips to runtime skips, adding category traits, and creating/cleaning up explicit Copilot sessions per test.

Changes:

  • Replace hard [Fact(Skip = ...)] with runtime Assert.Skip(...) when COPILOT_GITHUB_TOKEN is missing.
  • Add [Trait("Category", "Integration")] (and "IntegrationDisabled" for Remote MCP) to support CI filtering.
  • Update tests to explicitly create sessions and pass them into RunAsync/RunStreamingAsync, plus add a helper to delete sessions.

@github-actions github-actions Bot 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.

Automated Code Review

Reviewers: 5 | Confidence: 91%

✓ Correctness

The PR correctly updates integration tests to use runtime skip via Assert.Skip (checking COPILOT_GITHUB_TOKEN env var), adds explicit session creation/cleanup, and properly fixes the FunctionTool test to use SessionConfig with Tools, OnPermissionRequest, and SystemMessage. All API usage is verified correct against the GitHubCopilotAgent source: constructor overloads, CreateSessionAsync (both parameterless and with sessionId), RunAsync/RunStreamingAsync with session parameter, and the DeleteSessionAsync helper's pattern match on GitHubCopilotAgentSession.SessionId. The cleanup helper correctly handles the case where SessionId might be null (no-op) if a run never completed server-side session creation.

✓ Security Reliability

This PR makes straightforward improvements to integration tests: replacing compile-time skip with runtime Assert.Skip, adding proper session lifecycle management, and CI-ready traits. No security or reliability issues found. The session cleanup code runs after assertions (not in try/finally), meaning server-side sessions could leak on test failure, but this is a minor concern for test code with presumably TTL-expiring server sessions, and the critical resources (CopilotClient, GitHubCopilotAgent) are properly disposed via await using.

✓ Test Coverage

The integration test changes are well-structured: meaningful assertions (checking specific content, tool invocation flags), proper skip logic via Assert.Skip, explicit session lifecycle management, and CI-ready traits. The only minor concern is that session cleanup is not wrapped in try/finally, meaning if an assertion fails mid-test the remote session won't be deleted — though this is a common pattern in integration tests and the await-using pattern handles local resource cleanup.

✓ Failure Modes

The PR changes are straightforward integration test fixes. The session cleanup pattern (calling DeleteSessionAsync after assertions rather than in try/finally) means server-side sessions leak on test failure, but this is explicitly stated as intentional (matching the Python pattern) and sessions presumably have server-side TTLs. The DeleteSessionAsync helper correctly handles the null-SessionId case (no server session was created). No silent failures, swallowed exceptions, or data-loss scenarios are introduced.

✗ Design Approach

The new explicit-session approach matches the agent API, but the cleanup design is still incomplete: every new DeleteSessionAsync(...) call is on the success path only, so any exception or assertion failure before those lines leaves the Copilot session undeleted. That undercuts the PR’s stated goal of making these integration tests more reliable for CI.

Flagged Issues

  • Session cleanup is not protected by finally, so failures still leak Copilot sessions (for example dotnet/tests/Microsoft.Agents.AI.GitHub.Copilot.IntegrationTests/GitHubCopilotAgentTests.cs:37-48). The repo already uses a safer pattern for this in dotnet/tests/Foundry.IntegrationTests/ResponsesAgentExtensionCreateTests.cs:39-59.

Automated review by giles17's agents

- Wrap act/assert in try/finally so sessions are always deleted even on failure
- Use IsNullOrWhiteSpace instead of IsNullOrEmpty for token check

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Copilot SDK runtime reads this env var directly for authentication.
No Node.js/npm install needed - the SDK downloads the CLI binary at build time.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@giles17 giles17 added this pull request to the merge queue Jun 10, 2026
Merged via the queue into main with commit a5f4e00 Jun 10, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants