[release/10.0.1xx] Improve PerformanceTest reliability - #12389
[release/10.0.1xx] Improve PerformanceTest reliability#12389jonathanpeppers wants to merge 2 commits into
Conversation
These two commonly fail (but also randomly pass!): * `Build_From_Clean_DontIncludeRestore` * `Build_JLO_Change` The general goal of the MSBuild performance tests is to catch large regressions in incremental builds. Let's remove these two as they are lower value: * Flaky, due to the long time they take and the variability of build machine performance. * Perf regressions in initial build time are less important than incremental build time regressions. It would be higher value for PRs to be greener than run these, I think. `Build_No_Changes` and `Build_CSharp_Change` are more valuable to keep as they are less flaky and more likely to catch real-world regressions.
#11249) * [tests] Skip PerformanceTest on slow CI machines using evaluation time When a performance test exceeds its expected time, check the MSBuild project evaluation duration from the binlog. If evaluation alone exceeds 500ms (normal is ~200-350ms), the CI machine is too slow for reliable performance measurements. Use Assert.Inconclusive() instead of Assert.Fail() so the test is marked as skipped rather than failed. Analysis of 10 recent CI failures showed evaluation times of 558-854ms on slow machines, while normal machines should be well under 500ms. Also refactored binlog reading so BinaryLog.ReadBuild() is called once per iteration, with the Build object passed to helper methods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix CS0118: use alias to disambiguate Build type from namespace Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This backport improves reliability of MSBuildDeviceIntegration performance tests on the release/10.0.1xx branch by removing known-flaky perf tests and by skipping failures on slow CI machines using MSBuild project evaluation time derived from the binlog.
Changes:
- Removed two flaky MSBuild performance tests (
Build_From_Clean_DontIncludeRestore,Build_JLO_Change) and their reference timing entries. - When perf results exceed expected thresholds, evaluates binlog
ProjectEvaluationdurations and marks the test inconclusive when evaluation indicates a slow machine. - Refactored binlog reading so
BinaryLog.ReadBuild()happens once per iteration and helper methods consume the parsedBuild.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/MSBuildDeviceIntegration/Tests/PerformanceTest.cs | Removes flaky tests; adds slow-machine detection via binlog evaluation time; refactors binlog reading to reduce repeated parsing. |
| tests/msbuild-times-reference/MSBuildDeviceIntegration.csv | Removes reference timing entries for the deleted flaky tests. |
Suppressed comments (2)
tests/MSBuildDeviceIntegration/Tests/PerformanceTest.cs:90
- Same issue as
Profile:ProfileTaskaverages across iterations but only checks evaluation time from the last binlog. This can still leave the test flaky on slow/variable CI machines.
Accumulate the max evaluation time across iterations and use that for the slow-machine check.
double total = 0;
StructuredBuild build = null;
for (int i=0; i < iterations; i++) {
action (builder);
build = ReadBinLog (builder);
var duration = GetTaskDuration (build, builder, task);
tests/MSBuildDeviceIntegration/Tests/PerformanceTest.cs:109
AssertSlowMachinecurrently enumeratesFindChildrenRecursive<ProjectEvaluation>()twice (Any()thenMax()), and its signature forces callers to pass aBuildeven though the method only needs the evaluation-time value. Since this code runs on the failure path, simplify it to compute the max evaluation time once and accept that value as an argument (paired with tracking it across iterations).
void AssertSlowMachine (StructuredBuild build, int expected, double actual)
{
var evaluations = build.FindChildrenRecursive<ProjectEvaluation> ();
var maxEval = evaluations.Any () ? evaluations.Max (e => e.Duration.TotalMilliseconds) : 0;
TestContext.Out.WriteLine ($"max evaluation time: {maxEval}ms (threshold: {MaxEvaluationTimeInMs}ms)");
if (maxEval > MaxEvaluationTimeInMs) {
Assert.Inconclusive ($"Exceeded expected time of {expected}ms, actual {actual}ms, but evaluation time was {maxEval:F0}ms (threshold: {MaxEvaluationTimeInMs}ms), indicating a slow CI machine.");
}
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
/azp run |
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
|
@dalexsoto review |
dalexsoto
left a comment
There was a problem hiding this comment.
The two backported reliability changes match their stated scope. The final-binlog limitation is an accepted follow-up from main, and no independent blocker remains.
Backports #10756 and #11249 to release/10.0.1xx.
This PR contains only their two squash commits from main, cherry-picked in order.