Show proper warning message no tests to run because testcasefilter #1656

smadala · 2018-06-20T14:45:55Z

Description

Show better warning message on no tests discover/run when testcasefilter provided.
As the testcasefilter can be really lengthy in LUT scenarion.
1. Showing entire testcasefilter can cause following issues.
  a. Small scrollpane for output -> test
  b. Lengthy message can impact the performance of VS
2. Showing no given testcasefilter can lead to no proper logging/diagnostic information for user. Having proper logging/diagnostic info can help the user to understand the issue and help themselves.
  Example issue

First I have chosen 60 character as max testcasefilter length. While testing realized that max length 256 characters looks good.

For #1569

I have to fix this issue in current PR to write acceptance tests.
Ported required change from Moving discovery output to console logger #1111

Related issue

#1643
#1569

…-no-tests-message-on-filter

smadala · 2018-06-25T05:36:20Z

scripts/build/TestPlatform.Dependencies.props

@@ -6,8 +6,8 @@
    <!-- Name of the elements must be in sync with test\Microsoft.TestPlatform.TestUtilities\IntegrationTestBase.cs -->
    <NETTestSdkPreviousVersion>15.5.0</NETTestSdkPreviousVersion>

-    <MSTestFrameworkVersion>1.3.2</MSTestFrameworkVersion>
-    <MSTestAdapterVersion>1.3.2</MSTestAdapterVersion>
+    <MSTestFrameworkVersion>1.3.1</MSTestFrameworkVersion>


This change required to show proper diagnostic info on test failures of vstest repo. Related to microsoft/testfx#451 . We should fix the trx parsing logic in vstest repo.

Lets create a tracking bug for this.

Created one #1658

smadala · 2018-06-25T05:37:33Z

src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscovererEnumerator.cs

