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.

Sign up for GitHub

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

Update framework detection logic to not rely on throwing/catching NRE #3714

Merged
merged 2 commits into from
Jun 3, 2022
Merged

Update framework detection logic to not rely on throwing/catching NRE #3714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f661007
Fix some incorrect merge and missed warning
Evangelink Jun 2, 2022
6128a31
Update framework detection logic to not rely on throwing/catching NRE
Evangelink Jun 2, 2022
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
4 changes: 4 additions & 0 deletions TestPlatform.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "testhost.arm64", "src\testh
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "DumpMinitool.arm64", "src\DataCollectors\DumpMinitool.arm64\DumpMinitool.arm64.csproj", "{62E9D32B-B989-43CF-81A2-B38B3367FCA3}"
EndProject
Project("{D954291E-2A0B-460D-934E-DC6B0785DB48}") = "Microsoft.TestPlatform.Nullability", "src\Microsoft.TestPlatform.Nullability\Microsoft.TestPlatform.Nullability.shproj", "{9DF3BC3C-2A53-46E7-9291-40728ACE5F82}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -1023,6 +1025,7 @@ Global
{29270853-90DC-4C39-9621-F47AE40A79B6} = {B27FAFDF-2DBA-4AB0-BA85-FD5F21D359D6}
{186069FE-E1E8-4DE1-BEA4-0FF1484D22D1} = {ED0C35EB-7F31-4841-A24F-8EB708FFA959}
{62E9D32B-B989-43CF-81A2-B38B3367FCA3} = {B705537C-B82C-4A30-AFA5-6244D9A7DAEB}
{9DF3BC3C-2A53-46E7-9291-40728ACE5F82} = {ED0C35EB-7F31-4841-A24F-8EB708FFA959}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {0541B30C-FF51-4E28-B172-83F5F3934BCD}
Expand All @@ -1037,6 +1040,7 @@ Global
src\Microsoft.TestPlatform.Nullability\Microsoft.TestPlatform.Nullability.projitems*{68adc720-316e-4895-9f8e-c3ccadd262be}*SharedItemsImports = 5
src\Microsoft.TestPlatform.Execution.Shared\Microsoft.TestPlatform.Execution.Shared.projitems*{71cb42ff-e750-4a3b-9c3a-ac938853cc89}*SharedItemsImports = 5
src\Microsoft.TestPlatform.Execution.Shared\Microsoft.TestPlatform.Execution.Shared.projitems*{7f26eda3-c8c4-4b7f-a9b6-d278c2f40a13}*SharedItemsImports = 13
src\Microsoft.TestPlatform.Nullability\Microsoft.TestPlatform.Nullability.projitems*{9df3bc3c-2a53-46e7-9291-40728ace5f82}*SharedItemsImports = 13
src\Microsoft.TestPlatform.Nullability\Microsoft.TestPlatform.Nullability.projitems*{fbf74c8f-695c-4967-84ac-358eefb1376d}*SharedItemsImports = 5
EndGlobalSection
EndGlobal
3 changes: 2 additions & 1 deletion src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using System.Text;
Expand Down Expand Up @@ -434,7 +435,7 @@ public static bool TryGetLegacySettingElements(string runsettingsXml, out Dictio
public static void UpdateTargetPlatform(XmlDocument runSettingsDocument, string platform, bool overwrite = false)
=> AddNodeIfNotPresent(runSettingsDocument, TargetPlatformNodePath, TargetPlatformNodeName, platform, overwrite);

public static bool TryGetDeviceXml(XPathNavigator runSettingsNavigator, out string? deviceXml)
public static bool TryGetDeviceXml(XPathNavigator runSettingsNavigator, [NotNullWhen(true)] out string? deviceXml)
{
ValidateArg.NotNull(runSettingsNavigator, nameof(runSettingsNavigator));

Expand Down
6 changes: 3 additions & 3 deletions src/vstest.console/CommandLine/AssemblyMetadataProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ internal AssemblyMetadataProvider(IFileHelper fileHelper)
}

/// <inheritdoc />
public FrameworkName GetFrameWork(string filePath)
public FrameworkName GetFrameworkName(string filePath)
Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow unrelated but I got confused by the API name compared to its return type when fixing the auto-detection algorithm so I also fixed the name.

{
FrameworkName frameworkName = new(Framework.DefaultFramework.Name);
try
Expand All @@ -42,10 +42,10 @@ public FrameworkName GetFrameWork(string filePath)
}
catch (Exception ex)
{
EqtTrace.Warning("AssemblyMetadataProvider.GetFrameWork: failed to determine TargetFrameworkVersion exception: {0} for assembly: {1}", ex, filePath);
EqtTrace.Warning("AssemblyMetadataProvider.GetFrameworkName: failed to determine TargetFrameworkVersion exception: {0} for assembly: {1}", ex, filePath);
}

EqtTrace.Info("AssemblyMetadataProvider.GetFrameWork: Determined framework:'{0}' for source: '{1}'", frameworkName, filePath);
EqtTrace.Info("AssemblyMetadataProvider.GetFrameworkName: Determined framework:'{0}' for source: '{1}'", frameworkName, filePath);

return frameworkName;
}
Expand Down
33 changes: 12 additions & 21 deletions src/vstest.console/CommandLine/InferHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,35 +133,24 @@ public Architecture AutoDetectArchitecture(IList<string?>? sources, Architecture
public Framework AutoDetectFramework(IList<string?>? sources, out IDictionary<string, Framework> sourceToFrameworkMap)
{
sourceToFrameworkMap = new Dictionary<string, Framework>();
Framework framework = Framework.DefaultFramework;

if (sources == null || sources.Count == 0)
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not filtering null sources here because the simplest solution is to actually return Framework instead of FrameworkName on DetermineFramework. This way I can solve the "problem" of instances equality.

return framework;
return Framework.DefaultFramework;

try
var framework = DetermineFramework(sources, out sourceToFrameworkMap, out var conflictInFxIdentifier);
if (conflictInFxIdentifier)
{
var finalFx = DetermineFrameworkName(sources, out sourceToFrameworkMap, out var conflictInFxIdentifier);
TPDebug.Assert(finalFx != null, "finalFx should not be null");
framework = Framework.FromString(finalFx.FullName);
if (conflictInFxIdentifier)
{
// TODO Log to console and client.
EqtTrace.Info(
"conflicts in Framework identifier of provided sources(test assemblies), using default framework:{0}",
framework);
}
}
catch (Exception ex)
Copy link
Member Author

Choose a reason for hiding this comment

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

Try/catch was only catching NRE when finalFx is null which happens only when all sources provided are null. DetermineFrameworkName and Framework.FromString are having inner try/catch.

{
EqtTrace.Error("Failed to determine framework:{0}, using default: {1}", ex, framework);
// TODO Log to console and client.
EqtTrace.Info(
"conflicts in Framework identifier of provided sources(test assemblies), using default framework: {0}",
framework);
}

EqtTrace.Info("Determined framework for all sources: {0}", framework);

return framework;
}

private FrameworkName? DetermineFrameworkName(IEnumerable<string?> sources, out IDictionary<string, Framework> sourceToFrameworkMap, out bool conflictInFxIdentifier)
private Framework DetermineFramework(IEnumerable<string?> sources, out IDictionary<string, Framework> sourceToFrameworkMap, out bool conflictInFxIdentifier)
Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially we could move this as local function of AutoDetetFramework.

{
sourceToFrameworkMap = new Dictionary<string, Framework>();

Expand All @@ -180,7 +169,7 @@ public Framework AutoDetectFramework(IList<string?>? sources, out IDictionary<st
FrameworkName fx;
if (IsDllOrExe(source))
{
fx = _assemblyMetadataProvider.GetFrameWork(source);
fx = _assemblyMetadataProvider.GetFrameworkName(source);
}
else
{
Expand Down Expand Up @@ -231,7 +220,9 @@ public Framework AutoDetectFramework(IList<string?>? sources, out IDictionary<st
}
}

return finalFx;
return finalFx != null
? Framework.FromString(finalFx.FullName)
: defaultFramework;
}

private static bool IsDllOrExe(string? filePath)
Expand Down
2 changes: 1 addition & 1 deletion src/vstest.console/CommandLine/Interfaces/IAssemblyMetadataProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ internal interface IAssemblyMetadataProvider
/// <summary>
/// Determines FrameworkName from filePath.
/// </summary>
FrameworkName GetFrameWork(string filePath);
FrameworkName GetFrameworkName(string filePath);

/// <summary>
/// Determines Architecture from filePath.
Expand Down
7 changes: 4 additions & 3 deletions src/vstest.console/TestPlatformHelpers/TestRequestManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,12 +859,13 @@ private bool UpdateTargetDevice(
XPathNavigator navigator,
XmlDocument document)
{
bool updateRequired = InferRunSettingsHelper.TryGetDeviceXml(navigator, out string deviceXml);
if (updateRequired)
if (InferRunSettingsHelper.TryGetDeviceXml(navigator, out string? deviceXml))
{
InferRunSettingsHelper.UpdateTargetDevice(document, deviceXml);
return true;
}
return updateRequired;

return false;
}

private bool UpdateCollectSourceInformation(
Expand Down
2 changes: 1 addition & 1 deletion test/vstest.ProgrammerTests/Fakes/FakeAssemblyMetadataProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public Architecture GetArchitecture(string filePath)
return file.Architecture;
}

public FrameworkName GetFrameWork(string filePath)
public FrameworkName GetFrameworkName(string filePath)
{
var file = FakeFileHelper.GetFakeFile<FakeTestDllFile>(filePath);
return file.FrameworkName;
Expand Down
4 changes: 2 additions & 2 deletions test/vstest.console.PlatformTests/AssemblyMetadataProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public void GetFrameWorkForDotNetAssembly(string framework)
var assemblyPath = _testEnvironment.GetTestAsset("SimpleTestProject3.dll", framework);
LoadAssemblyIntoMemory(assemblyPath);
var stopWatch = Stopwatch.StartNew();
var actualFx = _assemblyMetadataProvider.GetFrameWork(assemblyPath);
var actualFx = _assemblyMetadataProvider.GetFrameworkName(assemblyPath);
stopWatch.Stop();

if (framework.Equals("net451"))
Expand All @@ -142,7 +142,7 @@ public void GetFrameWorkForNativeDll()
var assemblyPath = $@"{_testEnvironment.PackageDirectory}\microsoft.testplatform.testasset.nativecpp\2.0.0\contentFiles\any\any\Microsoft.TestPlatform.TestAsset.NativeCPP.dll";
LoadAssemblyIntoMemory(assemblyPath);
var stopWatch = Stopwatch.StartNew();
var fx = _assemblyMetadataProvider.GetFrameWork(assemblyPath);
var fx = _assemblyMetadataProvider.GetFrameworkName(assemblyPath);
stopWatch.Stop();

Console.WriteLine(PerfAssertMessageFormat, expectedElapsedTime, stopWatch.ElapsedMilliseconds);
Expand Down
16 changes: 8 additions & 8 deletions test/vstest.console.UnitTests/CommandLine/InferHelperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,18 +222,18 @@ public void AutoDetectFrameworkShouldReturnDefaultFullFrameworkForJsFiles()
[TestMethod]
public void AutoDetectFrameworkShouldReturnHighestVersionFxOnSameFxName()
{
_mockAssemblyHelper.SetupSequence(sh => sh.GetFrameWork(It.IsAny<string>()))
_mockAssemblyHelper.SetupSequence(sh => sh.GetFrameworkName(It.IsAny<string>()))
.Returns(new FrameworkName(_frameworkNet46.Name))
.Returns(new FrameworkName(_frameworkNet47.Name))
.Returns(new FrameworkName(_frameworkNet45.Name));
Assert.AreEqual(_frameworkNet47.Name, _inferHelper.AutoDetectFramework(new List<string?>() { "net46.dll", "net47.exe", "net45.dll" }, out _).Name);
_mockAssemblyHelper.Verify(ah => ah.GetFrameWork(It.IsAny<string>()), Times.Exactly(3));
_mockAssemblyHelper.Verify(ah => ah.GetFrameworkName(It.IsAny<string>()), Times.Exactly(3));
}

[TestMethod]
public void AutoDetectFrameworkShouldPopulatetheDictionaryForAllTheSources()
{
_mockAssemblyHelper.SetupSequence(sh => sh.GetFrameWork(It.IsAny<string>()))
_mockAssemblyHelper.SetupSequence(sh => sh.GetFrameworkName(It.IsAny<string>()))
.Returns(new FrameworkName(_frameworkNet46.Name))
.Returns(new FrameworkName(_frameworkNet47.Name))
.Returns(new FrameworkName(_frameworkNet45.Name));
Expand All @@ -244,28 +244,28 @@ public void AutoDetectFrameworkShouldPopulatetheDictionaryForAllTheSources()
Assert.AreEqual(_frameworkNet46.Name, sourceFrameworks["net46.dll"].Name);
Assert.AreEqual(_frameworkNet47.Name, sourceFrameworks["net47.exe"].Name);
Assert.AreEqual(_frameworkNet45.Name, sourceFrameworks["net45.dll"].Name);
_mockAssemblyHelper.Verify(ah => ah.GetFrameWork(It.IsAny<string>()), Times.Exactly(3));
_mockAssemblyHelper.Verify(ah => ah.GetFrameworkName(It.IsAny<string>()), Times.Exactly(3));
}

[TestMethod]
public void AutoDetectFrameworkShouldReturnHighestVersionFxOnEvenManyLowerVersionFxNameExists()
{
_mockAssemblyHelper.SetupSequence(sh => sh.GetFrameWork(It.IsAny<string>()))
_mockAssemblyHelper.SetupSequence(sh => sh.GetFrameworkName(It.IsAny<string>()))
.Returns(new FrameworkName(_frameworkCore10.Name))
.Returns(new FrameworkName(_frameworkCore11.Name))
.Returns(new FrameworkName(_frameworkCore10.Name));
Assert.AreEqual(_frameworkCore11.Name, _inferHelper.AutoDetectFramework(new List<string?>() { "netcore10_1.dll", "netcore11.dll", "netcore10_2.dll" }, out _).Name);
_mockAssemblyHelper.Verify(ah => ah.GetFrameWork(It.IsAny<string>()), Times.Exactly(3));
_mockAssemblyHelper.Verify(ah => ah.GetFrameworkName(It.IsAny<string>()), Times.Exactly(3));
}

private void SetupAndValidateForSingleAssembly(string assemblyName, Framework fx, bool verify)
{
_mockAssemblyHelper.Setup(sh => sh.GetFrameWork(assemblyName))
_mockAssemblyHelper.Setup(sh => sh.GetFrameworkName(assemblyName))
.Returns(new FrameworkName(fx.Name));
Assert.AreEqual(fx.Name, _inferHelper.AutoDetectFramework(new List<string?>() { assemblyName }, out _).Name);
if (verify)
{
_mockAssemblyHelper.Verify(ah => ah.GetFrameWork(assemblyName));
_mockAssemblyHelper.Verify(ah => ah.GetFrameworkName(assemblyName));
}
}
}
2 changes: 1 addition & 1 deletion test/vstest.console.UnitTests/Processors/ListFullyQualifiedTestsArgumentProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public ListFullyQualifiedTestsArgumentProcessorTests()
_mockTestPlatformEventSource = new Mock<ITestPlatformEventSource>();
_mockAssemblyMetadataProvider = new Mock<IAssemblyMetadataProvider>();
_mockAssemblyMetadataProvider.Setup(x => x.GetArchitecture(It.IsAny<string>())).Returns(Architecture.X64);
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameWork(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameworkName(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_inferHelper = new InferHelper(_mockAssemblyMetadataProvider.Object);
_mockProcessHelper = new Mock<IProcessHelper>();
_mockAttachmentsProcessingManager = new Mock<ITestRunAttachmentsProcessingManager>();
Expand Down
2 changes: 1 addition & 1 deletion test/vstest.console.UnitTests/Processors/ListTestsArgumentProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public ListTestsArgumentProcessorTests()
_mockTestPlatformEventSource = new Mock<ITestPlatformEventSource>();
_mockAssemblyMetadataProvider = new Mock<IAssemblyMetadataProvider>();
_mockAssemblyMetadataProvider.Setup(x => x.GetArchitecture(It.IsAny<string>())).Returns(Architecture.X64);
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameWork(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameworkName(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_inferHelper = new InferHelper(_mockAssemblyMetadataProvider.Object);
_mockProcessHelper = new Mock<IProcessHelper>();
_mockAttachmentsProcessingManager = new Mock<ITestRunAttachmentsProcessingManager>();
Expand Down
2 changes: 1 addition & 1 deletion test/vstest.console.UnitTests/Processors/RunSpecificTestsArgumentProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public RunSpecificTestsArgumentProcessorTests()
_mockAssemblyMetadataProvider = new Mock<IAssemblyMetadataProvider>();
_inferHelper = new InferHelper(_mockAssemblyMetadataProvider.Object);
_mockAssemblyMetadataProvider.Setup(x => x.GetArchitecture(It.IsAny<string>())).Returns(Architecture.X64);
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameWork(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameworkName(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_mockFileHelper.Setup(fh => fh.Exists(_dummyTestFilePath)).Returns(true);
_mockFileHelper.Setup(x => x.GetCurrentDirectory()).Returns("");
_mockMetricsPublisher = new Mock<IMetricsPublisher>();
Expand Down
2 changes: 1 addition & 1 deletion test/vstest.console.UnitTests/Processors/RunTestsArgumentProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public RunTestsArgumentProcessorTests()
SetupMockExtensions();
_mockAssemblyMetadataProvider.Setup(a => a.GetArchitecture(It.IsAny<string>()))
.Returns(Architecture.X86);
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameWork(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_mockAssemblyMetadataProvider.Setup(x => x.GetFrameworkName(It.IsAny<string>())).Returns(new FrameworkName(Constants.DotNetFramework40));
_mockProcessHelper = new Mock<IProcessHelper>();
_mockAttachmentsProcessingManager = new Mock<ITestRunAttachmentsProcessingManager>();
_environment = new Mock<IEnvironment>();
Expand Down
Loading