Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Jun 25, 2018
4 changes: 2 additions & 2 deletions scripts/build/TestPlatform.Dependencies.props
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

datadriven_h

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets create a tracking bug for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created one #1658

<MSTestAdapterVersion>1.3.1</MSTestAdapterVersion>
<MSTestAssertExtensionVersion>1.0.3-preview</MSTestAssertExtensionVersion>

<XUnitFrameworkVersion>2.3.1</XUnitFrameworkVersion>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Discovery
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
using Microsoft.VisualStudio.TestPlatform.PlatformAbstractions;

using Utilities;
using CrossPlatEngineResources = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Resources.Resources;

/// <summary>
Expand Down Expand Up @@ -109,7 +109,7 @@ private void LoadTestsFromAnExtension(string extensionAssembly, IEnumerable<stri

var discoverersFromDeprecatedLocations = false;

var discovererToSourcesMap = GetDiscovererToSourcesMap(extensionAssembly, sources, logger, this.assemblyProperties);
var discovererToSourcesMap = DiscovererEnumerator.GetDiscovererToSourcesMap(extensionAssembly, sources, logger, this.assemblyProperties);
var totalAdapterLoadTIme = DateTime.UtcNow - timeStart;

// Collecting Data Point for TimeTaken to Load Adapters
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check other methods as well. Example: TryToLoadDiscoverer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

}

// See if discoverer can be instantiated successfully else move next.
try
{
discovererType = discoverer.Value.GetType();
}
catch (Exception e)
{
var mesage = string.Format(
CultureInfo.CurrentUICulture,
CrossPlatEngineResources.DiscovererInstantiationException,
e.Message);
logger.SendMessage(TestMessageLevel.Warning, mesage);
EqtTrace.Error("DiscovererEnumerator.LoadTestsFromAnExtension: {0} ", e);
if (this.discoveryResultCache.TotalDiscoveredTests == 0)
{
DiscovererEnumerator.LogWarningOnNoTestsDiscovered(sources, testCaseFilter, logger);
}

continue;
}
this.CollectTelemetryAtEnd(totalTimeTakenByAdapters, totalAdaptersUsed);
}

private void CollectTelemetryAtEnd(double totalTimeTakenByAdapters, double totalAdaptersUsed)
{
// Collecting Total Time Taken by Adapters
this.requestData.MetricsCollection.Add(TelemetryDataConstants.TimeTakenInSecByAllAdapters,
totalTimeTakenByAdapters);

// Collecting Total Adapters Used to Discover tests
this.requestData.MetricsCollection.Add(TelemetryDataConstants.NumberOfAdapterUsedToDiscoverTests,
totalAdaptersUsed);
}

