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.

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

Fixed the normal verbosity level to not log the full information for non-failed tests #1396

Merged
merged 10 commits into from
Feb 1, 2018
Merged

Fixed the normal verbosity level to not log the full information for non-failed tests #1396

merged 10 commits into from
Feb 1, 2018

Conversation

cltshivash
Copy link
Contributor

Description

Updated the normal verbosity behavior to not output the detailed information for a test other than failed test. Added a detailed verbosity option to do the same

@@ -334,7 +394,7 @@ public void TestResultHandlerShouldWriteToConsoleShouldShowPassedTestsForNormalV
}

[TestMethod]
public void TestResultHandlerShouldShowStdOutMsgOfPassedTestIfVerbosityIsNormal()
public void TestResultHandlerShouldShowNotStdOutMsgOfPassedTestIfVerbosityIsNormal()
Copy link
Contributor

Choose a reason for hiding this comment

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

TestResultHandlerShouldShowNotStdOutMsgOfPassedTestIfVerbosityIsNormal [](start = 20, length = 70)

Nit: ShouldNotShow
please change the name for test following this one as well.

@@ -142,9 +142,9 @@ public void Initialize(TestLoggerEvents events, string testRunDirectory)
}

// Register for the events.
events.TestRunMessage += this.TestMessageHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

We have been following this convention to use 'this' with members, makes it readable, any specific reason why we are removing it ?

@@ -337,7 +337,6 @@ private void TestMessageHandler(object sender, TestRunMessageEventArgs e)
Output.Warning(ConsoleLogger.AppendPrefix, e.Message);
break;
case TestMessageLevel.Error:
this.testOutcome = TestOutcome.Failed;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is removed? This impact behavior. If the adapter/platform send any error message after this change we not failing test run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix this and add an UT. Currently didn't see any UT fail.. Also why is the failure of a run decided at a logger level ?

break;
}

var output = string.Format(CultureInfo.CurrentCulture, CommandLineResources.PassedTestIndicator, testDisplayName);
Copy link
Contributor

Choose a reason for hiding this comment

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

On Minimal level the passed test name should not get displayed. Update the test to catch this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -453,16 +481,16 @@ private void TestRunCompleteHandler(object sender, TestRunCompleteEventArgs e)
{
Output.Error(false, CommandLineResources.TestRunAborted);
}
else if (this.testOutcome == TestOutcome.Failed && this.testsTotal > 0)
else if (testsFailed > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

testsFailed [](start = 21, length = 11)

We should revert this one.
We could have a passing tests and then we some failure occurred.
TestOutcome is set to failed in case some error occurs during the run, it is not necessary that we would have failing test.

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. Will fix this. Didnt see any UT failing though.. will add one.

@cltshivash cltshivash merged commit 46ccd50 into microsoft:master Feb 1, 2018
@cltshivash cltshivash deleted the users/shivash/verbosityfix branch February 1, 2018 04:23
@@ -61,7 +61,8 @@ internal enum Verbosity
{
Quiet,
Minimal,
Normal
Normal,
Detailed
Copy link
Contributor

Choose a reason for hiding this comment

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

Add detailed to help for customer to aware this option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants