Skip to content

Commit 2c9e388

Browse files
CopilotMalcolmnixon
Copilot
and
Malcolmnixon
authored
Improve GitHubRepoConnector testability via virtual factory method (#90)
* Initial plan * Add testability improvements to GitHubRepoConnector Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Add mocking patterns documentation to CONTRIBUTING.md Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Fix duplicate GraphQL client creation - pass existing client to CollectChangesFromPullRequestsAsync Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Rename to MockGitHubGraphQLHttpMessageHandler and add helper methods Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Update CONTRIBUTING.md with renamed class and helper method examples Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Reorder parameters and convert helper methods to use collections Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Restructure tests - create MockableGitHubRepoConnector helper and separate test files Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Implement proper data structures for PR and Issue mock responses Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Add comprehensive tests for version selection, changes, bugs, and known issues Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> * Move mock records to top of file and add MockRelease and MockTag records Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
1 parent 97e9e1d commit 2c9e388

File tree

6 files changed

+911
-9
lines changed

6 files changed

+911
-9
lines changed

‎CONTRIBUTING.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,34 @@ Examples:
147147
- `Assert.DoesNotContain(item, collection)`
148148
- Always clean up resources (use `try/finally` for console redirection)
149149

150+
### Mocking and Testing Patterns
151+
152+
When testing classes that depend on external services (like GitHub GraphQL API):
153+
154+
1. **Use virtual methods for dependency creation**: Classes expose internal virtual methods
155+
(e.g., `CreateGraphQLClient`) that can be overridden in derived test classes
156+
2. **Mock HTTP responses**: Use `MockGitHubGraphQLHttpMessageHandler` to simulate GitHub API responses
157+
3. **Helper methods**: Use built-in helper methods for common responses
158+
159+
Example:
160+
161+
```csharp
162+
// Create a mock HTTP handler using helper methods
163+
using var mockHandler = new MockGitHubGraphQLHttpMessageHandler()
164+
.AddCommitsResponse("commit123")
165+
.AddReleasesResponse("v1.0.0")
166+
.AddEmptyPullRequestsResponse();
167+
168+
// Create HttpClient and inject into GitHubGraphQLClient
169+
using var mockHttpClient = new HttpClient(mockHandler);
170+
using var client = new GitHubGraphQLClient(mockHttpClient);
171+
172+
// Now the client will use mock responses instead of real API calls
173+
var commits = await client.GetCommitsAsync("owner", "repo", "branch");
174+
```
175+
176+
See `GitHubRepoConnectorTestabilityTests.cs` for complete examples.
177+
150178
### Running Tests
151179

152180
```bash

‎src/DemaConsulting.BuildMark/RepoConnectors/GitHubRepoConnector.cs‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,20 @@ public class GitHubRepoConnector : RepoConnectorBase
4141
{ "security", "security" }
4242
};
4343

44+
/// <summary>
45+
/// Creates a GitHub GraphQL client for API operations.
46+
/// </summary>
47+
/// <param name="token">GitHub personal access token for authentication.</param>
48+
/// <returns>A new GitHubGraphQLClient instance.</returns>
49+
/// <remarks>
50+
/// This method is virtual to allow derived classes to override it for testing purposes.
51+
/// Tests can provide a client configured with a mock HttpClient for controlled responses.
52+
/// </remarks>
53+
internal virtual GitHubGraphQLClient CreateGraphQLClient(string token)
54+
{
55+
return new GitHubGraphQLClient(token);
56+
}
57+
4458
/// <summary>
4559
/// Gets build information for a release.
4660
/// </summary>
@@ -61,7 +75,7 @@ public override async Task<BuildInformation> GetBuildInformationAsync(Version? v
6175
var token = await GetGitHubTokenAsync();
6276

6377
// Create GraphQL client
64-
using var graphqlClient = new GitHubGraphQLClient(token);
78+
using var graphqlClient = CreateGraphQLClient(token);
6579

6680
// Fetch all data from GitHub
6781
var gitHubData = await FetchGitHubDataAsync(graphqlClient, owner, repo, branch.Trim());
@@ -80,11 +94,11 @@ public override async Task<BuildInformation> GetBuildInformationAsync(Version? v
8094

8195
// Collect changes from PRs
8296
var (bugs, nonBugChanges, allChangeIds) = await CollectChangesFromPullRequestsAsync(
97+
graphqlClient,
8398
commitsInRange,
8499
lookupData,
85100
owner,
86-
repo,
87-
token);
101+
repo);
88102

89103
// Collect known issues
90104
var knownIssues = CollectKnownIssues(gitHubData.Issues, allChangeIds);
@@ -446,28 +460,25 @@ private static int DetermineSearchStartIndex(int toIndex, int releaseCount)
446460
/// <summary>
447461
/// Collects changes from pull requests in the commit range.
448462
/// </summary>
463+
/// <param name="graphqlClient">GitHub GraphQL client for API operations.</param>
449464
/// <param name="commitsInRange">Commits in range.</param>
450465
/// <param name="lookupData">Lookup data structures.</param>
451466
/// <param name="owner">Repository owner.</param>
452467
/// <param name="repo">Repository name.</param>
453-
/// <param name="token">GitHub token.</param>
454468
/// <returns>Tuple of (bugs, nonBugChanges, allChangeIds).</returns>
455469
private static async Task<(List<ItemInfo> bugs, List<ItemInfo> nonBugChanges, HashSet<string> allChangeIds)>
456470
CollectChangesFromPullRequestsAsync(
471+
GitHubGraphQLClient graphqlClient,
457472
List<Commit> commitsInRange,
458473
LookupData lookupData,
459474
string owner,
460-
string repo,
461-
string token)
475+
string repo)
462476
{
463477
// Initialize collections for tracking changes
464478
var allChangeIds = new HashSet<string>();
465479
var bugs = new List<ItemInfo>();
466480
var nonBugChanges = new List<ItemInfo>();
467481

468-
// Create GraphQL client for finding linked issues (reused across multiple PR queries)
469-
using var graphqlClient = new GitHubGraphQLClient(token);
470-
471482
// Process each commit that has an associated PR
472483
foreach (var pr in commitsInRange
473484
.Where(c => lookupData.CommitHashToPr.ContainsKey(c.Sha))

‎test/DemaConsulting.BuildMark.Tests/GitHubRepoConnectorTests.cs‎

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,4 +451,225 @@ public void GitHubRepoConnector_DetermineBaselineVersion_NoReleases_ReturnsNull(
451451
Assert.IsNull(fromVersion);
452452
Assert.IsNull(fromHash);
453453
}
454+
455+
/// <summary>
456+
/// Test that GetBuildInformationAsync works with mocked data.
457+
/// </summary>
458+
[TestMethod]
459+
public async Task GitHubRepoConnector_GetBuildInformationAsync_WithMockedData_ReturnsValidBuildInformation()
460+
{
461+
// Arrange - Create mock responses using helper methods
462+
using var mockHandler = new MockGitHubGraphQLHttpMessageHandler()
463+
.AddCommitsResponse(new[] { "abc123def456" })
464+
.AddReleasesResponse(new[] { new MockRelease("v1.0.0", "2024-01-01T00:00:00Z") })
465+
.AddPullRequestsResponse(Array.Empty<MockPullRequest>())
466+
.AddIssuesResponse(Array.Empty<MockIssue>())
467+
.AddTagsResponse(new[] { new MockTag("v1.0.0", "abc123def456") });
468+
469+
using var mockHttpClient = new HttpClient(mockHandler);
470+
var connector = new MockableGitHubRepoConnector(mockHttpClient);
471+
472+
// Set up mock command responses
473+
connector.SetCommandResponse("git remote get-url origin", "https://github.com/test/repo.git");
474+
connector.SetCommandResponse("git rev-parse --abbrev-ref HEAD", "main");
475+
connector.SetCommandResponse("git rev-parse HEAD", "abc123def456");
476+
connector.SetCommandResponse("gh auth token", "test-token");
477+
478+
// Act
479+
var buildInfo = await connector.GetBuildInformationAsync(Version.Create("v1.0.0"));
480+
481+
// Assert
482+
Assert.IsNotNull(buildInfo);
483+
Assert.AreEqual("1.0.0", buildInfo.CurrentVersionTag.VersionInfo.FullVersion);
484+
Assert.AreEqual("abc123def456", buildInfo.CurrentVersionTag.CommitHash);
485+
Assert.IsNotNull(buildInfo.Changes);
486+
Assert.IsNotNull(buildInfo.Bugs);
487+
Assert.IsNotNull(buildInfo.KnownIssues);
488+
}
489+
490+
/// <summary>
491+
/// Test that GetBuildInformationAsync correctly selects previous version and generates changelog link.
492+
/// </summary>
493+
[TestMethod]
494+
public async Task GitHubRepoConnector_GetBuildInformationAsync_WithMultipleVersions_SelectsCorrectPreviousVersionAndGeneratesChangelogLink()
495+
{
496+
// Arrange - Create mock responses with multiple versions
497+
using var mockHandler = new MockGitHubGraphQLHttpMessageHandler()
498+
.AddCommitsResponse(new[] { "commit3", "commit2", "commit1" })
499+
.AddReleasesResponse(new[]
500+
{
501+
new MockRelease("v2.0.0", "2024-03-01T00:00:00Z"),
502+
new MockRelease("v1.1.0", "2024-02-01T00:00:00Z"),
503+
new MockRelease("v1.0.0", "2024-01-01T00:00:00Z")
504+
})
505+
.AddPullRequestsResponse(Array.Empty<MockPullRequest>())
506+
.AddIssuesResponse(Array.Empty<MockIssue>())
507+
.AddTagsResponse(new[]
508+
{
509+
new MockTag("v2.0.0", "commit3"),
510+
new MockTag("v1.1.0", "commit2"),
511+
new MockTag("v1.0.0", "commit1")
512+
});
513+
514+
using var mockHttpClient = new HttpClient(mockHandler);
515+
var connector = new MockableGitHubRepoConnector(mockHttpClient);
516+
517+
// Set up mock command responses
518+
connector.SetCommandResponse("git remote get-url origin", "https://github.com/test/repo.git");
519+
connector.SetCommandResponse("git rev-parse --abbrev-ref HEAD", "main");
520+
connector.SetCommandResponse("git rev-parse HEAD", "commit3");
521+
connector.SetCommandResponse("gh auth token", "test-token");
522+
523+
// Act
524+
var buildInfo = await connector.GetBuildInformationAsync(Version.Create("v2.0.0"));
525+
526+
// Assert
527+
Assert.IsNotNull(buildInfo);
528+
Assert.AreEqual("2.0.0", buildInfo.CurrentVersionTag.VersionInfo.FullVersion);
529+
Assert.AreEqual("commit3", buildInfo.CurrentVersionTag.CommitHash);
530+
531+
// Should have selected v1.1.0 as baseline (previous non-prerelease)
532+
Assert.IsNotNull(buildInfo.BaselineVersionTag);
533+
Assert.AreEqual("1.1.0", buildInfo.BaselineVersionTag.VersionInfo.FullVersion);
534+
Assert.AreEqual("commit2", buildInfo.BaselineVersionTag.CommitHash);
535+
536+
// Should have changelog link
537+
Assert.IsNotNull(buildInfo.CompleteChangelogLink);
538+
Assert.IsTrue(buildInfo.CompleteChangelogLink.TargetUrl.Contains("v1.1.0...v2.0.0"));
539+
}
540+
541+
/// <summary>
542+
/// Test that GetBuildInformationAsync correctly gathers changes from PRs with labels.
543+
/// </summary>
544+
[TestMethod]
545+
public async Task GitHubRepoConnector_GetBuildInformationAsync_WithPullRequests_GathersChangesCorrectly()
546+
{
547+
// Arrange - Create mock responses with PRs containing different label types
548+
// We need commits in range between v1.0.0 and v1.1.0
549+
using var mockHandler = new MockGitHubGraphQLHttpMessageHandler()
550+
.AddCommitsResponse(new[] { "commit3", "commit2", "commit1" })
551+
.AddReleasesResponse(new[]
552+
{
553+
new MockRelease("v1.1.0", "2024-02-01T00:00:00Z"),
554+
new MockRelease("v1.0.0", "2024-01-01T00:00:00Z")
555+
})
556+
.AddPullRequestsResponse(new[]
557+
{
558+
new MockPullRequest(
559+
Number: 101,
560+
Title: "Add new feature",
561+
Url: "https://github.com/test/repo/pull/101",
562+
Merged: true,
563+
MergeCommitSha: "commit3",
564+
HeadRefOid: "feature-branch",
565+
Labels: new List<string> { "feature", "enhancement" }),
566+
new MockPullRequest(
567+
Number: 100,
568+
Title: "Fix critical bug",
569+
Url: "https://github.com/test/repo/pull/100",
570+
Merged: true,
571+
MergeCommitSha: "commit2",
572+
HeadRefOid: "bugfix-branch",
573+
Labels: new List<string> { "bug" })
574+
})
575+
.AddIssuesResponse(Array.Empty<MockIssue>())
576+
.AddTagsResponse(new[]
577+
{
578+
new MockTag("v1.1.0", "commit3"),
579+
new MockTag("v1.0.0", "commit1")
580+
})
581+
// Mock the linked issues query to return empty (PRs are treated as standalone changes)
582+
.AddResponse("closingIssuesReferences", @"{""data"":{""repository"":{""pullRequest"":{""closingIssuesReferences"":{""nodes"":[],""pageInfo"":{""hasNextPage"":false,""endCursor"":null}}}}}}");
583+
584+
using var mockHttpClient = new HttpClient(mockHandler);
585+
var connector = new MockableGitHubRepoConnector(mockHttpClient);
586+
587+
// Set up mock command responses
588+
connector.SetCommandResponse("git remote get-url origin", "https://github.com/test/repo.git");
589+
connector.SetCommandResponse("git rev-parse --abbrev-ref HEAD", "main");
590+
connector.SetCommandResponse("git rev-parse HEAD", "commit3");
591+
connector.SetCommandResponse("gh auth token", "test-token");
592+
593+
// Act
594+
var buildInfo = await connector.GetBuildInformationAsync(Version.Create("v1.1.0"));
595+
596+
// Assert
597+
Assert.IsNotNull(buildInfo);
598+
Assert.AreEqual("1.1.0", buildInfo.CurrentVersionTag.VersionInfo.FullVersion);
599+
600+
// PRs without linked issues are treated based on their labels
601+
// PR 100 with "bug" label should be in bugs
602+
Assert.IsNotNull(buildInfo.Bugs);
603+
Assert.IsTrue(buildInfo.Bugs.Count >= 1, $"Expected at least 1 bug, got {buildInfo.Bugs.Count}");
604+
var bugPR = buildInfo.Bugs.FirstOrDefault(b => b.Index == 100);
605+
Assert.IsNotNull(bugPR, "PR 100 should be categorized as a bug");
606+
Assert.AreEqual("Fix critical bug", bugPR.Title);
607+
608+
// PR 101 with "feature" label should be in changes
609+
Assert.IsNotNull(buildInfo.Changes);
610+
Assert.IsTrue(buildInfo.Changes.Count >= 1, $"Expected at least 1 change, got {buildInfo.Changes.Count}");
611+
var featurePR = buildInfo.Changes.FirstOrDefault(c => c.Index == 101);
612+
Assert.IsNotNull(featurePR, "PR 101 should be categorized as a change");
613+
Assert.AreEqual("Add new feature", featurePR.Title);
614+
}
615+
616+
/// <summary>
617+
/// Test that GetBuildInformationAsync correctly identifies known issues.
618+
/// </summary>
619+
[TestMethod]
620+
public async Task GitHubRepoConnector_GetBuildInformationAsync_WithOpenIssues_IdentifiesKnownIssues()
621+
{
622+
// Arrange - Create mock responses with open and closed issues
623+
using var mockHandler = new MockGitHubGraphQLHttpMessageHandler()
624+
.AddCommitsResponse(new[] { "commit1" })
625+
.AddReleasesResponse(new[] { new MockRelease("v1.0.0", "2024-01-01T00:00:00Z") })
626+
.AddPullRequestsResponse(Array.Empty<MockPullRequest>())
627+
.AddIssuesResponse(new[]
628+
{
629+
new MockIssue(
630+
Number: 201,
631+
Title: "Known bug in feature X",
632+
Url: "https://github.com/test/repo/issues/201",
633+
State: "OPEN",
634+
Labels: new List<string> { "bug" }),
635+
new MockIssue(
636+
Number: 202,
637+
Title: "Feature request for Y",
638+
Url: "https://github.com/test/repo/issues/202",
639+
State: "OPEN",
640+
Labels: new List<string> { "feature" }),
641+
new MockIssue(
642+
Number: 203,
643+
Title: "Fixed bug",
644+
Url: "https://github.com/test/repo/issues/203",
645+
State: "CLOSED",
646+
Labels: new List<string> { "bug" })
647+
})
648+
.AddTagsResponse(new[] { new MockTag("v1.0.0", "commit1") });
649+
650+
using var mockHttpClient = new HttpClient(mockHandler);
651+
var connector = new MockableGitHubRepoConnector(mockHttpClient);
652+
653+
// Set up mock command responses
654+
connector.SetCommandResponse("git remote get-url origin", "https://github.com/test/repo.git");
655+
connector.SetCommandResponse("git rev-parse --abbrev-ref HEAD", "main");
656+
connector.SetCommandResponse("git rev-parse HEAD", "commit1");
657+
connector.SetCommandResponse("gh auth token", "test-token");
658+
659+
// Act
660+
var buildInfo = await connector.GetBuildInformationAsync(Version.Create("v1.0.0"));
661+
662+
// Assert
663+
Assert.IsNotNull(buildInfo);
664+
665+
// Known issues are open issues that aren't linked to any changes in this release
666+
Assert.IsNotNull(buildInfo.KnownIssues);
667+
// Since we have no PRs, all open issues should be known issues
668+
Assert.IsTrue(buildInfo.KnownIssues.Count >= 1, $"Expected at least 1 known issue, got {buildInfo.KnownIssues.Count}");
669+
670+
// Verify at least one known issue is present
671+
var knownIssueTitles = buildInfo.KnownIssues.Select(i => i.Title).ToList();
672+
Assert.IsTrue(knownIssueTitles.Any(t => t.Contains("Known bug") || t.Contains("Feature request")),
673+
"Should have at least one of the open issues as a known issue");
674+
}
454675
}

0 commit comments

Comments
 (0)