From c3d03fba217ea3000c43fbb9c35433d9f82935fe Mon Sep 17 00:00:00 2001 From: Tim Cassell Date: Fri, 6 Nov 2026 01:22:05 -0500 Subject: [PATCH] Fail integration tests on logged warnings A failed reference-gathering pass is only a warning, and the benchmark still builds and runs, so CanExecute's full validation now fails on any logged warning. Analyser conclusions are excluded since they depend on the machine. Tests that expect warnings opt out with allowWarnings, as the NativeAOT tests do for BenchmarkDotNet's own trim and AOT warnings. With publishing toolchains going through reference gathering, that check catches a Wasm failure: the static web assets pipeline converts the program's assembly, which the gathering pass never compiles. The pass only resolves references, so it disables static web assets. The other integration tests now cover gathering for publishing toolchains, which makes the dedicated R2R gathering test redundant. Co-Authored-By: Claude Opus 5.5 --- .../Toolchains/CsProj/CsProjBuilder.cs | 6 +- .../NativeAotTests.cs | 7 ++- .../R2RTests.cs | 61 ------------------- .../Shared/BenchmarkTestExecutor.cs | 29 +++++---- .../Shared/WarningCollectingLogger.cs | 58 ++++++++++++++++++ 5 files changed, 85 insertions(+), 76 deletions(-) create mode 100644 tests/BenchmarkDotNet.IntegrationTests/Shared/WarningCollectingLogger.cs diff --git a/src/BenchmarkDotNet/Toolchains/CsProj/CsProjBuilder.cs b/src/BenchmarkDotNet/Toolchains/CsProj/CsProjBuilder.cs index 46dce5ca63..1143500364 100644 --- a/src/BenchmarkDotNet/Toolchains/CsProj/CsProjBuilder.cs +++ b/src/BenchmarkDotNet/Toolchains/CsProj/CsProjBuilder.cs @@ -321,9 +321,11 @@ protected void AddGatheredReferences(XElement project) new XElement("SkipCompilerExecution", "true"), // A self-contained executable requires an apphost, which this pass never produces. new XElement("SelfContained", "false"), - // Both of these copy a build product that no longer gets produced. + // All of these process a build product that no longer gets produced (static web assets convert + // and fingerprint the program's assembly, for one, which is how Wasm lays out its app). new XElement("UseAppHost", "false"), - new XElement("SkipCopyBuildProduct", "true"))); + new XElement("SkipCopyBuildProduct", "true"), + new XElement("StaticWebAssetsEnabled", "false"))); project.Add(new XElement("Target", new XAttribute("Name", "BdnWriteReferences"), diff --git a/tests/BenchmarkDotNet.IntegrationTests/NativeAotTests.cs b/tests/BenchmarkDotNet.IntegrationTests/NativeAotTests.cs index 5ccb13a7c0..6feee56456 100644 --- a/tests/BenchmarkDotNet.IntegrationTests/NativeAotTests.cs +++ b/tests/BenchmarkDotNet.IntegrationTests/NativeAotTests.cs @@ -34,6 +34,9 @@ private ManualConfig GetConfig() .WithEnvironmentVariable(NativeAotBenchmark.EnvVarKey, IsAvx2Supported().ToString().ToLower())); } + // BenchmarkDotNet itself is not trim or AOT safe, so publishing reports IL2104 and IL3053 for its assembly. + private const bool AllowWarnings = true; + [FactEnvSpecific("It's impossible to reliably detect the version of NativeAOT if the process is not a .NET Core or NativeAOT process", EnvRequirement.DotNetCoreOnly, EnvRequirement.NonGitHubDraftPR)] public void LatestNativeAotVersionIsSupported() { @@ -42,7 +45,7 @@ public void LatestNativeAotVersionIsSupported() try { - CanExecute(GetConfig()); + CanExecute(GetConfig(), allowWarnings: AllowWarnings); } catch (MisconfiguredEnvironmentException e) { @@ -66,7 +69,7 @@ public void NativeAotSupportsInProcessDiagnosers() try { - CanExecute(config); + CanExecute(config, allowWarnings: AllowWarnings); } catch (MisconfiguredEnvironmentException e) { diff --git a/tests/BenchmarkDotNet.IntegrationTests/R2RTests.cs b/tests/BenchmarkDotNet.IntegrationTests/R2RTests.cs index ffb10cb913..ae397794cf 100644 --- a/tests/BenchmarkDotNet.IntegrationTests/R2RTests.cs +++ b/tests/BenchmarkDotNet.IntegrationTests/R2RTests.cs @@ -2,11 +2,8 @@ using BenchmarkDotNet.Configs; using BenchmarkDotNet.IntegrationTests.Xunit; using BenchmarkDotNet.Jobs; -using BenchmarkDotNet.Loggers; using BenchmarkDotNet.Tests.XUnit; using BenchmarkDotNet.Toolchains.R2R; -using System.Reflection; -using System.Xml.Linq; namespace BenchmarkDotNet.IntegrationTests { @@ -14,64 +11,6 @@ public class R2RTests : BenchmarkTestExecutor { public R2RTests(ITestOutputHelper output) : base(output) { } - [FactEnvSpecific("R2R requires .NET Core runtime", EnvRequirement.DotNetCoreOnly)] - public void R2RToolchainGathersReferencesBeforePublishing() - { - var logger = new AccumulationLogger(); - var config = ManualConfig.CreateEmpty() - .AddLogger(logger) - .AddJob(Job.Dry - .WithToolchain(CsProjR2RToolchain.R2R10_0)) - .WithBuildTimeout(TimeSpan.FromSeconds(540)) - .WithOption(ConfigOptions.KeepBenchmarkFiles, true) - .WithOption(ConfigOptions.LogBuildOutput, true); - var summary = CanExecute(config, fullValidation: false); - string log = logger.GetLog(); - Assert.True(summary.Reports.Length == 1, log); - var report = summary.Reports[0]; - var paths = report.BuildResult.ArtifactsPaths; - Output.WriteLine($"Reference-gathering artifacts: {paths.BuildArtifactsDirectoryPath}"); - - string referencesList = Path.Combine(paths.BuildArtifactsDirectoryPath, "ref.txt"); - string referencesProps = Path.Combine(paths.BuildArtifactsDirectoryPath, "ref.props"); - Assert.True(File.Exists(referencesList) && new FileInfo(referencesList).Length > 0, - $"Reference gathering did not produce a nonempty ref.txt.{Environment.NewLine}{log}"); - Assert.True(File.Exists(referencesProps), - $"Reference gathering did not complete successfully (ref.props missing).{Environment.NewLine}{log}"); - - var gatheredPaths = File.ReadAllLines(referencesList).Select(line => line.Split('|')[0]).ToArray(); - var references = XDocument.Load(referencesProps).Descendants("Reference").ToArray(); - foreach (string assemblyName in new[] { typeof(Job).Assembly.GetName().Name!, typeof(R2RBenchmark).Assembly.GetName().Name! }) - { - var matchingReferences = references.Where(reference => (string?)reference.Attribute("Include") == assemblyName).ToArray(); - Assert.True(matchingReferences.Length == 1, $"Expected gathered reference {assemblyName}.{Environment.NewLine}{log}"); - string? hintPath = (string?)matchingReferences[0].Element("HintPath"); - Assert.True(hintPath is not null && File.Exists(hintPath), $"Gathered reference {assemblyName} is missing.{Environment.NewLine}{log}"); - Assert.True(AssemblyName.GetAssemblyName(hintPath!).Name == assemblyName, log); - Assert.True(gatheredPaths.Any(path => string.Equals(path, hintPath, StringComparison.OrdinalIgnoreCase)), log); - } - - Assert.True(log.Contains("BenchmarkDotNetGatherReferences=true"), log); - Assert.False(log.Contains("--no-dependencies"), log); - Assert.False(log.Contains("Failed to gather assembly references"), log); - Assert.True(report.BuildResult.IsBuildSuccess, log); - Assert.True(File.Exists(paths.ExecutablePath), log); - string compositeAssembly = Path.Combine(paths.BinariesDirectoryPath, $"{paths.ProgramName}.r2r.dll"); - Assert.True(File.Exists(compositeAssembly) && new FileInfo(compositeAssembly).Length > 0, - $"Expected composite assembly: {compositeAssembly}{Environment.NewLine}{log}"); - Assert.True(report.ExecuteResults.Count > 0 && report.ExecuteResults.All(result => result.IsSuccess), log); - Assert.True(report.Success, log); - - var project = XDocument.Load(paths.ProjectFilePath); - var r2rProperties = Assert.Single(project.Root!.Elements("PropertyGroup"), - group => group.Element("PublishReadyToRunComposite") is not null); - Assert.Equal("true", (string?)r2rProperties.Element("SelfContained")); - Assert.Equal("true", (string?)r2rProperties.Element("PublishReadyToRun")); - Assert.Equal("true", (string?)r2rProperties.Element("PublishReadyToRunComposite")); - Assert.Equal(BenchmarkDotNet.Portability.RuntimeInformation.GetPortableRuntimeIdentifier(), - (string?)r2rProperties.Element("RuntimeIdentifier")); - } - [FactEnvSpecific("R2R requires .NET Core runtime", EnvRequirement.DotNetCoreOnly, EnvRequirement.NonGitHubDraftPR)] public void R2RToolchainCanExecuteBenchmarks() { diff --git a/tests/BenchmarkDotNet.IntegrationTests/Shared/BenchmarkTestExecutor.cs b/tests/BenchmarkDotNet.IntegrationTests/Shared/BenchmarkTestExecutor.cs index 58c72ad38f..7c8fb9bfe2 100644 --- a/tests/BenchmarkDotNet.IntegrationTests/Shared/BenchmarkTestExecutor.cs +++ b/tests/BenchmarkDotNet.IntegrationTests/Shared/BenchmarkTestExecutor.cs @@ -26,9 +26,10 @@ protected BenchmarkTestExecutor(ITestOutputHelper output) /// type that defines Benchmarks /// Optional custom config to be used instead of the default /// Optional: disable validation (default = true/enabled) + /// Optional: don't fail the validation on logged warnings (default = false) /// The summary from the benchmark run - public Summary CanExecute(IConfig? config = null, bool fullValidation = true) - => CanExecute(typeof(TBenchmark), config, fullValidation); + public Summary CanExecute(IConfig? config = null, bool fullValidation = true, bool allowWarnings = false) + => CanExecute(typeof(TBenchmark), config, fullValidation, allowWarnings); /// /// Runs Benchmarks with the most simple config (SingleRunFastConfig) @@ -38,31 +39,34 @@ public Summary CanExecute(IConfig? config = null, bool fullValidatio /// type that defines Benchmarks /// Optional custom config to be used instead of the default /// Optional: disable validation (default = true/enabled) + /// Optional: don't fail the validation on logged warnings (default = false) /// The summary from the benchmark run - public Summary CanExecute(Type type, IConfig? config = null, bool fullValidation = true) + public Summary CanExecute(Type type, IConfig? config = null, bool fullValidation = true, bool allowWarnings = false) { // Make sure we ALWAYS combine the Config (default or passed in) with any Config applied to the Type/Class - var summary = BenchmarkRunner.Run(type, GetMinimalConfig(config)); + var warnings = new WarningCollectingLogger(); + var summary = BenchmarkRunner.Run(type, GetMinimalConfig(config).AddLogger(warnings)); if (fullValidation) { - ValidateSummary(summary); + ValidateSummary(summary, warnings, allowWarnings); } return summary; } - public ValueTask CanExecuteAsync(IConfig? config = null, bool fullValidation = true) - => CanExecuteAsync(typeof(TBenchmark), config, fullValidation); + public ValueTask CanExecuteAsync(IConfig? config = null, bool fullValidation = true, bool allowWarnings = false) + => CanExecuteAsync(typeof(TBenchmark), config, fullValidation, allowWarnings); - public async ValueTask CanExecuteAsync(Type type, IConfig? config = null, bool fullValidation = true) + public async ValueTask CanExecuteAsync(Type type, IConfig? config = null, bool fullValidation = true, bool allowWarnings = false) { // Make sure we ALWAYS combine the Config (default or passed in) with any Config applied to the Type/Class - var summary = await BenchmarkRunner.RunAsync(type, GetMinimalConfig(config)); + var warnings = new WarningCollectingLogger(); + var summary = await BenchmarkRunner.RunAsync(type, GetMinimalConfig(config).AddLogger(warnings)); if (fullValidation) { - ValidateSummary(summary); + ValidateSummary(summary, warnings, allowWarnings); } return summary; @@ -85,7 +89,7 @@ private IConfig GetMinimalConfig(IConfig? config) return config!; } - private static void ValidateSummary(Summary summary) + private static void ValidateSummary(Summary summary, WarningCollectingLogger warnings, bool allowWarnings) { Assert.False(summary.HasCriticalValidationErrors, "The \"Summary\" should have NOT \"HasCriticalValidationErrors\""); @@ -103,6 +107,9 @@ private static void ValidateSummary(Summary summary) Assert.True(summary.Reports.All(r => r.ExecuteResults.All(er => er.IsSuccess)), "All reports should have succeeded to execute"); + + Assert.True(allowWarnings || warnings.Warnings.Count == 0, + "The run should not log any warnings:" + Environment.NewLine + string.Join(Environment.NewLine, warnings.Warnings)); } protected IConfig CreateSimpleConfig(OutputLogger? logger = null, Job? job = null) diff --git a/tests/BenchmarkDotNet.IntegrationTests/Shared/WarningCollectingLogger.cs b/tests/BenchmarkDotNet.IntegrationTests/Shared/WarningCollectingLogger.cs new file mode 100644 index 0000000000..bf93c769ae --- /dev/null +++ b/tests/BenchmarkDotNet.IntegrationTests/Shared/WarningCollectingLogger.cs @@ -0,0 +1,58 @@ +using BenchmarkDotNet.Loggers; + +namespace BenchmarkDotNet.IntegrationTests; + +/// +/// Collects the warnings a benchmark run logs, so that a test can fail on them. +/// +/// +/// Analyser conclusions are skipped: they describe the measurements and the machine (a tiny Dry run, an antivirus, +/// a virtual machine) rather than a problem with the run itself, and the summary keeps them anyway. +/// +internal sealed class WarningCollectingLogger : ILogger +{ + // The header ConclusionHelper prints before its warning conclusions, which last until something else is written. + private const string ConclusionsHeader = "// * Warnings *"; + + private readonly List warnings = []; + private string currentLine = ""; + private bool inConclusions; + + public string Id => nameof(WarningCollectingLogger); + public int Priority => 0; + + public IReadOnlyList Warnings => warnings; + + public void Write(LogKind logKind, string text) + { + Track(logKind, text); + if (logKind == LogKind.Warning) + currentLine += text; + } + + public void WriteLine() => EndLine(); + + public void WriteLine(LogKind logKind, string text) + { + Write(logKind, text); + EndLine(); + } + + public void Flush() { } + + private void Track(LogKind logKind, string text) + { + if (logKind != LogKind.Warning) + inConclusions = false; + else if (text == ConclusionsHeader) + inConclusions = true; + } + + private void EndLine() + { + // ConclusionHelper writes an empty warning line before its header. + if (!inConclusions && currentLine.Length > 0) + warnings.Add(currentLine); + currentLine = ""; + } +}