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

Exceptions flow to Translation layer #1434

Merged
merged 5 commits into from
Feb 19, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,6 @@ private void StartTestRun(TestRunRequestPayload testRunPayload, ITestRequestMana
{
EqtTrace.Error("DesignModeClient: Exception in StartTestRun: " + ex);

// If there is an exception during test run request creation or some time during the process
// In such cases, TestPlatform will never send a TestRunComplete event and IDE need to be sent a run complete message
// We need recoverability in translationlayer-designmode scenarios
var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = ex.ToString() };
this.communicationManager.SendMessage(MessageType.TestMessage, testMessagePayload);
var runCompletePayload = new TestRunCompletePayload()
Expand Down Expand Up @@ -331,9 +328,6 @@ private void StartDiscovery(DiscoveryRequestPayload discoveryRequestPayload, ITe
{
EqtTrace.Error("DesignModeClient: Exception in StartDiscovery: " + ex);

// If there is an exception during test discovery request creation or some time during the process
// In such cases, TestPlatform will never send a DiscoveryComplete event and IDE need to be sent a discovery complete message
// We need recoverability in translationlayer-designmode scenarios
var testMessagePayload = new TestMessagePayload { MessageLevel = TestMessageLevel.Error, Message = ex.ToString() };
this.communicationManager.SendMessage(MessageType.TestMessage, testMessagePayload);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ public interface ITestRequestManager : IDisposable
/// <param name="discoveryPayload">Discovery payload</param>
/// <param name="disoveryEventsRegistrar">Discovery events registrar - registers and unregisters discovery events</param>
/// <param name="protocolConfig">Protocol related information</param>
/// <returns>True, if successful</returns>
bool DiscoverTests(DiscoveryRequestPayload discoveryPayload, ITestDiscoveryEventsRegistrar disoveryEventsRegistrar, ProtocolConfig protocolConfig);
void DiscoverTests(DiscoveryRequestPayload discoveryPayload, ITestDiscoveryEventsRegistrar disoveryEventsRegistrar, ProtocolConfig protocolConfig);

/// <summary>
/// Run Tests with given a test of sources
Expand All @@ -43,8 +42,7 @@ public interface ITestRequestManager : IDisposable
/// <param name="customTestHostLauncher">Custom testHostLauncher for the run</param>
/// <param name="testRunEventsRegistrar">RunEvents registrar</param>
/// <param name="protocolConfig">Protocol related information</param>
/// <returns>True, if sucessful</returns>
bool RunTests(TestRunRequestPayload testRunRequestPayLoad, ITestHostLauncher customTestHostLauncher, ITestRunEventsRegistrar testRunEventsRegistrar, ProtocolConfig protocolConfig);
void RunTests(TestRunRequestPayload testRunRequestPayLoad, ITestHostLauncher customTestHostLauncher, ITestRunEventsRegistrar testRunEventsRegistrar, ProtocolConfig protocolConfig);

/// <summary>
/// Cancel the current TestRun request
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -553,17 +553,6 @@ internal void AddCollectorDataEntries(IEnumerable<CollectorDataEntry> collectorD
{
Debug.Assert(collectorDataEntry != null, "'collectorDataEntry' is null");
Debug.Assert(!this.collectorDataEntries.Contains(collectorDataEntry), "The collector data entry already exists in the collection");
#if DEBUG
// Verify that any URI data attachments in the entry have relative paths
foreach (IDataAttachment attachment in collectorDataEntry.Attachments)
{
UriDataAttachment uriDataAttachment = attachment as UriDataAttachment;
if (uriDataAttachment != null)
{
Debug.Assert(uriDataAttachment.Uri.IsAbsoluteUri, "'collectorDataEntry' contains a URI data attachment with a relative URI");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same goes here, if can pick attachments, from relative URI, the change it that this path should not be null/empty

}
}
#endif

this.collectorDataEntries.Add(collectorDataEntry.Clone(testResultsDirectory, false));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ internal UriDataAttachment Clone(string baseDirectory, bool useAbsoluteUri)
{
Debug.Assert(!string.IsNullOrEmpty(baseDirectory), "'baseDirectory' is null or empty");
Debug.Assert(baseDirectory == baseDirectory.Trim(), "'baseDirectory' contains whitespace at the ends");
Debug.Assert(Path.IsPathRooted(baseDirectory), "'baseDirectory' is not a rooted path");
Copy link
Contributor

Choose a reason for hiding this comment

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

Assertion if to test assumption, here it says that this method expects basedirectory to be rooted, if that is not the case, then may be change to to that it should not be null or empty.
Removal should only happen when we are not at all needing baseDirectory


if (useAbsoluteUri != this.uri.IsAbsoluteUri)
{
Expand Down
10 changes: 9 additions & 1 deletion src/vstest.console/CommandLine/Executor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -329,11 +329,19 @@ private bool ExecuteArgumentProcessor(IArgumentProcessor processor, ref int exit
}
catch (Exception ex)
{
if (ex is CommandLineException || ex is TestPlatformException || ex is SettingsException)
if (ex is CommandLineException || ex is TestPlatformException || ex is SettingsException || ex is InvalidOperationException)
{
EqtTrace.Error("ExecuteArgumentProcessor: failed to execute argument process: {0}", ex);
this.Output.Error(false, ex.Message);
result = ArgumentProcessorResult.Fail;

// Send inner exception only when its message is different to avoid duplicate.
if (ex is TestPlatformException &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of this we can do ex.ToString() in line 335 which will print inner exception. But in that case duplicacy will be there if inner exception and outer exception message are same. This happens for many scenarios in TestPlatformException

ex.InnerException != null &&
!string.Equals(ex.InnerException.Message, ex.Message, StringComparison.CurrentCultureIgnoreCase))
{
this.Output.Error(false, ex.InnerException.Message);
}
}
else
{
Expand Down
29 changes: 1 addition & 28 deletions src/vstest.console/Internal/ConsoleLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -523,38 +523,11 @@ private void TestRunCompleteHandler(object sender, TestRunCompleteEventArgs e)
}
#endregion

/// <summary>
/// Raises test run errors occured before console logger starts listening error events.
/// </summary>
/// <param name="testRunResultAggregator"></param>
/// <param name="exception"></param>
public static void RaiseTestRunError(TestRunResultAggregator testRunResultAggregator, Exception exception)
{
if (Output == null)
{
Output = ConsoleOutput.Instance;
}

// testRunResultAggregator can be null, if error is being raised in discovery context.
testRunResultAggregator?.MarkTestRunFailed();

Output.Error(AppendPrefix, exception.Message);

// Send inner exception only when its message is different to avoid duplicate.
if (exception is TestPlatformException &&
exception.InnerException != null &&
string.Compare(exception.Message, exception.InnerException.Message, StringComparison.CurrentCultureIgnoreCase) != 0)
{
Output.Error(AppendPrefix, exception.InnerException.Message);
}
}

/// <summary>
/// Raises test run warning occured before console logger starts listening warning events.
/// </summary>
/// <param name="testRunResultAggregator"></param>
/// <param name="warningMessage"></param>
public static void RaiseTestRunWarning(TestRunResultAggregator testRunResultAggregator, string warningMessage)
public static void RaiseTestRunWarning(string warningMessage)
{
if (ConsoleLogger.Output == null)
{
Expand Down
12 changes: 3 additions & 9 deletions src/vstest.console/Processors/EnableLoggerArgumentProcessor.cs
Original file line number Diff line number Diff line change
@@ -1,28 +1,22 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.ComponentModel;
using System.Xml;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move these as well below?

using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Utilities;

namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors
{
using Microsoft.VisualStudio.TestPlatform.Common.Logging;
using System;
using System.Collections.Generic;
using System.Diagnostics.Contracts;
using System.Globalization;

using Microsoft.VisualStudio.TestPlatform.CommandLine.Processors.Utilities;

using CommandLineResources = Microsoft.VisualStudio.TestPlatform.CommandLine.Resources.Resources;
using Microsoft.VisualStudio.TestPlatform.CommandLine.Internal;
using Microsoft.VisualStudio.TestPlatform.Client;
using Microsoft.VisualStudio.TestPlatform.Common.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
using System.Collections.ObjectModel;
using Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities;
using CommandLineResources = Microsoft.VisualStudio.TestPlatform.CommandLine.Resources.Resources;

/// <summary>
/// An argument processor that allows the user to enable a specific logger
Expand Down Expand Up @@ -218,7 +212,7 @@ public static void AddLoggerToRunSettings(string loggerArgument, IRunSettingsPro

// Remove existing logger.
var existingLoggerIndex = loggerRunSettings.GetExistingLoggerIndex(logger);
if (existingLoggerIndex > 0)
if (existingLoggerIndex >= 0)
{
loggerRunSettings.LoggerSettingsList.RemoveAt(existingLoggerIndex);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ public ArgumentProcessorResult Execute()

var runSettings = this.runSettingsManager.ActiveRunSettings.SettingsXml;

var success = this.testRequestManager.DiscoverTests(
this.testRequestManager.DiscoverTests(
new DiscoveryRequestPayload { Sources = this.commandLineOptions.Sources, RunSettings = runSettings },
this.discoveryEventsRegistrar, Constants.DefaultProtocolConfig);

Expand All @@ -226,7 +226,7 @@ public ArgumentProcessorResult Execute()
}

File.WriteAllLines(this.commandLineOptions.ListTestsTargetPath, this.discoveredTests);
return success ? ArgumentProcessorResult.Success : ArgumentProcessorResult.Fail;
return ArgumentProcessorResult.Success;
}

#endregion
Expand Down
4 changes: 2 additions & 2 deletions src/vstest.console/Processors/ListTestsArgumentProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,11 @@ public ArgumentProcessorResult Execute()

var runSettings = this.runSettingsManager.ActiveRunSettings.SettingsXml;

var success = this.testRequestManager.DiscoverTests(
this.testRequestManager.DiscoverTests(
new DiscoveryRequestPayload() { Sources = this.commandLineOptions.Sources, RunSettings = runSettings },
this.discoveryEventsRegistrar, Constants.DefaultProtocolConfig);

return success ? ArgumentProcessorResult.Success : ArgumentProcessorResult.Fail;
return ArgumentProcessorResult.Success;
}

#endregion
Expand Down
21 changes: 7 additions & 14 deletions src/vstest.console/Processors/RunSpecificTestsArgumentProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors

using Microsoft.VisualStudio.TestPlatform.Client.RequestHelper;
using Microsoft.VisualStudio.TestPlatform.CommandLine.TestPlatformHelpers;
using Microsoft.VisualStudio.TestPlatform.CommandLine.Processors.Utilities;
using Microsoft.VisualStudio.TestPlatform.CommandLineUtilities;
using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
Expand Down Expand Up @@ -210,17 +208,15 @@ public ArgumentProcessorResult Execute()
throw new CommandLineException(string.Format(CultureInfo.CurrentUICulture, CommandLineResources.InvalidTestCaseFilterValueForSpecificTests));
}

bool result = false;

this.effectiveRunSettings = this.runSettingsManager.ActiveRunSettings.SettingsXml;

// Discover tests from sources and filter on every discovery reported.
result = this.DiscoverTestsAndSelectSpecified(this.commandLineOptions.Sources);
this.DiscoverTestsAndSelectSpecified(this.commandLineOptions.Sources);

// Now that tests are discovered and filtered, we run only those selected tests.
result = result && this.ExecuteSelectedTests();
this.ExecuteSelectedTests();

return result ? ArgumentProcessorResult.Success : ArgumentProcessorResult.Fail;
return ArgumentProcessorResult.Success;
}

#endregion
Expand All @@ -231,24 +227,23 @@ public ArgumentProcessorResult Execute()
/// Discovers tests from the given sources and selects only specified tests.
/// </summary>
/// <param name="sources"> Test source assemblies paths. </param>
private bool DiscoverTestsAndSelectSpecified(IEnumerable<string> sources)
private void DiscoverTestsAndSelectSpecified(IEnumerable<string> sources)
{
this.output.WriteLine(CommandLineResources.StartingDiscovery, OutputLevel.Information);
if (!string.IsNullOrEmpty(EqtTrace.LogFile))
{
this.output.Information(false, CommandLineResources.VstestDiagLogOutputPath, EqtTrace.LogFile);
}

return this.testRequestManager.DiscoverTests(
this.testRequestManager.DiscoverTests(
new DiscoveryRequestPayload() { Sources = sources, RunSettings = this.effectiveRunSettings }, this.discoveryEventsRegistrar, Constants.DefaultProtocolConfig);
}

/// <summary>
/// Executes the selected tests
/// </summary>
private bool ExecuteSelectedTests()
private void ExecuteSelectedTests()
{
bool result = true;
if (this.selectedTestCases.Count > 0)
{
if (this.undiscoveredFilters.Count() != 0)
Expand All @@ -263,7 +258,7 @@ private bool ExecuteSelectedTests()

EqtTrace.Verbose("RunSpecificTestsArgumentProcessor:Execute: Test run is queued.");
var runRequestPayload = new TestRunRequestPayload() { TestCases = this.selectedTestCases.ToList(), RunSettings = this.effectiveRunSettings, KeepAlive = keepAlive, TestPlatformOptions = new TestPlatformOptions() { TestCaseFilter = this.commandLineOptions.TestCaseFilterValue }};
result &= this.testRequestManager.RunTests(runRequestPayload, null, null, Constants.DefaultProtocolConfig);
this.testRequestManager.RunTests(runRequestPayload, null, null, Constants.DefaultProtocolConfig);
}
else
{
Expand All @@ -286,8 +281,6 @@ private bool ExecuteSelectedTests()

this.output.Warning(false, warningMessage);
}

return result;
}

/// <summary>
Expand Down
12 changes: 4 additions & 8 deletions src/vstest.console/Processors/RunTestsArgumentProcessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Processors

using Microsoft.VisualStudio.TestPlatform.Client.RequestHelper;
using Microsoft.VisualStudio.TestPlatform.CommandLine.TestPlatformHelpers;
using Microsoft.VisualStudio.TestPlatform.CommandLineUtilities;
using Microsoft.VisualStudio.TestPlatform.Common;
using Microsoft.VisualStudio.TestPlatform.Common.Interfaces;
using Microsoft.VisualStudio.TestPlatform.ObjectModel;
Expand Down Expand Up @@ -168,16 +167,15 @@ public ArgumentProcessorResult Execute()
this.output.Information(false, CommandLineResources.VstestDiagLogOutputPath, EqtTrace.LogFile);
}

var success = true;
if (this.commandLineOptions.Sources.Any())
{
success = this.RunTests(this.commandLineOptions.Sources);
this.RunTests(this.commandLineOptions.Sources);
}

return success ? ArgumentProcessorResult.Success : ArgumentProcessorResult.Fail;
return ArgumentProcessorResult.Success;
}

private bool RunTests(IEnumerable<string> sources)
private void RunTests(IEnumerable<string> sources)
{
// create/start test run
if (EqtTrace.IsInfoEnabled)
Expand All @@ -197,14 +195,12 @@ private bool RunTests(IEnumerable<string> sources)
var keepAlive = false;

var runRequestPayload = new TestRunRequestPayload() { Sources = this.commandLineOptions.Sources.ToList(), RunSettings = runSettings, KeepAlive = keepAlive, TestPlatformOptions= new TestPlatformOptions() { TestCaseFilter = this.commandLineOptions.TestCaseFilterValue } };
var result = this.testRequestManager.RunTests(runRequestPayload, null, this.testRunEventsRegistrar, Constants.DefaultProtocolConfig);
this.testRequestManager.RunTests(runRequestPayload, null, this.testRunEventsRegistrar, Constants.DefaultProtocolConfig);

if (EqtTrace.IsInfoEnabled)
{
EqtTrace.Info("RunTestsArgumentProcessor:Execute: Test run is completed.");
}

return result;
}

private class TestRunRequestEventsRegistrar : ITestRunEventsRegistrar
Expand Down
Loading