Logger support in run settings #1382

abhishekkumawat23 · 2018-01-22T06:50:18Z

Description

Contains following changes:

Loggers information from RunSettings
Logger support enabled for design mode

RFC PR

microsoft/vstest-docs#111

…gerInRunSettingsU1 # Conflicts: # test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

…mawat23/vstest into loggerInRunSettingsU1

cltshivash · 2018-01-22T10:26:32Z

src/vstest.console/TestPlatformHelpers/TestRequestManager.cs

@@ -210,7 +196,7 @@ public bool DiscoverTests(DiscoveryRequestPayload discoveryPayload, ITestDiscove
                    ex is SettingsException ||
                    ex is InvalidOperationException)
                {
-                    LoggerUtilities.RaiseTestRunError(testLoggerManager, null, ex);
+                    ConsoleLogger.RaiseTestRunError(null, ex);


ConsoleLogger.RaiseTestRunError(null, ex); [](start = 20, length = 42)

nitpick: can we move this closer to the logger initialization code alone..?

Verified that logger initialization is happening within this block only.

cltshivash · 2018-01-22T10:36:46Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

@@ -296,6 +296,13 @@ public static void UpdateTargetPlatform(XmlDocument runSettingsDocument, string
            AddNodeIfNotPresent<string>(runSettingsDocument, TargetPlatformNodePath, TargetPlatformNodeName, platform, overwrite);
        }

+        public static void UpdateLoggerRunSettings(XmlDocument xmlDocument, string loggerRunSettings)


remove if not reqd

cltshivash · 2018-01-22T10:46:26Z

src/vstest.console/TestPlatformHelpers/TestRequestManager.cs

+            var loggerRunSettings = XmlRunSettingsUtilities.GetLoggerRunSettings(runsettingsXml) ?? new LoggerRunSettings();
+            var existingLoggerIndex = LoggerUtilities.GetExistingLoggerIndex(ConsoleLogger.FriendlyName, new Uri(ConsoleLogger.ExtensionUri),
+                loggerRunSettings.LoggerSettingsList);
+


nitpick: loggerRunSettings.GetExistingLoggerIndex

singhsarab · 2018-01-29T09:00:10Z

src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/ITestEngine.cs

@@ -37,5 +37,7 @@ public interface ITestEngine
        /// </summary>
        /// <returns>ITestExtensionManager object that helps with extensibility</returns>
        ITestExtensionManager GetExtensionManager();
+
+        ITestLoggerManager GetLoggerManager(IRequestData requestData);


singhsarab · 2018-01-29T09:01:26Z

src/Microsoft.TestPlatform.Common/Interfaces/Engine/ClientProtocol/ITestLoggerManager.cs

+        /// <summary>
+        /// Dispose logger manager.
+        /// </summary>
+        void Dispose();


IDisposable instead.

singhsarab · 2018-01-29T09:06:56Z

src/Microsoft.TestPlatform.ObjectModel/Constants.cs

+        /// </summary>
+        public const string LoggerSettingName = "Logger";
+
+        /// <summary>


Aren't similar consts defined for datacollector already ?

singhsarab · 2018-01-29T09:08:52Z

src/Microsoft.TestPlatform.ObjectModel/Logger/LoggerSettings.cs

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+// ReSharper disable CheckNamespace


Nit: Remove

singhsarab · 2018-01-29T09:29:45Z

src/vstest.console/Processors/EnableLoggerArgumentProcessor.cs

+                    };
+                }
+
+                // Converting logger cosole params to Configuration element


Nit: console

singhsarab · 2018-01-29T09:33:16Z

src/vstest.console/Processors/EnableLoggerArgumentProcessor.cs

@@ -199,6 +243,18 @@ private static void HandleInvalidArgument(string argument)
                    argument));
        }

+        /// <summary>


In general, prefer InheritDoc instead of generic docs

singhsarab · 2018-01-29T09:35:49Z

test/Microsoft.TestPlatform.Extensions.BlameDataCollector.UnitTests/BlameLoggerTests.cs

