Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove duplicate counting of test results in Consolelogger #2267

Merged
merged 4 commits into from
Jan 2, 2020
Merged

Remove duplicate counting of test results in Consolelogger #2267

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4e96249
Remove duplicate counting of test result outcome in Consolelogger
Dec 5, 2019
c473a11
Fixed order tests accptance test
Dec 10, 2019
4d874f7
Remove comment and ignore out value
nohwnd Jan 2, 2020
b08f32a
Compare to empty Guid
nohwnd Jan 2, 2020
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
91 changes: 80 additions & 11 deletions src/vstest.console/Internal/ConsoleLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Internal
{
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Diagnostics;
Expand Down Expand Up @@ -85,6 +86,16 @@ internal class ConsoleLogger : ITestLoggerWithParameters
/// </summary>
public const string NoProgressParam = "noprogress";

/// <summary>
/// Property Id storing the ParentExecutionId.
/// </summary>
public const string ParentExecutionIdPropertyIdentifier = "ParentExecId";

/// <summary>
/// Property Id storing the ExecutionId.
/// </summary>
public const string ExecutionIdPropertyIdentifier = "ExecutionId";

#endregion

internal enum Verbosity
Expand All @@ -107,11 +118,8 @@ internal enum Verbosity
private Verbosity verbosityLevel = Verbosity.Minimal;
#endif

private int testsTotal = 0;
private int testsPassed = 0;
private int testsFailed = 0;
private int testsSkipped = 0;
private bool testRunHasErrorMessages = false;
private ConcurrentDictionary<Guid, TestOutcome> leafExecutionIdAndTestOutcomePairDictionary = new ConcurrentDictionary<Guid, TestOutcome>();

#endregion

Expand Down Expand Up @@ -371,6 +379,39 @@ private static void DisplayFullInformation(TestResult result)
}
}

/// <summary>
/// Returns the parent Execution id of given test result.
/// </summary>
/// <param name="testResult"></param>
/// <returns></returns>
private Guid GetParentExecutionId(TestResult testResult)
hvinett marked this conversation as resolved.
Show resolved Hide resolved
{
var parentExecutionIdProperty = testResult.Properties.FirstOrDefault(property =>
property.Id.Equals(ParentExecutionIdPropertyIdentifier));
return parentExecutionIdProperty == null
nohwnd marked this conversation as resolved.
Show resolved Hide resolved
? Guid.Empty
: testResult.GetPropertyValue(parentExecutionIdProperty, Guid.Empty);
}

/// <summary>
/// Returns execution id of given test result
/// </summary>
/// <param name="testResult"></param>
/// <returns></returns>
private Guid GetExecutionId(TestResult testResult)
{
var executionIdProperty = testResult.Properties.FirstOrDefault(property =>
property.Id.Equals(ExecutionIdPropertyIdentifier));
var executionId = Guid.Empty;

nohwnd marked this conversation as resolved.
Show resolved Hide resolved
if (executionIdProperty != null)
{
executionId = testResult.GetPropertyValue(executionIdProperty, Guid.Empty);
}

return executionId.Equals(Guid.Empty) ? Guid.NewGuid() : executionId;
}

#endregion

#region Event Handlers
Expand Down Expand Up @@ -468,9 +509,6 @@ private void TestResultHandler(object sender, TestResultEventArgs e)
ValidateArg.NotNull<object>(sender, "sender");
ValidateArg.NotNull<TestResultEventArgs>(e, "e");

// Update the test count statistics based on the result of the test.
this.testsTotal++;

var testDisplayName = e.Result.DisplayName;

if (string.IsNullOrWhiteSpace(e.Result.DisplayName))
Expand All @@ -484,11 +522,20 @@ private void TestResultHandler(object sender, TestResultEventArgs e)
testDisplayName = string.Format("{0} [{1}]", testDisplayName, formattedDuration);
}

var executionId = GetExecutionId(e.Result);
var parentExecutionId = GetParentExecutionId(e.Result);

if (parentExecutionId != null && leafExecutionIdAndTestOutcomePairDictionary.ContainsKey(parentExecutionId))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hvinett GetParentExecutionId returns non-nullable guid, removing the nullability check, as well as replacing it with != Guid.Empty check passes all the tests in ConsoleLoggerTests what is the intent here?

Copy link
Contributor Author

@hvinett hvinett Jan 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept parentExecutionId != null, because .containskey() throws error if key is null. As it is not null we can check parentExecutionId != Guid.Empty to avoid check in dictionary. Is that fine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Let me quickly patch that, because I have the code open, and merge if that passes (it should, I tried it before).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

{
leafExecutionIdAndTestOutcomePairDictionary.TryRemove(parentExecutionId, out var value);
singhsarab marked this conversation as resolved.
Show resolved Hide resolved
}

leafExecutionIdAndTestOutcomePairDictionary.TryAdd(executionId, e.Result.Outcome);