private void DiscoverTestsFromSingleDiscoverer(
LazyExtension<ITestDiscoverer, ITestDiscovererCapabilities> discoverer,
Dictionary<LazyExtension<ITestDiscoverer, ITestDiscovererCapabilities>,IEnumerable<string>> discovererToSourcesMap,
DiscoveryContext context,
TestCaseDiscoverySink discoverySink,
IMessageLogger logger,
ref bool discoverersFromDeprecatedLocations,
ref double totalAdaptersUsed,
ref double totalTimeTakenByAdapters)
{
if (TryToLoadDiscoverer(discoverer, logger, out var discovererType) == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: DiscovererEnumerator.TryToLoadDiscoverer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
// Fail to instantiate the discoverer type.
return;
}

// on instantiated successfully, get tests
try
{
EqtTrace.Verbose(
"DiscovererEnumerator.DiscoverTestsFromSingleDiscoverer: Loading tests for {0}",
discoverer.Value.GetType().FullName);

var currentTotalTests = this.discoveryResultCache.TotalDiscoveredTests;
var newTimeStart = DateTime.UtcNow;

// if instantiated successfully, get tests
try
this.testPlatformEventSource.AdapterDiscoveryStart(discoverer.Metadata.DefaultExecutorUri.AbsoluteUri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: line spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't see any.

discoverer.Value.DiscoverTests(discovererToSourcesMap[discoverer], context, logger, discoverySink);

var totalAdapterRunTime = DateTime.UtcNow - newTimeStart;

this.testPlatformEventSource.AdapterDiscoveryStop(this.discoveryResultCache.TotalDiscoveredTests -
currentTotalTests);

// Record Total Tests Discovered By each Adapter.
this.RecordTotalTestsDiscoveredByCurrentAdapter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Better naming? May be RecordTotalTestsDiscoveredByDiscoverer as we are already passing discoverer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After removing refs. this method no more required.

currentTotalTests,
discoverer,
ref discoverersFromDeprecatedLocations,
ref totalAdaptersUsed);

EqtTrace.Verbose("DiscovererEnumerator.DiscoverTestsFromSingleDiscoverer: Done loading tests for {0}",
discoverer.Value.GetType().FullName);


if (discoverersFromDeprecatedLocations)
Copy link
Contributor

Choose a reason for hiding this comment

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

discoverersFromDeprecatedLocations doesn't seem to be functionality of RecordTotalTestsDiscoveredByCurrentAdapter, we can separate it out. Also this usage of discoverersFromDeprecatedLocations can go to the new method as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good proposal. Fixed it.

{
if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose(
"DiscoveryContext.LoadTests: Loading tests for {0}",
discoverer.Value.GetType().FullName);
}

var currentTotalTests = this.discoveryResultCache.TotalDiscoveredTests;
var newTimeStart = DateTime.UtcNow;

this.testPlatformEventSource.AdapterDiscoveryStart(discoverer.Metadata.DefaultExecutorUri.AbsoluteUri);
discoverer.Value.DiscoverTests(discovererToSourcesMap[discoverer], context, logger, discoverySink);

var totalAdapterRunTime = DateTime.UtcNow - newTimeStart;

this.testPlatformEventSource.AdapterDiscoveryStop(this.discoveryResultCache.TotalDiscoveredTests - currentTotalTests);

// Collecting Total Tests Discovered By each Adapter.
if (this.discoveryResultCache.TotalDiscoveredTests > currentTotalTests)
{
var totalDiscoveredTests = this.discoveryResultCache.TotalDiscoveredTests - currentTotalTests;
this.requestData.MetricsCollection.Add(string.Format("{0}.{1}", TelemetryDataConstants.TotalTestsByAdapter, discoverer.Metadata.DefaultExecutorUri), totalDiscoveredTests);
if (!CrossPlatEngine.Constants.DefaultAdapters.Contains(discoverer.Metadata.DefaultExecutorUri.ToString(), StringComparer.OrdinalIgnoreCase))
{
var discovererLocation = discoverer.Value.GetType().GetTypeInfo().Assembly.GetAssemblyLocation();

discoverersFromDeprecatedLocations |= Path.GetDirectoryName(discovererLocation).Equals(CrossPlatEngine.Constants.DefaultAdapterLocation, StringComparison.OrdinalIgnoreCase);
}
totalAdaptersUsed++;
}

if (EqtTrace.IsVerboseEnabled)
{
EqtTrace.Verbose(
"DiscoveryContext.LoadTests: Done loading tests for {0}",
discoverer.Value.GetType().FullName);
}

if (discoverersFromDeprecatedLocations)
{
logger.SendMessage(TestMessageLevel.Warning, string.Format(CultureInfo.CurrentCulture, CrossPlatEngineResources.DeprecatedAdapterPath));
}

// Collecting Data Point for Time Taken to Discover Tests by each Adapter
this.requestData.MetricsCollection.Add(string.Format("{0}.{1}", TelemetryDataConstants.TimeTakenToDiscoverTestsByAnAdapter, discoverer.Metadata.DefaultExecutorUri), totalAdapterRunTime.TotalSeconds);
totalTimeTakenByAdapters += totalAdapterRunTime.TotalSeconds;
logger.SendMessage(TestMessageLevel.Warning,
string.Format(CultureInfo.CurrentCulture, CrossPlatEngineResources.DeprecatedAdapterPath));
}
catch (Exception e)

// Collecting Data Point for Time Taken to Discover Tests by each Adapter
this.requestData.MetricsCollection.Add(
string.Format("{0}.{1}", TelemetryDataConstants.TimeTakenToDiscoverTestsByAnAdapter,
discoverer.Metadata.DefaultExecutorUri), totalAdapterRunTime.TotalSeconds);
totalTimeTakenByAdapters += totalAdapterRunTime.TotalSeconds;
}
catch (Exception e)
{
var message = string.Format(
CultureInfo.CurrentUICulture,
CrossPlatEngineResources.ExceptionFromLoadTests,
discovererType.Name,
e.Message);

logger.SendMessage(TestMessageLevel.Error, message);
EqtTrace.Error("DiscovererEnumerator.DiscoverTestsFromSingleDiscoverer: {0} ", e);
}
}

