Skip to content

Conversation

@harshjain2
Copy link
Contributor

Added Level as prefix for loggers.

@harshjain2 harshjain2 requested review from codito and smadala August 1, 2017 11:35
{
this.mockOutput.Object.Error("HelloError", null);
this.mockOutput.Verify(o => o.WriteLine("HelloError", OutputLevel.Error), Times.Once());
this.mockOutput.Verify(o => o.WriteLine("Error: HelloError", OutputLevel.Error), Times.Once());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use Resources.CommandLineError here?

SetColorForAction(ConsoleColor.Red, () =>
{
Output(output, OutputLevel.Error, DefaultFormat, format, args);
Output(output, OutputLevel.Error, Resources.CommandLineError, format, args);
Copy link
Contributor

@codito codito Aug 1, 2017

Choose a reason for hiding this comment

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

Since this is a backward compatibility fix (it hides errors for the user, and that's the reason it was removed for newer experience), suggest to use a console logger parameter for this. E.g. /logger:console;prefix=true. This will keep the default behavior strict.

@harshjain2
Copy link
Contributor Author

@dotnet-bot test this please.

{
/// <summary>
/// Bool to decide whether Verbose level should be added as prefix or not in log messages.
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if an Extension methods unit should contain state or configuration.

var prefixExists = parameters.TryGetValue(ConsoleLogger.PrefixParam, out string prefix);
if (prefixExists)
{
bool.TryParse(prefix, out OutputExtensions.AppendPrefix);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this returns false, we don't need to log?


[TestMethod]
public void OutputErrorForSimpleMessageShouldOutputTheMessageStringWithPrefixIfSet()
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to validate other scenarios? Can it be a data driven test?

/// <summary>
/// Bool to decide whether Verbose level should be added as prefix or not in log messages.
/// </summary>
internal static bool AppendPrefix;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if an extension methods class should have state.

@smadala
Copy link
Contributor

smadala commented Aug 10, 2017

Having prefix=true always makes finding warning and error messages easy, especially output is redirect to files in CI/CD scenarios. Currently user can identify the level of message by color of message in console which we loss once it redirect to file.

Update help for vstest.console.exe and dotnet test, update doc in vstest-docs.

@harshjain2 harshjain2 requested a review from Faizan2304 August 11, 2017 12:50
Copy link
Contributor

@smadala smadala left a comment

Choose a reason for hiding this comment

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

Add doc in vstest-docs.

@harshjain2 harshjain2 merged commit 4b3243e into microsoft:master Aug 31, 2017
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.

5 participants