Skip to content

Commit d93c427

Browse files
Youssef1313Youssef1313
authored
Adjust dotnet test UX (for MTP) (dotnet#48797)
1 parent 7601b9c commit d93c427

File tree

7 files changed

+189
-99
lines changed

7 files changed

+189
-99
lines changed

‎src/Cli/dotnet/Commands/Test/Terminal/TerminalTestReporter.cs

Lines changed: 10 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -159,30 +159,15 @@ public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount,
159159
_terminalWithProgress.StartShowingProgress(workerCount);
160160
}
161161

162-
public void AssemblyRunStarted(string assembly, string? targetFramework, string? architecture, string? executionId, string? instanceId)
162+
public void AssemblyRunStarted(string assembly, string? targetFramework, string? architecture, string? executionId)
163163
{
164164
var assemblyRun = GetOrAddAssemblyRun(assembly, targetFramework, architecture, executionId);
165-
assemblyRun.Tries.Add(instanceId);
165+
assemblyRun.TryCount++;
166166

167167
// If we fail to parse out the parameter correctly this will enable retry on re-run of the assembly within the same execution.
168168
// Not good enough for general use, because we want to show (try 1) even on the first try, but this will at
169169
// least show (try 2) etc. So user is still aware there is retry going on, and counts of tests won't break.
170-
_isRetry |= assemblyRun.Tries.Count > 1;
171-
172-
if (_isRetry)
173-
{
174-
// When we are retrying the new assembly run should ignore all previously failed tests and
175-
// clear all errors. We restarted the run and will retry all failed tests.
176-
//
177-
// In case of folded dynamic data tests we do not know how many tests we will get in each run,
178-
// if more or less, or the same amount as before,and we also will rerun tests that passed previously
179-
// because we are unable to run just a single test from that dynamic data source.
180-
// This will cause the total number of tests to differ between runs, and there is nothing we can do about it.
181-
assemblyRun.TotalTests -= assemblyRun.FailedTests;
182-
assemblyRun.RetriedFailedTests += assemblyRun.FailedTests;
183-
assemblyRun.FailedTests = 0;
184-
assemblyRun.ClearAllMessages();
185-
}
170+
_isRetry |= assemblyRun.TryCount > 1;
186171

187172
if (_options.ShowAssembly && _options.ShowAssemblyStartAndComplete)
188173
{
@@ -191,7 +176,7 @@ public void AssemblyRunStarted(string assembly, string? targetFramework, string?
191176
if (_isRetry)
192177
{
193178
terminal.SetColor(TerminalColor.DarkGray);
194-
terminal.Append($"({string.Format(CliCommandStrings.Try, assemblyRun.Tries.Count)}) ");
179+
terminal.Append($"({string.Format(CliCommandStrings.Try, assemblyRun.TryCount)}) ");
195180
terminal.ResetColor();
196181
}
197182

@@ -480,7 +465,7 @@ internal void TestCompleted(
480465
string? errorOutput)
481466
{
482467
TestProgressState asm = _assemblies[$"{assembly}|{targetFramework}|{architecture}|{executionId}"];
483-
var attempt = asm.Tries.Count;
468+
var attempt = asm.TryCount;
484469

485470
if (_options.ShowActiveTests)
486471
{
@@ -493,20 +478,17 @@ internal void TestCompleted(
493478
case TestOutcome.Timeout:
494479
case TestOutcome.Canceled:
495480
case TestOutcome.Fail:
496-
asm.FailedTests++;
497-
asm.TotalTests++;
481+
asm.ReportFailedTest(testNodeUid, instanceId);
498482
break;
499483
case TestOutcome.Passed:
500-
asm.PassedTests++;
501-
asm.TotalTests++;
484+
asm.ReportPassingTest(testNodeUid, instanceId);
502485
break;
503486
case TestOutcome.Skipped:
504-
asm.SkippedTests++;
505-
asm.TotalTests++;
487+
asm.ReportSkippedTest(testNodeUid, instanceId);
506488
break;
507489
}
508490

509-
if (_isRetry && asm.Tries.Count > 1 && outcome == TestOutcome.Passed)
491+
if (_isRetry && asm.TryCount > 1 && outcome == TestOutcome.Passed)
510492
{
511493
// This is a retry of a test, and the test succeeded, so these tests are potentially flaky.
512494
// Tests that come from dynamic data sources and previously succeeded will also run on the second attempt,
@@ -904,50 +886,6 @@ public void StartCancelling()
904886
});
905887
}
906888

907-
internal void WriteErrorMessage(string assembly, string? targetFramework, string? architecture, string? executionId, string text, int? padding)
908-
{
909-
TestProgressState asm = GetOrAddAssemblyRun(assembly, targetFramework, architecture, executionId);
910-
asm.AddError(text);
911-
912-
_terminalWithProgress.WriteToTerminal(terminal =>
913-
{
914-
terminal.SetColor(TerminalColor.Red);
915-
if (padding == null)
916-
{
917-
terminal.AppendLine(text);
918-
}
919-
else
920-
{
921-
AppendIndentedLine(terminal, text, new string(' ', padding.Value));
922-
}
923-
924-
terminal.ResetColor();
925-
});
926-
}
927-
928-
internal void WriteWarningMessage(string assembly, string? targetFramework, string? architecture, string? executionId, string text, int? padding)
929-
{
930-
TestProgressState asm = GetOrAddAssemblyRun(assembly, targetFramework, architecture, executionId);
931-
asm.AddWarning(text);
932-
_terminalWithProgress.WriteToTerminal(terminal =>
933-
{
934-
terminal.SetColor(TerminalColor.Yellow);
935-
if (padding == null)
936-
{
937-
terminal.AppendLine(text);
938-
}
939-
else
940-
{
941-
AppendIndentedLine(terminal, text, new string(' ', padding.Value));
942-
}
943-
944-
terminal.ResetColor();
945-
});
946-
}
947-
948-
internal void WriteErrorMessage(string assembly, string? targetFramework, string? architecture, string? executionId, Exception exception)
949-
=> WriteErrorMessage(assembly, targetFramework, architecture, executionId, exception.ToString(), padding: null);
950-
951889
public void WriteMessage(string text, SystemConsoleColor? color = null, int? padding = null)
952890
{
953891
if (color != null)
@@ -994,9 +932,7 @@ internal void TestDiscovered(
994932
TestProgressState asm = _assemblies[$"{assembly}|{targetFramework}|{architecture}|{executionId}"];
995933

996934
// TODO: add mode for discovered tests to the progress bar - jajares
997-
asm.PassedTests++;
998-
asm.TotalTests++;
999-
asm.DiscoveredTests.Add(new(displayName, uid));
935+
asm.DiscoverTest(displayName, uid);
1000936
_terminalWithProgress.UpdateWorker(asm.SlotIndex);
1001937
}
1002938

‎src/Cli/dotnet/Commands/Test/Terminal/TestProgressState.cs

Lines changed: 93 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ namespace Microsoft.DotNet.Cli.Commands.Test.Terminal;
55

66
internal sealed class TestProgressState(long id, string assembly, string? targetFramework, string? architecture, IStopwatch stopwatch)
77
{
8+
private readonly Dictionary<string, (int Passed, int Skipped, int Failed, string LastInstanceId)> _testUidToResults = new();
9+
810
public string Assembly { get; } = assembly;
911

1012
public string AssemblyName { get; } = Path.GetFileName(assembly)!;
@@ -15,19 +17,15 @@ internal sealed class TestProgressState(long id, string assembly, string? target
1517

1618
public IStopwatch Stopwatch { get; } = stopwatch;
1719

18-
public List<string> Attachments { get; } = [];
19-
20-
public List<IProgressMessage> Messages { get; } = [];
20+
public int FailedTests { get; private set; }
2121

22-
public int FailedTests { get; internal set; }
22+
public int PassedTests { get; private set; }
2323

24-
public int PassedTests { get; internal set; }
24+
public int SkippedTests { get; private set; }
2525

26-
public int SkippedTests { get; internal set; }
26+
public int TotalTests => PassedTests + SkippedTests + FailedTests;
2727

28-
public int TotalTests { get; internal set; }
29-
30-
public int RetriedFailedTests { get; internal set; }
28+
public int RetriedFailedTests { get; private set; }
3129

3230
public TestNodeResultsState? TestNodeResultsState { get; internal set; }
3331

@@ -38,20 +36,99 @@ internal sealed class TestProgressState(long id, string assembly, string? target
3836
public long Version { get; internal set; }
3937

4038
public List<(string? DisplayName, string? UID)> DiscoveredTests { get; internal set; } = [];
39+
4140
public int? ExitCode { get; internal set; }
41+
4242
public bool Success { get; internal set; }
4343

44-
public List<string> Tries { get; } = [];
44+
public int TryCount { get; internal set; }
45+
4546
public HashSet<string> FlakyTests { get; } = [];
4647

47-
internal void AddError(string text)
48-
=> Messages.Add(new ErrorMessage(text));
48+
public void ReportPassingTest(string testNodeUid, string instanceId)
49+
{
50+
if (_testUidToResults.TryGetValue(testNodeUid, out var value))
51+
{
52+
if (value.LastInstanceId == instanceId)
53+
{
54+
// We are getting a test result for the same instance id.
55+
value.Passed++;
56+
_testUidToResults[testNodeUid] = value;
57+
}
58+
else
59+
{
60+
// We are getting a test result for a different instance id.
61+
// This means that the test was retried.
62+
// We discard the results from the previous instance id
63+
RetriedFailedTests++;
64+
PassedTests -= value.Passed;
65+
SkippedTests -= value.Skipped;
66+
FailedTests -= value.Failed;
67+
_testUidToResults[testNodeUid] = (Passed: 1, Skipped: 0, Failed: 0, LastInstanceId: instanceId);
68+
}
69+
}
70+
else
71+
{
72+
// This is the first time we see this test node.
73+
_testUidToResults.Add(testNodeUid, (Passed: 1, Skipped: 0, Failed: 0, LastInstanceId: instanceId));
74+
}
75+
76+
PassedTests++;
77+
}
78+
79+
public void ReportSkippedTest(string testNodeUid, string instanceId)
80+
{
81+
if (_testUidToResults.TryGetValue(testNodeUid, out var value))
82+
{
83+
if (value.LastInstanceId == instanceId)
84+
{
85+
value.Skipped++;
86+
_testUidToResults[testNodeUid] = value;
87+
}
88+
else
89+
{
90+
PassedTests -= value.Passed;
91+
SkippedTests -= value.Skipped;
92+
FailedTests -= value.Failed;
93+
_testUidToResults[testNodeUid] = (Passed: 0, Skipped: 1, Failed: 0, LastInstanceId: instanceId);
94+
}
95+
}
96+
else
97+
{
98+
_testUidToResults.Add(testNodeUid, (Passed: 0, Skipped: 1, Failed: 0, LastInstanceId: instanceId));
99+
}
100+
101+
SkippedTests++;
102+
}
49103

50-
internal void AddWarning(string text)
51-
=> Messages.Add(new WarningMessage(text));
104+
public void ReportFailedTest(string testNodeUid, string instanceId)
105+
{
106+
if (_testUidToResults.TryGetValue(testNodeUid, out var value))
107+
{
108+
if (value.LastInstanceId == instanceId)
109+
{
110+
value.Failed++;
111+
_testUidToResults[testNodeUid] = value;
112+
}
113+
else
114+
{
115+
PassedTests -= value.Passed;
116+
SkippedTests -= value.Skipped;
117+
FailedTests -= value.Failed;
118+
_testUidToResults[testNodeUid] = (Passed: 0, Skipped: 0, Failed: 1, LastInstanceId: instanceId);
119+
}
120+
}
121+
else
122+
{
123+
_testUidToResults.Add(testNodeUid, (Passed: 0, Skipped: 0, Failed: 1, LastInstanceId: instanceId));
124+
}
125+
126+
FailedTests++;
127+
}
52128

53-
internal void ClearAllMessages()
129+
public void DiscoverTest(string displayName, string uid)
54130
{
55-
Messages.Clear();
131+
PassedTests++;
132+
DiscoveredTests.Add(new(displayName, uid));
56133
}
57134
}

‎src/Cli/dotnet/Commands/Test/TestApplicationEventHandlers.cs

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,19 +8,26 @@ namespace Microsoft.DotNet.Cli.Commands.Test;
88

99
internal sealed class TestApplicationsEventHandlers(TerminalTestReporter output) : IDisposable
1010
{
11-
private readonly ConcurrentDictionary<TestApplication, (string ModulePath, string TargetFramework, string Architecture, string ExecutionId, string InstanceId)> _executions = new();
11+
private readonly ConcurrentDictionary<TestApplication, (string ModulePath, string TargetFramework, string Architecture, string ExecutionId)> _executions = new();
1212
private readonly TerminalTestReporter _output = output;
1313

1414
public void OnHandshakeReceived(object sender, HandshakeArgs args)
1515
{
16-
var testApplication = (TestApplication)sender;
17-
var executionId = args.Handshake.Properties[HandshakeMessagePropertyNames.ExecutionId];
18-
var instanceId = args.Handshake.Properties[HandshakeMessagePropertyNames.InstanceId];
19-
var arch = args.Handshake.Properties[HandshakeMessagePropertyNames.Architecture]?.ToLower();
20-
var tfm = TargetFrameworkParser.GetShortTargetFramework(args.Handshake.Properties[HandshakeMessagePropertyNames.Framework]);
21-
(string ModulePath, string TargetFramework, string Architecture, string ExecutionId, string InstanceId) appInfo = new(testApplication.Module.RunProperties.RunCommand, tfm, arch, executionId, instanceId);
22-
_executions[testApplication] = appInfo;
23-
_output.AssemblyRunStarted(appInfo.ModulePath, appInfo.TargetFramework, appInfo.Architecture, appInfo.ExecutionId, appInfo.InstanceId);
16+
var hostType = args.Handshake.Properties[HandshakeMessagePropertyNames.HostType];
17+
// https://github.com/microsoft/testfx/blob/2a9a353ec2bb4ce403f72e8ba1f29e01e7cf1fd4/src/Platform/Microsoft.Testing.Platform/Hosts/CommonTestHost.cs#L87-L97
18+
if (hostType == "TestHost")
19+
{
20+
// AssemblyRunStarted counts "retry count", and writes to terminal "(Try <number-of-try>) Running tests from <assembly>"
21+
// So, we want to call it only for test host, and not for test host controller (or orchestrator, if in future it will handshake as well)
22+
// Calling it for both test host and test host controllers means we will count retries incorrectly, and will messages twice.
23+
var testApplication = (TestApplication)sender;
24+
var executionId = args.Handshake.Properties[HandshakeMessagePropertyNames.ExecutionId];
25+
var arch = args.Handshake.Properties[HandshakeMessagePropertyNames.Architecture]?.ToLower();
26+
var tfm = TargetFrameworkParser.GetShortTargetFramework(args.Handshake.Properties[HandshakeMessagePropertyNames.Framework]);
27+
(string ModulePath, string TargetFramework, string Architecture, string ExecutionId) appInfo = new(testApplication.Module.RunProperties.RunCommand, tfm, arch, executionId);
28+
_executions[testApplication] = appInfo;
29+
_output.AssemblyRunStarted(appInfo.ModulePath, appInfo.TargetFramework, appInfo.Architecture, appInfo.ExecutionId);
30+
}
2431

2532
LogHandshake(args);
2633
}
@@ -57,6 +64,11 @@ public void OnDiscoveredTestsReceived(object sender, DiscoveredTestEventArgs arg
5764

5865
public void OnTestResultsReceived(object sender, TestResultEventArgs args)
5966
{
67+
// TODO: If we got some results for ExecutionId1 and InstanceId1
68+
// Then we started getting ExecutionId1 and InstanceId2,
69+
// Then we started getting ExecutionId1 and InstanceId1 again.
70+
// Should we discard the last result from ExecutionId1 and InstanceId1 completely?
71+
// Or is it considered a violation of the protocol and should never happen? (in that case maybe we should throw?)
6072
var testApp = (TestApplication)sender;
6173
var appInfo = _executions[testApp];
6274

‎test/TestAssets/TestProjects/TestAppSimpleWithRetry/TestAppSimpleWithRetry.csproj

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), testAsset.props))\testAsset.props" />
3+
4+
<PropertyGroup>
5+
<TargetFramework>$(CurrentTargetFramework)</TargetFramework>
6+
<OutputType>Exe</OutputType>
7+
<!-- Use of crash dump is to ensure we are testing the UX in case a test host controller exists -->
8+
<TestingPlatformCommandLineArguments>$(TestingPlatformCommandLineArguments) --retry-failed-tests 3 --crashdump</TestingPlatformCommandLineArguments>
9+
<EnableMSTestRunner>true</EnableMSTestRunner>
10+
</PropertyGroup>
11+
12+
<ItemGroup>
13+
<PackageReference Include="MSTest" Version="$(MSTestVersion)" />
14+
<PackageReference Include="Microsoft.Testing.Extensions.Retry" Version="$(MicrosoftTestingPlatformVersion)" />
15+
<PackageReference Include="Microsoft.Testing.Extensions.CrashDump" Version="$(MicrosoftTestingPlatformVersion)" />
16+
</ItemGroup>
17+
</Project>

‎test/TestAssets/TestProjects/TestAppSimpleWithRetry/TestClass1.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System;
5+
using System.Linq;
6+
using Microsoft.VisualStudio.TestTools.UnitTesting;
7+
8+
[TestClass]
9+
public class TestClass1
10+
{
11+
[TestMethod]
12+
public void TestMethod1()
13+
{
14+
if (Environment.GetCommandLineArgs().Any(arg => arg.Contains("Retries") && !arg.EndsWith("2")))
15+
{
16+
Assert.Fail("Failing...");
17+
}
18+
}
19+
}

‎test/TestAssets/TestProjects/TestAppSimpleWithRetry/dotnet.config

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
[dotnet.test.runner]
2+
name= "Microsoft.Testing.Platform"

0 commit comments

Comments
 (0)