-            this.testLoggerManager = new TestableTestLoggerManager();
-            this.testLoggerManager.AddLogger(this.blameLogger, BlameLogger.ExtensionUri, null);
-            this.testLoggerManager.EnableLogging();
+            // this.testLoggerManager = new TestableTestLoggerManager();


Enable these please.

singhsarab · 2018-01-29T09:37:26Z

src/Microsoft.TestPlatform.Common/Resources/Resources.resx

@@ -166,10 +166,10 @@
    <value>Error: Invalid operator '{0}'</value>
  </data>
  <data name="LoggerInitializationError" xml:space="preserve">
-    <value>Exception occurred while initializing logger with URI '{0}'.  The logger will not be used.  Exception: {1}</value>
+    <value>Exception occurred while initializing logger with {0}: '{1}'.  The logger will not be used.  Exception: {2}</value>


Run build.cmd, then add the other lang changes as well.

singhsarab · 2018-01-29T11:34:33Z

src/vstest.console/Processors/Utilities/LoggerUtilities.cs

+        /// <param name="friendlyName"></param>
+        /// <param name="uri"></param>
+        /// <param name="loggerSettingsList"></param>
+        /// <returns></returns>


Review docs

acesiddhu · 2018-01-29T11:44:51Z

test/vstest.console.UnitTests/Internal/ConsoleLoggerTests.cs

-            this.mockOutput.Verify(o => o.WriteLine("Error123", OutputLevel.Error), Times.Once());
+            // this.mockOutput.Verify(o => o.WriteLine("Informational123", OutputLevel.Information), Times.Once());
+            // this.mockOutput.Verify(o => o.WriteLine("Warning123", OutputLevel.Warning), Times.Once());
+            // this.mockOutput.Verify(o => o.WriteLine("Error123", OutputLevel.Error), Times.Once());


why remove this?

acesiddhu · 2018-01-29T11:44:57Z

test/vstest.console.UnitTests/Internal/ConsoleLoggerTests.cs

+            // Assert.ThrowsException<NullReferenceException>(() =>
+            // {
+                // testRunRequest.Raise(m => m.OnRunStatsChange += null, eventarg);
+            // });
        }


acesiddhu · 2018-01-29T11:45:03Z

test/vstest.console.UnitTests/Internal/ConsoleLoggerTests.cs

-            this.mockOutput.Verify(o => o.WriteLine(CommandLineResources.StdOutMessagesBanner, OutputLevel.Information), Times.Once());
-            this.mockOutput.Verify(o => o.WriteLine(" " + message, OutputLevel.Information), Times.Once());
+            // this.mockOutput.Verify(o => o.WriteLine(CommandLineResources.StdOutMessagesBanner, OutputLevel.Information), Times.Once());
+            // this.mockOutput.Verify(o => o.WriteLine(" " + message, OutputLevel.Information), Times.Once());


singhsarab · 2018-01-29T11:48:04Z

src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs

+        /// <returns>ITestLoggerManager object that helps with logger extensibility.</returns>
+        public ITestLoggerManager GetLoggerManager(IRequestData requestData)
+        {
+            return new TestLoggerManager(


validate the data in IRequestData is getting logged properly, even after the request is complete.

singhsarab

with suggestions

acesiddhu · 2018-01-29T11:56:57Z

src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

@@ -296,6 +296,13 @@ public static void UpdateTargetPlatform(XmlDocument runSettingsDocument, string
            AddNodeIfNotPresent<string>(runSettingsDocument, TargetPlatformNodePath, TargetPlatformNodeName, platform, overwrite);
        }

+        public static void UpdateLoggerRunSettings(XmlDocument xmlDocument, string loggerRunSettings)
+        {
+            // TODO: No need for entry here. move to testrequestmanager.


remove if not required

acesiddhu · 2018-01-29T12:05:00Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/LoggerNameValueConfigurationManager.cs

+        {
+            if (configurationElement == null)
+            {
+                // There is no configuration


not required here

acesiddhu · 2018-01-29T12:05:45Z

src/Microsoft.TestPlatform.CrossPlatEngine/Client/TestLoggerManager.cs

-            // Add the logger and if it is a non-existent logger, throw.
+            var loggerType =
+                assembly?.GetTypes()
+                    .FirstOrDefault(x => x.AssemblyQualifiedName.Equals(assemblyQualifiedName));


put inside try

acesiddhu · 2018-01-29T12:06:13Z

src/Microsoft.TestPlatform.Common/Utilities/RunSettingsProviderExtensions.cs

@@ -45,6 +45,12 @@ public static void AddDefaultRunSettings(this IRunSettingsProvider runSettingsPr
            runSettingsProvider.UpdateRunSettings(runSettingsXml);
        }

+        public static void UpdateRunSettingsXmlDocumentInnerXml(XmlDocument xmlDocument, string key, string data)


remove if not required

its required.

acesiddhu

abhishkk · 2018-01-30T11:47:29Z

Validated all scenarios and resolved pr comments.
Scenarios validated:

Logger is respected from runsettings.
If same logger multiple times, first is respected.
If same logger is present both in command line and runsettings, command line is respected.
If design mode is false, console logger is added automatically.
if design mode is false and console logger is present in runsettings, existing console logger params are used but assemblyQualifiedName is updated.
Console logger params like verbosity is working fine.
Among uri, assemblyQualifiedName and friendlyName, priority order is assemblyQualifiedName > uri > friendlyName
Exception thrown on invalid xml, invalid attributes, invalid node in runsettings.
Exception thrown if unable to initialize logger.
EqtTrace, Logs are logged for appropriate scenarios.

abhishkk added 11 commits January 18, 2018 22:03

Initial commit

4835d8c

Initial commit 2

e3fed0b

Initial commit 3

76e2fae

Initial commit 4

4278003

Clean up

9837f37

Merge branch 'master' of https://github.com/Microsoft/vstest into log…

78cff04

…gerInRunSettingsU1 # Conflicts: # test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs

fixes for usings

ee0dd06

test commented temporary

a6f671f

console logger fix

8c70207

Merge branch 'loggerInRunSettingsU1' of https://github.com/abhishekku…

1eb51e9

…mawat23/vstest into loggerInRunSettingsU1

test fix

e6f9855

cltshivash reviewed Jan 22, 2018

View reviewed changes

abhishkk changed the title ~~[In Progress] Logger in run settings u1~~ Logger support in run settings Jan 29, 2018

abhishkk requested a review from singhsarab January 29, 2018 07:01

Merge branch 'master' into loggerInRunSettingsU1

bd35abb

singhsarab reviewed Jan 29, 2018

View reviewed changes

acesiddhu reviewed Jan 29, 2018

View reviewed changes

singhsarab reviewed Jan 29, 2018

View reviewed changes

singhsarab approved these changes Jan 29, 2018

View reviewed changes

acesiddhu reviewed Jan 29, 2018

View reviewed changes

acesiddhu approved these changes Jan 29, 2018

View reviewed changes

abhishkk added 3 commits January 29, 2018 19:24

Merge branch 'master' into loggerInRunSettingsU1

473e44b

PR comments

183e92d

test fix

38d3334

singhsarab merged commit b0fc6c9 into microsoft:master Jan 30, 2018

smadala mentioned this pull request Mar 19, 2018

Enabling of TRX Logger/Any loggers is not possible VIA runsettings/testsettings #1262

Closed

smadala mentioned this pull request Aug 9, 2018

Console logger always enabled, even if another logger is specified #1046

Closed

Logger support in run settings #1382

Logger support in run settings #1382

Conversation

abhishekkumawat23 commented Jan 22, 2018 • edited by abhishkk Loading

Description

RFC PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhsarab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acesiddhu left a comment

Choose a reason for hiding this comment

abhishkk commented Jan 30, 2018 • edited Loading

abhishekkumawat23 commented Jan 22, 2018 •

edited by abhishkk

Loading

abhishkk commented Jan 30, 2018 •

edited

Loading