private static bool TryToLoadDiscoverer(LazyExtension<ITestDiscoverer, ITestDiscovererCapabilities> discoverer, IMessageLogger logger, out Type discovererType)
{
discovererType = null;

// See if discoverer can be instantiated successfully else move next.
try
{
discovererType = discoverer.Value.GetType();
}
catch (Exception e)
{
var mesage = string.Format(
CultureInfo.CurrentUICulture,
CrossPlatEngineResources.DiscovererInstantiationException,
e.Message);
logger.SendMessage(TestMessageLevel.Warning, mesage);
EqtTrace.Error("DiscovererEnumerator.LoadTestsFromAnExtension: {0} ", e);

return false;
}

return true;
}

private void RecordTotalTestsDiscoveredByCurrentAdapter(long currentTotalTests, LazyExtension<ITestDiscoverer, ITestDiscovererCapabilities> discoverer,
ref bool discoverersFromDeprecatedLocations, ref double totalAdaptersUsed)
{
if (this.discoveryResultCache.TotalDiscoveredTests > currentTotalTests)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can inverse the if condition to reduce nesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not required after removing over use of ref.

{
var totalDiscoveredTests = this.discoveryResultCache.TotalDiscoveredTests - currentTotalTests;
this.requestData.MetricsCollection.Add(
string.Format("{0}.{1}", TelemetryDataConstants.TotalTestsByAdapter,
discoverer.Metadata.DefaultExecutorUri), totalDiscoveredTests);
if (!CrossPlatEngine.Constants.DefaultAdapters.Contains(discoverer.Metadata.DefaultExecutorUri.ToString(),
StringComparer.OrdinalIgnoreCase))
{
var message = string.Format(
CultureInfo.CurrentUICulture,
CrossPlatEngineResources.ExceptionFromLoadTests,
discovererType.Name,
e.Message);
var discovererLocation = discoverer.Value.GetType().GetTypeInfo().Assembly.GetAssemblyLocation();

logger.SendMessage(TestMessageLevel.Error, message);
EqtTrace.Error("DiscovererEnumerator.LoadTestsFromAnExtension: {0} ", e);
discoverersFromDeprecatedLocations |= Path.GetDirectoryName(discovererLocation)
Copy link
Contributor

Choose a reason for hiding this comment

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

If discoverersFromDeprecatedLocations is true, then no need to do anything like getting discovererLocation etc (as we are doing OR in discoverersFromDeprecatedLocations bool)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed it.

.Equals(CrossPlatEngine.Constants.DefaultAdapterLocation, StringComparison.OrdinalIgnoreCase);
}

totalAdaptersUsed++;
}
}

// Collecting Total Time Taken by Adapters
this.requestData.MetricsCollection.Add(TelemetryDataConstants.TimeTakenInSecByAllAdapters, totalTimeTakenByAdapters);
private static void LogWarningOnNoTestsDiscovered(IEnumerable<string> sources, string testCaseFilter, IMessageLogger logger)
{
var sourcesString = string.Join(" ", sources);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actual change is here for discovery scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we join strings by \n instead of space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping in one makes easy to copy and run tests from those assemblies again from CLI and currently we are keeping in one line other places.


// Collecting Total Adapters Used to Discover tests
this.requestData.MetricsCollection.Add(TelemetryDataConstants.NumberOfAdapterUsedToDiscoverTests, totalAdaptersUsed);
// Print warning on no tests.
if (string.IsNullOrEmpty(testCaseFilter) == false)
Copy link
Contributor

Choose a reason for hiding this comment

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

== false not required. We can use !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already discussed.

{
LogWarningOnNoTestsDiscoveredWithTestCaseFilter(testCaseFilter, logger, sourcesString);
}
else
{
logger.SendMessage(
TestMessageLevel.Warning,
string.Format(
CultureInfo.CurrentUICulture,
CrossPlatEngineResources.TestRunFailed_NoDiscovererFound_NoTestsAreAvailableInTheSources,
sourcesString));
}
}