switch (e.Result.Outcome)
{
case TestOutcome.Skipped:
{
this.testsSkipped++;
if (this.verbosityLevel == Verbosity.Quiet)
{
break;
Expand All @@ -512,7 +559,6 @@ private void TestResultHandler(object sender, TestResultEventArgs e)

case TestOutcome.Failed:
{
this.testsFailed++;
if (this.verbosityLevel == Verbosity.Quiet)
{
break;
Expand All @@ -533,7 +579,6 @@ private void TestResultHandler(object sender, TestResultEventArgs e)

case TestOutcome.Passed:
{
this.testsPassed++;
if (this.verbosityLevel == Verbosity.Normal || this.verbosityLevel == Verbosity.Detailed)
{
// Pause the progress indicator before displaying test result information
Expand Down Expand Up @@ -617,6 +662,11 @@ private string GetFormattedDurationString(TimeSpan duration)
/// </summary>
private void TestRunCompleteHandler(object sender, TestRunCompleteEventArgs e)
{
//System.Diagnostics.Debugger.Launch();
nohwnd marked this conversation as resolved.
Show resolved Hide resolved
var testsTotal = 0;
var testsPassed = 0;
var testsFailed = 0;
var testsSkipped = 0;
// Stop the progress indicator as we are about to print the summary
this.progressIndicator?.Stop();
Output.WriteLine(string.Empty, OutputLevel.Information);
Expand All @@ -636,6 +686,25 @@ private void TestRunCompleteHandler(object sender, TestRunCompleteEventArgs e)
}
}

foreach (KeyValuePair<Guid, TestOutcome> entry in leafExecutionIdAndTestOutcomePairDictionary)
{
testsTotal++;
switch (entry.Value)
{
case TestOutcome.Failed:
testsFailed++;
break;
case TestOutcome.Passed:
testsPassed++;
break;
case TestOutcome.Skipped:
testsSkipped++;
break;
default:
break;
}
}

if (e.IsCanceled)
{
Output.Error(false, CommandLineResources.TestRunCanceled);
Expand All @@ -644,7 +713,7 @@ private void TestRunCompleteHandler(object sender, TestRunCompleteEventArgs e)
{
Output.Error(false, CommandLineResources.TestRunAborted);
}
else if (this.testsFailed > 0 || this.testRunHasErrorMessages)
else if (testsFailed > 0 || this.testRunHasErrorMessages)
{
Output.Error(false, CommandLineResources.TestRunFailed);
}
Expand Down
2 changes: 1 addition & 1 deletion test/Microsoft.TestPlatform.AcceptanceTests/OrderedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public void OlderOrderedTestsShouldWorkFine(RunnerInfo runnerInfo)
this.ValidateFailedTests("FailingTest1");
this.ValidateSkippedTests("FailingTest2");
// Parent test result should fail as inner results contain failing test.
this.ValidateSummaryStatus(2, 2, 1);
this.ValidateSummaryStatus(2, 1, 1);
}
}
}
45 changes: 45 additions & 0 deletions test/vstest.console.UnitTests/Internal/ConsoleLoggerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,51 @@ public void AttachmentInformationShouldBeWrittenToConsoleIfAttachmentsArePresent
this.mockOutput.Verify(o => o.WriteLine(string.Format(CultureInfo.CurrentCulture, CommandLineResources.AttachmentOutputFormat, uriDataAttachment1.Uri.LocalPath), OutputLevel.Information), Times.Once());
}

[TestMethod]
public void ResultsInHeirarchichalOrderShouldReportCorrectCount()
{
var loggerEvents = new InternalTestLoggerEvents(TestSessionMessageLogger.Instance);
loggerEvents.EnableEvents();
var parameters = new Dictionary<string, string>();
parameters.Add("verbosity", "normal");
this.consoleLogger.Initialize(loggerEvents, parameters);

TestCase testCase1 = CreateTestCase("TestCase1");
TestCase testCase2 = CreateTestCase("TestCase2");
nohwnd marked this conversation as resolved.
Show resolved Hide resolved
TestCase testCase3 = CreateTestCase("TestCase3");

Guid parentExecutionId = Guid.NewGuid();
TestProperty ParentExecIdProperty = TestProperty.Register("ParentExecId", "ParentExecId", typeof(Guid), TestPropertyAttributes.Hidden, typeof(ObjectModel.TestResult));
TestProperty ExecutionIdProperty = TestProperty.Register("ExecutionId", "ExecutionId", typeof(Guid), TestPropertyAttributes.Hidden, typeof(ObjectModel.TestResult));
TestProperty TestTypeProperty = TestProperty.Register("TestType", "TestType" , typeof(Guid), TestPropertyAttributes.Hidden, typeof(ObjectModel.TestResult));

var result1 = new ObjectModel.TestResult(testCase1) { Outcome = TestOutcome.Failed };
result1.SetPropertyValue(ExecutionIdProperty, parentExecutionId);

var result2 = new ObjectModel.TestResult(testCase2) { Outcome = TestOutcome.Passed};
result2.SetPropertyValue(ExecutionIdProperty, Guid.NewGuid());
result2.SetPropertyValue(ParentExecIdProperty, parentExecutionId);

var result3 = new ObjectModel.TestResult(testCase3) { Outcome = TestOutcome.Failed };
result3.SetPropertyValue(ExecutionIdProperty, Guid.NewGuid());
result3.SetPropertyValue(ParentExecIdProperty, parentExecutionId);

loggerEvents.RaiseTestResult(new TestResultEventArgs(result1));
loggerEvents.RaiseTestResult(new TestResultEventArgs(result2));
loggerEvents.RaiseTestResult(new TestResultEventArgs(result3));

loggerEvents.CompleteTestRun(null, false, false, null, null, new TimeSpan(1, 0, 0, 0));

this.mockOutput.Verify(o => o.WriteLine(string.Format(CultureInfo.CurrentCulture, CommandLineResources.TestRunSummaryFailedTests, 1), OutputLevel.Information), Times.Once());
this.mockOutput.Verify(o => o.WriteLine(string.Format(CultureInfo.CurrentCulture, CommandLineResources.TestRunSummaryPassedTests, 1), OutputLevel.Information), Times.Once());
this.mockOutput.Verify(o => o.WriteLine(string.Format(CultureInfo.CurrentCulture, CommandLineResources.TestRunSummaryTotalTests, 2), OutputLevel.Information), Times.Once());
}

private TestCase CreateTestCase(string testCaseName)
{
return new TestCase(testCaseName, new Uri("some://uri"), "DummySourceFileName");
}

private void Setup()
{
this.mockRequestData = new Mock<IRequestData>();
Expand Down