Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions BuildMonitor.Test/GitHubRunErrorsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,61 @@ public void ASecondUpdateOfAFailedRunDoesNotFetchItsJobsAgain()
Assert.IsFalse(fetchAgain);
}

/// <summary>
/// A re-run keeps the run id. When attempt 2 also failed and was never seen running, the stored run
/// was already a failure with attempt 1's errors, so its jobs were never fetched again
/// (ktsu-dev/BuildMonitor#394).
/// </summary>
[TestMethod]
public void AFailedReRunOfAFailedRunFetchesItsJobsAgain()
{
// Arrange
DateTimeOffset firstAttempt = new(2026, 11, 3, 9, 0, 0, TimeSpan.Zero);
Run run = FailedRun();
run.Started = firstAttempt;
run.Errors = ["[build] OLD: error CS1002 from attempt 1"];

// Act
GitHub.StartAttempt(run, firstAttempt.AddHours(1));

// Assert
Assert.IsEmpty(run.Errors, "Attempt 1's errors don't describe attempt 2");
Assert.AreEqual(firstAttempt.AddHours(1), run.Started);
Assert.IsTrue(GitHub.ShouldFetchRunErrors(RunStatus.Failure, RunStatus.Failure, run.Errors), "The new attempt's failure should be read from its jobs");
}

[TestMethod]
public void RereadingAnUnchangedFailedRunKeepsItsErrorsAndDoesNotRefetch()
{
// Arrange
DateTimeOffset started = new(2026, 11, 3, 9, 0, 0, TimeSpan.Zero);
Run run = FailedRun();
run.Started = started;
run.Errors = ["[build] error CS1002: ; expected"];

// Act
GitHub.StartAttempt(run, started);

// Assert
AssertErrors(run, "[build] error CS1002: ; expected");
Assert.IsFalse(GitHub.ShouldFetchRunErrors(RunStatus.Failure, RunStatus.Failure, run.Errors), "The #311 guarantee: an unchanged failed run is not refetched on every update");
}

[TestMethod]
public void ARunReadForTheFirstTimeKeepsWhateverItHas()
{
// Arrange
Run run = FailedRun();
DateTimeOffset started = new(2026, 11, 3, 9, 0, 0, TimeSpan.Zero);

// Act
GitHub.StartAttempt(run, started);

// Assert
Assert.AreEqual(started, run.Started);
Assert.IsFalse(GitHub.IsNewAttempt(default, started));
}

[TestMethod]
[DataRow(WorkflowJobConclusion.Failure, true)]
[DataRow(WorkflowJobConclusion.TimedOut, true)]
Expand Down
29 changes: 27 additions & 2 deletions BuildMonitor/Providers/GitHub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@

private Owner? OwnerPendingTokenPopup { get; set; }

internal override void ShowMenu()

Check warning on line 97 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Refactor this method to reduce its Cognitive Complexity from 23 to the 15 allowed.
{
if (Hexa.NET.ImGui.ImGui.BeginMenu(Name))
{
Expand Down Expand Up @@ -205,7 +205,7 @@
}

// Add all organizations the user belongs to
IReadOnlyList<Organization> organizations = await CurrentClient!.Organization.GetAllForCurrent().ConfigureAwait(false);

Check warning on line 208 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Remove this null-forgiving operator; the compiler already knows this expression is not null here.
foreach (Organization org in organizations)
{
OwnerName orgName = org.Login.As<OwnerName>();
Expand All @@ -220,7 +220,7 @@
}).ConfigureAwait(false);
}

internal override async Task UpdateRepositoriesAsync(Owner owner)