private static void LogWarningOnNoTestsDiscoveredWithTestCaseFilter(
string testCaseFilter,
IMessageLogger logger,
string sourcesString)
{
var testCaseFilterToShow = TestCaseFilterDeterminer.ShortenTestCaseFilterIfRequired(testCaseFilter);

logger.SendMessage(
TestMessageLevel.Warning,
string.Format(CrossPlatEngineResources.NoTestsAvailableForGivenTestCaseFilter, testCaseFilterToShow, sourcesString));
}

private void SetAdapterLoggingSettings(IMessageLogger messageLogger, IRunSettings runSettings)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ namespace Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution

using ObjectModel.Client;
using ObjectModel.Logging;

using Utilities;
using CrossPlatEngineResources = Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Resources.Resources;

internal class RunTestsWithSources : BaseRunTests
Expand Down Expand Up @@ -62,10 +62,26 @@ protected override void BeforeRaisingTestRunComplete(bool exceptionsHitDuringRun
if (!exceptionsHitDuringRunTests && this.executorUriVsSourceList?.Count > 0 && !this.IsCancellationRequested
&& this.TestRunCache?.TotalExecutedTests <= 0)
{
IEnumerable<string> sources = new List<string>();
var sourcesArray = this.adapterSourceMap.Values.Aggregate(sources, (current, enumerable) => current.Concat(enumerable)).ToArray();
var sourcesString = string.Join(" ", sourcesArray);
this.LogWarningOnNoTestsDiscovered();
}
}

private void LogWarningOnNoTestsDiscovered()
Copy link
Contributor

Choose a reason for hiding this comment

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

NoTestsExecuted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

{
IEnumerable<string> sources = new List<string>();
var sourcesArray = this.adapterSourceMap.Values
.Aggregate(sources, (current, enumerable) => current.Concat(enumerable)).ToArray();
var sourcesString = string.Join(" ", sourcesArray);

if (this.TestExecutionContext.TestCaseFilter != null)
{
var testCaseFilterToShow = TestCaseFilterDeterminer.ShortenTestCaseFilterIfRequired(this.TestExecutionContext.TestCaseFilter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to LogWarningOnNoTestsDiscoveredWithTestCaseFilter, should there be LogWarningOnNoTestsExecutedWithTestCaseFilter method?
Current one also looks good being less lines of code in existing method. But we can do the same with discovery in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

this.TestRunEventsHandler?.HandleLogMessage(
TestMessageLevel.Warning,
string.Format(CrossPlatEngineResources.NoTestsAvailableForGivenTestCaseFilter, testCaseFilterToShow, sourcesString));
}
else
{
this.TestRunEventsHandler?.HandleLogMessage(
TestMessageLevel.Warning,
string.Format(
Expand Down Expand Up @@ -170,5 +186,22 @@ private Dictionary<Tuple<Uri, string>, IEnumerable<string>> GetExecutorVsSources

return result;
}

private static string TestCaseFilterToShow(string testCaseFilter)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is dead code.

{
var maxTestCaseFilterToShowLength = 63;
string testCaseFilterToShow;

if (testCaseFilter.Length > maxTestCaseFilterToShowLength)
{
testCaseFilterToShow = testCaseFilter.Substring(0, maxTestCaseFilterToShowLength - 3) + "...";
}
else
{
testCaseFilterToShow = testCaseFilter;
}

return testCaseFilterToShow;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ protected override void BeforeRaisingTestRunComplete(bool exceptionsHitDuringRun
protected override IEnumerable<Tuple<Uri, string>> GetExecutorUriExtensionMap(IFrameworkHandle testExecutorFrameworkHandle, RunContext runContext)
{
this.executorUriVsTestList = this.GetExecutorVsTestCaseList(this.testCases);

Debug.Assert(this.TestExecutionContext.TestCaseFilter == null, "TestCaseFilter should be null for specific tests.");
runContext.FilterExpressionWrapper = null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
<!--<EnableCodeAnalysis>true</EnableCodeAnalysis>-->
</PropertyGroup>
<ItemGroup>
<EmbeddedResource Include="Resources\Resources.resx" />
<EmbeddedResource Include="Resources\Resources.resx">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<Generator>ResXFileCodeGenerator</Generator>
<LastGenOutput>Resources.Designer.cs</LastGenOutput>
</EmbeddedResource>
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\Microsoft.TestPlatform.CommunicationUtilities\Microsoft.TestPlatform.CommunicationUtilities.csproj" />
Expand Down
Loading