Logger extensibility #557

Faizan2304 · 2017-03-01T19:58:11Z

This PR address following work

Loggers with name *.TestLogger.dll" will be discover from source directory
Loggers with name *.TestLogger.dll" will be discover from/TestAdapterPath
Loggers from source directory will be discovered only when DesignMode is false

1) support to load and enable from /TestAdapterPath and bin\debug folder 2) Move the logic to initialize logger from vstest.console to TestPlatform.client

…om TestPlatform.

msftclas · 2017-03-01T19:58:15Z

@Faizan2304,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

codito · 2017-03-02T04:43:51Z

src/Microsoft.TestPlatform.Client/TestPlatform.cs

+        /// <summary>
+        /// Update the test adapter paths provided through run settings to be used by the test service
+        /// </summary>
+        private void UpdateAndInitializeLoggers(TestRunCriteria testRunCriteria)


I think TP.Common is the right place for this logic.

Fixed in next commit

2) Fixed failed test

codito · 2017-03-03T04:01:09Z

scripts/test.ps1

@@ -164,6 +164,13 @@ function Invoke-Test
                # Fill in the framework in test containers
                $testContainerSet = $testContainers | % { [System.String]::Format($_, $fx) }
                $trxLogFileName  =  [System.String]::Format("Parallel_{0}_{1}", $fx, $Script:TPT_DefaultTrxFileName)
+
+                # Remove already existed trx file name as due to which warning will get generated and since we are expecting result in a particular format, that will break
+                $fullTrxFilePath = Join-Path $Script:TPT_TestResultsDir $trxLogFileName


Shouldn't trx file be overwritten automatically?

Or is it a warning that requires this step?

Yes it the warning due to which we required this step

codito · 2017-03-03T04:02:12Z

scripts/test.ps1

@@ -177,10 +184,10 @@ function Invoke-Test

                Reset-TestEnvironment