Check warning on line 223 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Refactor this method to reduce its Cognitive Complexity from 51 to the 15 allowed.
{
if (!HasValidCredentials(owner))
{
Expand Down Expand Up @@ -248,7 +248,7 @@
// Filter to only repos owned by this user (GetAllForCurrent returns repos from all orgs the user has access to)
IReadOnlyList<Octokit.Repository> currentUserRepos = await GitHubRepository.GetAllForCurrent().ConfigureAwait(false);
int totalCount = currentUserRepos.Count;
foreach (Octokit.Repository repo in currentUserRepos)

Check warning on line 251 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Loops should be simplified using the "Where" LINQ method
{
if (repo.Owner.Login.Equals(owner.Name.ToString(), StringComparison.OrdinalIgnoreCase))
{
Expand All @@ -269,7 +269,7 @@
catch (NotFoundException)
{
// Owner might be an org-only account, try org repos instead
userRepositories = [];

Check warning on line 272 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Remove this useless assignment to local variable 'userRepositories'.
}

// Try to get organization repositories - this only works for orgs
Expand All @@ -282,7 +282,7 @@
catch (NotFoundException)
{
// Owner is not an organization, that's fine
organizationRepositories = [];

Check warning on line 285 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Remove this useless assignment to local variable 'organizationRepositories'.
}
}

Expand All @@ -307,13 +307,13 @@

// Get existing repository or create new one
bool isNew = false;
Repository repository = owner.Repositories.GetOrAdd(repositoryId, _ =>

Check warning on line 310 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Use the lambda parameter instead of capturing the argument 'repositoryId'
{
isNew = true;
return owner.CreateRepository(repositoryName, repositoryId);
});

if (isNew)

Check warning on line 316 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Change this condition so that it does not always evaluate to 'False'. Some code paths are unreachable.
{
newRepos++;
Log.Info($"GitHub: Discovered repository: {owner.Name}/{repositoryName}");
Expand Down Expand Up @@ -348,7 +348,7 @@
}
}

if (newRepos > 0 || updatedRepos > 0 || archivedRepos > 0)

Check warning on line 351 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Change this condition so that it does not always evaluate to 'False'.
{
Log.Info($"GitHub: Repository update for {owner.Name}: {newRepos} new, {updatedRepos} updated, {archivedRepos} archived removed, {owner.Repositories.Count} total");
}
Expand All @@ -374,7 +374,7 @@

// Get existing build or create new one
bool isNew = false;
Build build = repository.Builds.GetOrAdd(buildId, _ =>

Check warning on line 377 in BuildMonitor/Providers/GitHub.cs

View workflow job for this annotation

GitHub Actions / ci / .NET / Analyze & Release

Use the lambda parameter instead of capturing the argument 'buildId'
{
isNew = true;
return repository.CreateBuild(buildName, buildId);
Expand Down Expand Up @@ -515,7 +515,7 @@

private async Task UpdateRunFromWorkflowAsync(Run run, WorkflowRun gitHubRun)
{
run.Started = gitHubRun.RunStartedAt;
StartAttempt(run, gitHubRun.RunStartedAt);
run.LastUpdated = gitHubRun.UpdatedAt;
run.Branch = gitHubRun.HeadBranch.As<BranchName>();

Expand Down Expand Up @@ -559,11 +559,36 @@
/// Whether a run's jobs should be fetched for error details: when it has just failed, or is a
/// failure whose errors have not been fetched yet. A completed fetch always leaves at least one
/// error behind (see <see cref="WithRunLevelFallback"/>), so it is not repeated on every update
/// (ktsu-dev/BuildMonitor#311).
/// (ktsu-dev/BuildMonitor#311). A new attempt of a failed run has its errors dropped by
/// <see cref="StartAttempt"/>, so it is fetched again too (ktsu-dev/BuildMonitor#394).
/// </summary>
internal static bool ShouldFetchRunErrors(RunStatus status, RunStatus previousStatus, IReadOnlyList<string> errors) =>
status == RunStatus.Failure && (previousStatus != RunStatus.Failure || errors.Count == 0);

/// <summary>
/// Whether a run read from GitHub is a later attempt than the one stored: a re-run keeps the run
/// id and gets a new start time. A run that has never been read has no stored start time, and is
/// not a re-run.
/// </summary>
internal static bool IsNewAttempt(DateTimeOffset storedStarted, DateTimeOffset started) =>
storedStarted != default && started != storedStarted;

/// <summary>
/// Records the start time GitHub reports for a run. A re-run keeps the run id but starts a new
/// attempt with its own start time, and the previous attempt's errors no longer describe the run,
/// so they are dropped, which makes <see cref="ShouldFetchRunErrors"/> fetch them afresh if the new
/// attempt failed too (ktsu-dev/BuildMonitor#394).
/// </summary>
internal static void StartAttempt(Run run, DateTimeOffset started)
{
if (IsNewAttempt(run.Started, started))
{
run.Errors = [];
}

run.Started = started;
}

/// <summary>
/// Whether a job's conclusion is one that fails its run. A job that hit its timeout ends as
/// <c>timed_out</c>, not <c>failure</c>, and is just as much the reason the run failed.
Expand Down
Loading