Add enhancement: trx logger can take logfile parameter #282

smadala · 2016-12-14T12:51:29Z

--loggerOutput option #243 Trx logger takes logfile parameter, ex:/logger:"trx;logfile=C:\pathto\test_results.trx"
Added tests for same

Edit:

logfilename parameter takes name of the filename(not path).
file will be create in test results directory.
ex: /logger:"trx;logfilename=test_results.trx"

smadala · 2016-12-14T12:55:09Z

src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs

        #endregion

        /// <summary>
        /// Gets the directory under which trx file should be saved.
        /// </summary>
        [SuppressMessage("StyleCop.CSharp.DocumentationRules", "SA1650:ElementDocumentationMustBeSpelledCorrectly", Justification = "Reviewed. Suppression is OK here.")]
-        public static string TrxFileDirectory
+        public static string TrxFileDirectory //TODO: Rename to TestResultsDirectory??


TestResultsDirectory makes sense. We will save test results attachments to TestResultsDirectory. After adding logfile parameter, TestResultsDirectory won't be always TrxFileDirectory.

codito · 2016-12-14T14:50:01Z

@smadala do we need an e2e test for this? Or better our test.cmd can probably dogfood this :-)

codito · 2016-12-15T04:29:36Z

src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs

-        /// <param name="testRunDirectory">Test Run Directory</param>
-        public void Initialize(TestLoggerEvents events, string testRunDirectory)
+        /// <inheritdoc/>
+        public void Initialize(TestLoggerEvents events, string testResultsDirPath)


When will this method get called?

There is overloaded method, which calls it.

codito · 2016-12-15T04:38:38Z

src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs

+        {
+            if (this.parametersDictionary != null && this.parametersDictionary.ContainsKey(LogFileParameterKey) && !string.IsNullOrWhiteSpace(this.parametersDictionary[LogFileParameterKey]))
+            {
+                this.SetTrxFilePathFromParameters();


Should we inline these methods. Some common code like creating a results directory could get combined.

codito · 2016-12-15T04:41:19Z

src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs

@@ -454,6 +450,41 @@ private void HandleSkippedTest(ObjectModel.TestResult rsTestResult)
            this.AddRunLevelInformationalMessage(message);
        }

+        private void DeriveTrxFilePath()
+        {
+            if (this.parametersDictionary != null && this.parametersDictionary.ContainsKey(LogFileParameterKey) && !string.IsNullOrWhiteSpace(this.parametersDictionary[LogFileParameterKey]))


Consider using Dictionary.TryGetValue. Use the value obtained for further checks instead of multiple dictionary lookups.

make sense, made changes.

codito · 2016-12-15T04:42:15Z

src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs

+        private void SetDefaultTrxFilePath()
+        {
+            // save the trx file to testResultsDirectory
+            if (Directory.Exists(TrxFileDirectory) == false)


It is confusing to use TrxFileDirectory in one case and derive the directory in the other.

Renamed it to testResultsDirPath.

harshjain2 · 2016-12-15T08:05:18Z

src/Microsoft.TestPlatform.Extensions.TrxLogger/TrxLogger.cs

        #endregion

        #region Fields

+


Suggestion: Remove extra lines.

harshjain2 · 2016-12-15T08:55:48Z

@smadala Make sure to verify that subsequent test runs with same value of logfile should override the previous trx logger file.

codito · 2016-12-16T11:55:37Z

We should fix #244 (for ubuntu) in this PR if possible.

* test.cmd -parallel for netcoreapp1.0 fails, raised issue #292

…stest into trx-logger-enhancement

smadala · 2016-12-20T14:45:45Z

@harshjain2 Tested the scenario. It is overwriting.

… logger

codito · 2016-12-22T04:58:36Z

src/vstest.console/Resources/Resources.resx

@@ -234,7 +234,7 @@
  <data name="EnableLoggersArgumentHelp" xml:space="preserve">
    <value>--logger|/logger:&lt;Logger Uri/FriendlyName&gt;
      Specify a logger for test results.  For example, to log results into a 
-      Visual Studio Test Results File (TRX) use  /logger:trx.
+      Visual Studio Test Results File (TRX) use  /logger:trx [;Logfile=&lt;path to log file&gt; [;Overwrite=&lt;Defaults to "true"&gt;]]


Should we be specific? LogFile can be absolute path or relative path? What does overwrite do?

Faizan2304 · 2016-12-23T09:27:41Z

test/Microsoft.TestPlatform.AcceptanceTests/LoggerTests.cs

+            this.InvokeVsTest(arguments);
+
+            arguments = PrepareArguments(this.GetSampleTestAssembly(), this.GetTestAdapterPath(), string.Empty, this.FrameworkArgValue);
+            arguments = string.Concat(arguments, $" /logger:\"trx;LogFileName={trxFileName}\"");


I believe order should be reverse.

codito · 2016-12-23T10:20:40Z

scripts/test.ps1

@@ -40,6 +40,7 @@ $env:TP_TOOLS_DIR = Join-Path $env:TP_ROOT_DIR "tools"
 $env:TP_PACKAGES_DIR = Join-Path $env:TP_ROOT_DIR "packages"
 $env:TP_OUT_DIR = Join-Path $env:TP_ROOT_DIR "artifacts"

+


codito · 2016-12-23T10:25:01Z

src/vstest.console/Resources/Resources.resx

@@ -234,7 +234,9 @@
  <data name="EnableLoggersArgumentHelp" xml:space="preserve">
    <value>--logger|/logger:&lt;Logger Uri/FriendlyName&gt;
      Specify a logger for test results.  For example, to log results into a 
-      Visual Studio Test Results File (TRX) use  /logger:trx.
+      Visual Studio Test Results File (TRX) use  /logger:trx [;LogFileName=&lt;Defaults to unique file name&gt;]


May be just calling FileName suffices instead of LogFileName. @pvlakshm

codito · 2016-12-23T10:25:38Z

test/Microsoft.TestPlatform.Common.UnitTests/Logging/TestLoggerManagerTests.cs

+            TestLoggerManager.Instance.AddLogger(new Uri("test-logger-with-parameter://logger"), new Dictionary<string, string>());
+            Assert.IsNotNull(ValidLoggerWithParameters.parameters, "parameters not getting passed");
+            Assert.IsTrue(ValidLoggerWithParameters.parameters.ContainsKey(DefaultLoggerParameterNames.TestRunDirectory), $"{DefaultLoggerParameterNames.TestRunDirectory} not added to parameters");
+            Assert.IsFalse(string.IsNullOrWhiteSpace(ValidLoggerWithParameters.parameters[DefaultLoggerParameterNames.TestRunDirectory]), $"parameter {DefaultLoggerParameterNames.TestRunDirectory} should not be null, empty or whitespace");


codito · 2016-12-23T10:26:22Z

👍

…stest into trx-logger-enhancement

Add enhancement: trx logger can take custome logfile

aafc889

smadala requested a review from codito December 14, 2016 12:51

smadala commented Dec 14, 2016

View reviewed changes

smadala added 2 commits December 14, 2016 18:41

Nit fixes

6eb5db1

Merge branch 'master' into trx-logger-enhancement

9069495

codito suggested changes Dec 15, 2016

View reviewed changes

harshjain2 reviewed Dec 15, 2016

View reviewed changes

Refactor TrxLogger.cs add a test

f477901

smadala added 2 commits December 20, 2016 20:03

Dogfooding trx logger filename parameter feature

fe2111f

* test.cmd -parallel for netcoreapp1.0 fails, raised issue #292

Merge branch 'trx-logger-enhancement' of https://github.com/smadala/v…

6556ae9

…stest into trx-logger-enhancement

Add overwrite parameter and warning on overwrite, update help for trx…

7548482

… logger

codito reviewed Dec 22, 2016

View reviewed changes

codito approved these changes Dec 22, 2016

View reviewed changes

smadala added 3 commits December 22, 2016 19:59

Take LogFileName as parameter and remove overwrite parameter

5f24b04

Add acceptance test for trx logger

6f17ee2

Merge branch 'master' into trx-logger-enhancement

0c24356

Faizan2304 reviewed Dec 23, 2016

View reviewed changes

Faizan2304 approved these changes Dec 23, 2016

View reviewed changes

codito reviewed Dec 23, 2016

View reviewed changes

smadala and others added 4 commits December 23, 2016 19:23

Fix acceptance test and nit

893f36a

Merge branch 'trx-logger-enhancement' of https://github.com/smadala/v…

7843fbc

…stest into trx-logger-enhancement

nit fix

21d3f3e

Merge branch 'master' into trx-logger-enhancement

4651482

Merge branch 'master' into trx-logger-enhancement

1fb09a7

smadala merged commit fc1f7b9 into microsoft:master Dec 26, 2016

Saeras mentioned this pull request Dec 23, 2022

TrxLogger throws a NullReferenceException when using TrxLogger.Initialize(TestLoggerEvents, string) #4224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enhancement: trx logger can take logfile parameter #282

Add enhancement: trx logger can take logfile parameter #282

smadala commented Dec 14, 2016 •

edited

Loading

smadala Dec 14, 2016

codito commented Dec 14, 2016

codito Dec 15, 2016

smadala Dec 20, 2016

codito Dec 15, 2016

codito Dec 15, 2016

smadala Dec 20, 2016

codito Dec 15, 2016

smadala Dec 20, 2016

harshjain2 Dec 15, 2016

smadala Dec 20, 2016

harshjain2 commented Dec 15, 2016 •

edited

Loading

codito commented Dec 16, 2016

smadala commented Dec 20, 2016 •

edited

Loading

codito Dec 22, 2016

Faizan2304 Dec 22, 2016

Faizan2304 Dec 23, 2016

smadala Dec 23, 2016

codito Dec 23, 2016

smadala Dec 23, 2016

codito Dec 23, 2016

codito Dec 23, 2016

codito commented Dec 23, 2016

		@@ -40,6 +40,7 @@ $env:TP_TOOLS_DIR = Join-Path $env:TP_ROOT_DIR "tools"
		$env:TP_PACKAGES_DIR = Join-Path $env:TP_ROOT_DIR "packages"
		$env:TP_OUT_DIR = Join-Path $env:TP_ROOT_DIR "artifacts"

Add enhancement: trx logger can take logfile parameter #282

Add enhancement: trx logger can take logfile parameter #282

Conversation

smadala commented Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

codito commented Dec 14, 2016

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

harshjain2 commented Dec 15, 2016 • edited Loading

codito commented Dec 16, 2016

smadala commented Dec 20, 2016 • edited Loading

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

codito commented Dec 23, 2016

smadala commented Dec 14, 2016 •

edited

Loading

harshjain2 commented Dec 15, 2016 •

edited

Loading

smadala commented Dec 20, 2016 •

edited

Loading