-                if ($output[-2].Contains("Test Run Successful.")) {
-                    Write-Log ".. . $($output[-3])"
+                if ($output[-3].Contains("Test Run Successful.")) {


Not quite sure how it is working today in clean machines.

Since we have added trx in our test, after summary it will add trx file info which is increasing the result by one line. Since every time we are deleting the trx file before running test, its equal to clean machine run

codito · 2017-03-03T04:02:58Z

src/Microsoft.TestPlatform.Client/TestPlatform.cs

                    List<string> adapterFiles = new List<string>(
-                        Directory.EnumerateFiles(adapterPath,TestPlatformConstants.TestAdapterPattern , SearchOption.AllDirectories)
+                        this.fileHelper.EnumerateFiles(adapterPath, TestPlatformConstants.TestAdapterResxPattern, SearchOption.AllDirectories)


nit: typo RegexPattern instead of ResxPattern

Fixed in next commit

codito · 2017-03-03T04:03:37Z

src/Microsoft.TestPlatform.Client/TestPlatform.cs

        /// <summary>
        /// Initializes a new instance of the <see cref="TestPlatform"/> class.
        /// </summary>
        public TestPlatform() : this(new TestEngine())
        {
+            fileHelper = new FileHelper();


Isn't it better is inject IFileHelper via ctor?

Yes, its a valid point. Fixed in next commit.

codito · 2017-03-03T06:03:29Z

src/Microsoft.TestPlatform.Common/Logging/TestLoggerManager.cs

        #endregion

        #region Public Methods

+        public void UpdateLoggerList(string arument, string loggerIdentifier, Dictionary<string, string> parameters)


Require docs for public methods.

updated in next commit

codito · 2017-03-03T06:04:24Z

src/Microsoft.TestPlatform.Common/Logging/TestLoggerManager.cs

+                    }
+                    else
+                    {
+                        throw new InvalidLoggerException(


nit: indent

codito · 2017-03-03T06:07:33Z

src/vstest.console/Processors/TestAdapterPathArgumentProcessor.cs

        internal virtual IEnumerable<string> GetTestAdaptersFromDirectory(string directory)
        {
-            return Directory.EnumerateFiles(directory, TestPlatformConstants.TestAdapterPattern, SearchOption.AllDirectories);
+            FileHelper fileHelper = new FileHelper();


Please check if there are tests for this. FileHelper should be injected.

Yes we have test for it.

codito · 2017-03-03T06:37:28Z

src/Microsoft.TestPlatform.Common/Logging/TestLoggerManager.cs

        #endregion

        #region Public Methods

+        public void UpdateLoggerList(string arument, string loggerIdentifier, Dictionary<string, string> parameters)


typo: argument

Fixed in next commit

faahmad added 7 commits February 28, 2017 12:26

Logger extensibility support: This PR is addressing the following work

3014f9b

1) support to load and enable from /TestAdapterPath and bin\debug folder 2) Move the logic to initialize logger from vstest.console to TestPlatform.client

Merge remote-tracking branch 'upstream/master'

5d8aac7

Initialize console logger from from vstest.console and rest logger fr…

89288db

…om TestPlatform.

Merge remote-tracking branch 'upstream/master'

ffa89e1

Merge remote-tracking branch 'upstream/master'

409dc44

Merge branch 'master' into loggerExtensibility

34d23db

Update loggers when DesignMode is false

01d1eb6

Faizan2304 requested review from codito and AbhitejJohn March 1, 2017 19:58

msftclas added the cla-not-required label Mar 1, 2017

Unit tests

8b56fd4

codito reviewed Mar 2, 2017

View reviewed changes

faahmad added 7 commits March 2, 2017 00:23

1) Build test.ps1 to address logger result

d9d8aa6

2) Fixed failed test

Merge remote-tracking branch 'upstream/master'

7be3bd9

Merge branch 'master' into loggerExtensibility

935afb6

Moved the logic to load and initilize loggers in TestLoggerManager

689cdc7

smoke Test fix

b71b303

Merge remote-tracking branch 'upstream/master'

afe0a00

Merge branch 'master' into loggerExtensibility

e40a450

codito reviewed Mar 3, 2017

View reviewed changes

src/Microsoft.TestPlatform.Common/Logging/TestLoggerManager.cs

}

else

{

throw new InvalidLoggerException(

Copy link

Contributor

codito Mar 3, 2017

Choose a reason for hiding this comment

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

nit: indent

codito reviewed Mar 3, 2017

View reviewed changes

codito approved these changes Mar 3, 2017

View reviewed changes

Addressed PR comment

73129bd

Faizan2304 merged commit 9a639af into microsoft:master Mar 3, 2017

Faizan2304 deleted the loggerExtensibility branch March 3, 2017 12:57

codito mentioned this pull request Mar 15, 2017

Load an extension from sources' paths #616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logger extensibility #557

Logger extensibility #557

Faizan2304 commented Mar 1, 2017

msftclas commented Mar 1, 2017

codito Mar 2, 2017

Faizan2304 Mar 2, 2017

codito Mar 3, 2017

codito Mar 3, 2017

Faizan2304 Mar 3, 2017

codito Mar 3, 2017

Faizan2304 Mar 3, 2017

codito Mar 3, 2017

Faizan2304 Mar 3, 2017

codito Mar 3, 2017

Faizan2304 Mar 3, 2017

codito Mar 3, 2017

Faizan2304 Mar 3, 2017

codito Mar 3, 2017

codito Mar 3, 2017

Faizan2304 Mar 3, 2017

codito Mar 3, 2017 •

edited

Loading

Faizan2304 Mar 3, 2017

Logger extensibility #557

Logger extensibility #557

Conversation

Faizan2304 commented Mar 1, 2017

msftclas commented Mar 1, 2017

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

codito Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codito Mar 3, 2017 •

edited

Loading