From 14096a7ee46e9612b3729d33557e9b4d5f4eb89b Mon Sep 17 00:00:00 2001 From: John Wesley Walker III <81404201+jww3@users.noreply.github.com> Date: Wed, 8 Feb 2023 13:33:26 +0000 Subject: [PATCH] Fixed/simplified unit tests. Close-over KeyValuePairs passed via IssueMetadata to ensure no deferred evaluation. (An `IEnumerable>` could very well be a mutable dictionary, so we want to capture its content in the moment -- not some future version of it.) --- src/Runner.Worker/ExecutionContext.cs | 5 ++-- src/Runner.Worker/IssueMatcher.cs | 5 +++- src/Test/L0/TestUtil.cs | 23 +++++++++++++++++++ src/Test/L0/Worker/ActionCommandManagerL0.cs | 15 +----------- src/Test/L0/Worker/ActionManagerL0.cs | 13 +---------- src/Test/L0/Worker/ActionManifestManagerL0.cs | 14 +---------- src/Test/L0/Worker/ActionRunnerL0.cs | 14 +---------- .../L0/Worker/CreateStepSummaryCommandL0.cs | 14 +---------- src/Test/L0/Worker/ExecutionContextL0.cs | 6 ++--- src/Test/L0/Worker/OutputManagerL0.cs | 14 +---------- 10 files changed, 38 insertions(+), 85 deletions(-) diff --git a/src/Runner.Worker/ExecutionContext.cs b/src/Runner.Worker/ExecutionContext.cs index 9fb0786af..4bfcfa5f7 100644 --- a/src/Runner.Worker/ExecutionContext.cs +++ b/src/Runner.Worker/ExecutionContext.cs @@ -586,14 +586,13 @@ namespace GitHub.Runner.Worker var result = new Issue() { Type = issueType, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false, Message = refinedMessage, }; - if (metadata != null) { + result.Category = metadata.Category; + result.IsInfrastructureIssue = metadata.IsInfrastructureIssue; foreach (var kvp in metadata.Data) { result[kvp.Key] = kvp.Value; diff --git a/src/Runner.Worker/IssueMatcher.cs b/src/Runner.Worker/IssueMatcher.cs index a1e85fed5..f55a49de6 100644 --- a/src/Runner.Worker/IssueMatcher.cs +++ b/src/Runner.Worker/IssueMatcher.cs @@ -21,7 +21,10 @@ namespace GitHub.Runner.Worker this.Category = category; this.IsInfrastructureIssue = infrastructureIssue; this.LogMessageOverride = logMessageOverride; - this.Data = data; + + // Close-over the incoming IEnumerable to force immediate evaluation. + var empty = Enumerable.Empty>(); + this.Data = new Dictionary(data ?? empty); } public readonly string Category; diff --git a/src/Test/L0/TestUtil.cs b/src/Test/L0/TestUtil.cs index c6c60be56..1f2458655 100644 --- a/src/Test/L0/TestUtil.cs +++ b/src/Test/L0/TestUtil.cs @@ -2,6 +2,8 @@ using Xunit; using GitHub.Runner.Sdk; using System.Runtime.CompilerServices; +using GitHub.DistributedTask.WebApi; +using GitHub.Runner.Worker; namespace GitHub.Runner.Common.Tests { @@ -41,5 +43,26 @@ namespace GitHub.Runner.Common.Tests Assert.True(Directory.Exists(testDataDir)); return testDataDir; } + + public static IReadOnlyIssue CreateTestIssue(IssueType type, string message, IssueMetadata metadata, bool writeToLog) + { + var result = new Issue() + { + Type = type, + Message = message, + }; + + if (metadata != null) + { + result.Category = metadata.Category; + result.IsInfrastructureIssue = metadata.IsInfrastructureIssue; + foreach (var kvp in metadata.Data) + { + result[kvp.Key] = kvp.Value; + } + } + + return result; + } } } diff --git a/src/Test/L0/Worker/ActionCommandManagerL0.cs b/src/Test/L0/Worker/ActionCommandManagerL0.cs index a94413cb8..63cfea9e3 100644 --- a/src/Test/L0/Worker/ActionCommandManagerL0.cs +++ b/src/Test/L0/Worker/ActionCommandManagerL0.cs @@ -6,7 +6,6 @@ using GitHub.DistributedTask.Pipelines.ContextData; using GitHub.DistributedTask.WebApi; using GitHub.Runner.Worker; using GitHub.Runner.Worker.Container; -using GitHub.Services.Common; using Moq; using Xunit; using Pipelines = GitHub.DistributedTask.Pipelines; @@ -94,19 +93,7 @@ namespace GitHub.Runner.Common.Tests.Worker }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())) .Callback((IReadOnlyIssue issue) => { diff --git a/src/Test/L0/Worker/ActionManagerL0.cs b/src/Test/L0/Worker/ActionManagerL0.cs index 1d14bf5ed..8d25c9039 100644 --- a/src/Test/L0/Worker/ActionManagerL0.cs +++ b/src/Test/L0/Worker/ActionManagerL0.cs @@ -2148,18 +2148,7 @@ runs: _ec.Object.Global.Plan = new TaskOrchestrationPlanReference(); _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"[{tag}]{message}"); }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); _ec.Setup(x => x.GetGitHubContext("workspace")).Returns(Path.Combine(_workFolder, "actions", "actions")); diff --git a/src/Test/L0/Worker/ActionManifestManagerL0.cs b/src/Test/L0/Worker/ActionManifestManagerL0.cs index aaec5f772..cd357d169 100644 --- a/src/Test/L0/Worker/ActionManifestManagerL0.cs +++ b/src/Test/L0/Worker/ActionManifestManagerL0.cs @@ -862,19 +862,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"{tag}{message}"); }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); } diff --git a/src/Test/L0/Worker/ActionRunnerL0.cs b/src/Test/L0/Worker/ActionRunnerL0.cs index f56f85536..236cba3d0 100644 --- a/src/Test/L0/Worker/ActionRunnerL0.cs +++ b/src/Test/L0/Worker/ActionRunnerL0.cs @@ -445,19 +445,7 @@ namespace GitHub.Runner.Common.Tests.Worker _ec.Object.Global.Variables = new Variables(_hc, new Dictionary()); _ec.Setup(x => x.Write(It.IsAny(), It.IsAny())).Callback((string tag, string message) => { _hc.GetTrace().Info($"[{tag}]{message}"); }); _ec.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _ec.Setup(x => x.AddIssue(It.IsAny())).Callback((IReadOnlyIssue issue) => { _hc.GetTrace().Info($"[{issue.Type}]{issue.Message}"); }); _hc.SetSingleton(_actionManager.Object); diff --git a/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs index 69b2cbdda..38acb1e2a 100644 --- a/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs +++ b/src/Test/L0/Worker/CreateStepSummaryCommandL0.cs @@ -255,19 +255,7 @@ namespace GitHub.Runner.Common.Tests.Worker _trace.Info($"Issue '{issue.Type}': {issue.Message}"); }); _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _executionContext.Setup(x => x.Write(It.IsAny(), It.IsAny())) .Callback((string tag, string message) => { diff --git a/src/Test/L0/Worker/ExecutionContextL0.cs b/src/Test/L0/Worker/ExecutionContextL0.cs index 91adbfb18..216737ab5 100644 --- a/src/Test/L0/Worker/ExecutionContextL0.cs +++ b/src/Test/L0/Worker/ExecutionContextL0.cs @@ -246,9 +246,9 @@ namespace GitHub.Runner.Common.Tests.Worker embeddedStep.AddIssue(embeddedStep.CreateIssue(IssueType.Warning, "warning annotation that should have step and line number information", null, true)); embeddedStep.AddIssue(embeddedStep.CreateIssue(IssueType.Notice, "notice annotation that should have step and line number information", null, true)); - jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Error).Count() == 1)), Times.AtLeastOnce); - jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Warning).Count() == 1)), Times.AtLeastOnce); - jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i.Data.ContainsKey("stepNumber") && i.Data.ContainsKey("logFileLineNumber") && i.Type == IssueType.Notice).Count() == 1)), Times.AtLeastOnce); + jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i["stepNumber"] != null && i["logFileLineNumber"] != null && i.Type == IssueType.Error).Count() == 1)), Times.AtLeastOnce); + jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i["stepNumber"] != null && i["logFileLineNumber"] != null && i.Type == IssueType.Warning).Count() == 1)), Times.AtLeastOnce); + jobServerQueue.Verify(x => x.QueueTimelineRecordUpdate(It.IsAny(), It.Is(t => t.Issues.Where(i => i["stepNumber"] != null && i["logFileLineNumber"] != null && i.Type == IssueType.Notice).Count() == 1)), Times.AtLeastOnce); } } diff --git a/src/Test/L0/Worker/OutputManagerL0.cs b/src/Test/L0/Worker/OutputManagerL0.cs index 09d98d965..9b74295e2 100644 --- a/src/Test/L0/Worker/OutputManagerL0.cs +++ b/src/Test/L0/Worker/OutputManagerL0.cs @@ -984,19 +984,7 @@ namespace GitHub.Runner.Common.Tests.Worker _onMatcherChanged = handler; }); _executionContext.Setup(x => x.CreateIssue(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny())) - .Returns((DTWebApi.IssueType type, string message, IssueMetadata metadata, bool writeToLog) => - { - var result = new DTWebApi.Issue() - { - Type = type, - Message = message, - Category = metadata?.Category, - IsInfrastructureIssue = metadata?.IsInfrastructureIssue ?? false - }; - result.Data.AddRangeIfRangeNotNull(metadata?.Data); - return result; - }); - + .Returns(TestUtil.CreateTestIssue); _executionContext.Setup(x => x.AddIssue(It.IsAny())) .Callback((DTWebApi.IReadOnlyIssue issue) => {