-            this.requestData.MetricsCollection.Add(TelemetryDataConstants.TimeTakenInSecByAllAdapters, totalTimeTakenByAdapters);
+        private static void LogWarningOnNoTestsDiscovered(IEnumerable<string> sources, string testCaseFilter, IMessageLogger logger)
+        {
+            var sourcesString = string.Join(" ", sources);


Actual change is here for discovery scenario.

smadala · 2018-06-25T05:38:44Z

src/Microsoft.TestPlatform.CrossPlatEngine/Execution/RunTestsWithSources.cs

@@ -170,5 +186,22 @@ protected override void InvokeExecutor(LazyExtension<ITestExecutor, ITestExecuto

            return result;
        }
+
+        private static string TestCaseFilterToShow(string testCaseFilter)


This is dead code.

smadala · 2018-06-25T05:39:52Z

src/Microsoft.TestPlatform.CrossPlatEngine/Microsoft.TestPlatform.CrossPlatEngine.csproj

@@ -11,7 +11,10 @@
    <!--<EnableCodeAnalysis>true</EnableCodeAnalysis>-->
  </PropertyGroup>
  <ItemGroup>
-    <EmbeddedResource Include="Resources\Resources.resx" />
+    <EmbeddedResource Include="Resources\Resources.resx">


To Make Resources.Designer.cs updates based on Resources.resx.

smadala · 2018-06-25T05:42:08Z

src/SettingsMigrator/Resources/xlf/Resources.cs.xlf

-Example: SettingsMigrator.exe  E:\MyTest\MyTestSettings.testsettings 
-Example: SettingsMigrator.exe  E:\MyTest\MyOldRunSettings.runsettings</source>
-        <target state="translated">Platné použití: SettingsMigrator.exe &lt;kompletní cesta k souboru testsettings nebo runsettings, který se má migrovat&gt;
+        <source>Valid usage:


These resources update missed in #1620
/cc @singhsarab @kaadhina

smadala · 2018-06-25T05:42:56Z

src/vstest.console/Internal/ConsoleLogger.cs

@@ -147,6 +147,12 @@ public void Initialize(TestLoggerEvents events, string testRunDirectory)
            events.TestRunMessage += this.TestMessageHandler;
            events.TestResult += this.TestResultHandler;
            events.TestRunComplete += this.TestRunCompleteHandler;
+
+            // Register for the discovery events.


Fix for #1569. ported from #1111

/cc @abhishkk

abhishkk · 2018-06-25T06:27:58Z

scripts/build/TestPlatform.Dependencies.props

@@ -6,8 +6,8 @@
    <!-- Name of the elements must be in sync with test\Microsoft.TestPlatform.TestUtilities\IntegrationTestBase.cs -->
    <NETTestSdkPreviousVersion>15.5.0</NETTestSdkPreviousVersion>

-    <MSTestFrameworkVersion>1.3.2</MSTestFrameworkVersion>
-    <MSTestAdapterVersion>1.3.2</MSTestAdapterVersion>
+    <MSTestFrameworkVersion>1.3.1</MSTestFrameworkVersion>


Lets create a tracking bug for this.

abhishkk · 2018-06-25T06:40:30Z

src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscovererEnumerator.cs

@@ -135,93 +135,174 @@ private void LoadTestsFromAnExtension(string extensionAssembly, IEnumerable<stri

            foreach (var discoverer in discovererToSourcesMap.Keys)
            {
-                Type discovererType = null;
+                this.DiscoverTestsFromSingleDiscoverer(discoverer, discovererToSourcesMap, context, discoverySink, logger, ref discoverersFromDeprecatedLocations, ref totalAdaptersUsed, ref totalTimeTakenByAdapters);


Avoid using ref and out in C#. Check this CA violation for more details.
Additional read: https://www.simplethread.com/ten-c-keywords-that-you-shouldne28099t-be-using/

Solution is to create multiple methods out the method with each method returning single value.

Check other methods as well. Example: TryToLoadDiscoverer

Avoid using ref and out in C#. Check this CA violation for more details.
Additional read: https://www.simplethread.com/ten-c-keywords-that-you-shouldne28099t-be-using/

Solution is to create multiple methods out the method with each method returning single value.

Reduced the ref usage. Only using in one place now.

Check other methods as well. Example: TryToLoadDiscoverer

TryDoSomework is very well used ways in BCL libraries. I would like to keep it as it is.

Using out keyword is not wrong but is not recommended. Please check above links for reasons. To get it cleared, what should be the recommended approach, we can have a second opinion. @cltshivash Can you recommend what should be the approach here?

abhishkk · 2018-06-25T06:59:45Z

src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscovererEnumerator.cs

+            ref double totalAdaptersUsed,
+            ref double totalTimeTakenByAdapters)
+        {
+            if (TryToLoadDiscoverer(discoverer, logger, out var discovererType) == false)


Nit: DiscovererEnumerator.TryToLoadDiscoverer

abhishkk · 2018-06-25T07:01:52Z

src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscovererEnumerator.cs

+            ref double totalAdaptersUsed,
+            ref double totalTimeTakenByAdapters)
+        {
+            if (TryToLoadDiscoverer(discoverer, logger, out var discovererType) == false)


TryToLoadDiscoverer already returns bool. No need for == false.

! is difficult to read. That's why kept false explicitly.

if(!succuess) is more readable than if(success == false). We can have a second opinion here as well to know what is the recommended approach here.

abhishkk · 2018-06-25T07:07:45Z

src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscovererEnumerator.cs


-                // if instantiated successfully, get tests
-                try
+                this.testPlatformEventSource.AdapterDiscoveryStart(discoverer.Metadata.DefaultExecutorUri.AbsoluteUri);


Nit: line spacing

Don't see any.

abhishkk · 2018-06-25T07:29:33Z

src/Microsoft.TestPlatform.CrossPlatEngine/Resources/Resources.Designer.cs

-
+
+        /// <summary>
+        ///   Looks up a localized string similar to No test is available for testcase filter `{0}` in {1}.


No test is available or No tests are available?

No test is available has been used in already existing resource string. To keep it consistency keeping it as it is.

abhishkk · 2018-06-25T07:30:28Z

src/Microsoft.TestPlatform.CrossPlatEngine/Utilities/TestCaseFilterDeterminer.cs

+    {
+        internal static string ShortenTestCaseFilterIfRequired(string testCaseFilter)
+        {
+            var maxLength = 256;


We can move it to class const

abhishkk · 2018-06-25T07:32:21Z

src/Microsoft.TestPlatform.CrossPlatEngine/Utilities/TestCaseFilterDeterminer.cs

+
+            if (testCaseFilter.Length > maxLength)
+            {
+                shortenTestCaseFilter = testCaseFilter.Substring(0, maxLength) + "...";


We can use ternary operator here as if else contains single line of code.
return testCaseFilter.Length > maxLength ? testCaseFilter.Substring(0, maxLength) + "..." : testCaseFilter OR
return testCaseFilter.Substring(0, Math.Min(testCaseFilter.Length, maxLength))

For better readability not using terninary operator.

That what case ternary operator should be used?
Ternary operators are useful for such scenarios only where if and else conditions have single lines

abhishkk · 2018-06-25T07:36:13Z

src/vstest.console/Internal/ConsoleLogger.cs

+            // Register for the discovery events.
+            events.DiscoveryMessage += this.TestMessageHandler;
+
+            // TODO Get changes from https://github.com/Microsoft/vstest/pull/1111/


Should there be a tracking item for this?

I have reopened the PR

abhishkk · 2018-06-25T07:37:07Z

src/vstest.console/Processors/RunTestsArgumentProcessor.cs

@@ -236,7 +238,7 @@ private void TestRunRequest_OnRunCompletion(object sender, TestRunCompleteEventA
                    var testsFoundInAnySource = (e.TestRunStatistics == null) ? false : (e.TestRunStatistics.ExecutedTests > 0);

                    // Indicate the user to use testadapterpath command if there are no tests found
-                    if (!testsFoundInAnySource && string.IsNullOrEmpty(CommandLineOptions.Instance.TestAdapterPath))
+                    if (!testsFoundInAnySource && string.IsNullOrEmpty(CommandLineOptions.Instance.TestAdapterPath) && this.commandLineOptions.TestCaseFilterValue == null) 


string.IsNullOrEmpty instead of == null?

TestCaseFilterValue can't be empty because we won't accept empty value.

abhishkk

Approved with suggestions

smadala · 2018-06-25T12:32:57Z

@abhishkk As suggestions are around personal choices of nits. I'm going ahead with merge. Will create another PR if required.

smadala added 6 commits June 14, 2018 14:25

Show proper warning message on no test with testcasefilter

28a448e

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

7a22a14

…-no-tests-message-on-filter

Show only first 60 character of testcasefilter

559545a

Add acceptance tests for discovery case

e17bd0d

Fix acceptance test

3e11ed9

Fix DiscovererEnumeratorTests nits

a11bcf8

smadala changed the title ~~[WIP] Show proper warning message no tests to run because testcasefilter~~ Show proper warning message no tests to run because testcasefilter Jun 21, 2018

smadala added 4 commits June 22, 2018 10:35

Add unit tests for DiscovererEnumeratorTests

fcb2595

Add unit tests for ConsoleLogger and RunTestsWithSources

2844597

Fix acceptance tests

971db88

Add resources changes

48c1c73

smadala requested review from mayankbansal018, cltshivash, abhishkk and kaadhina June 22, 2018 15:24

smadala commented Jun 25, 2018

View reviewed changes

abhishkk reviewed Jun 25, 2018

View reviewed changes

Reduce ref usage

d721871

abhishkk approved these changes Jun 25, 2018

View reviewed changes

smadala merged commit e0caba4 into microsoft:master Jun 25, 2018

smadala deleted the fix-no-tests-message-on-filter branch June 25, 2018 12:33

This was referenced Jun 25, 2018

TestSession.Message does not get logged to console during discovery #1569

Closed

Update resources files and test #1659

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show proper warning message no tests to run because testcasefilter #1656

Show proper warning message no tests to run because testcasefilter #1656

smadala commented Jun 20, 2018 •

edited

Loading

smadala Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

smadala Jun 25, 2018

smadala Jun 25, 2018

smadala Jun 25, 2018

smadala Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

abhishkk Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk Jun 25, 2018

smadala Jun 25, 2018

abhishkk left a comment

smadala commented Jun 25, 2018



		/// <summary>
		/// Looks up a localized string similar to No test is available for testcase filter `{0}` in {1}.

Show proper warning message no tests to run because testcasefilter #1656

Show proper warning message no tests to run because testcasefilter #1656

Conversation

smadala commented Jun 20, 2018 • edited Loading

Description

Related issue

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

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

abhishkk left a comment

Choose a reason for hiding this comment

smadala commented Jun 25, 2018

smadala commented Jun 20, 2018 •

edited

Loading