-
Notifications
You must be signed in to change notification settings - Fork 324
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
Aborting test run when source and target frameworks/architectures are incompatible #1789
Changes from 2 commits
f60b214
0a8e968
ce5de2f
37fa219
75e2164
92fe52c
9943636
30ebd14
e6230ba
3fa02c1
ce59bb3
d7057e2
220c955
c9740ea
e4f233f
2e66caa
550b4cc
10bc3f3
ed39fbd
58b3dfa
bc8c120
e850846
b033b87
3d5b844
615fbac
35ba9e5
1ddc150
6bcc76c
7836605
53b609b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -612,18 +612,21 @@ public static bool TryGetFrameworkXml(XPathNavigator runSettingsNavigator, out s | |
/// Returns the sources matching the specified platform and framework settings. | ||
/// For incompatible sources, warning is added to incompatibleSettingWarning. | ||
/// </summary> | ||
public static IEnumerable<String> FilterCompatibleSources(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks, out String incompatibleSettingWarning) | ||
public static IEnumerable<String> FilterCompatibleSources(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks, out String incompatibleSettingWarning, out bool incompatibilityErrorFound) | ||
{ | ||
incompatibleSettingWarning = string.Empty; | ||
bool incompatibilityFound = false; | ||
incompatibilityErrorFound = false; | ||
List<String> compatibleSources = new List<String>(); | ||
StringBuilder warnings = new StringBuilder(); | ||
warnings.AppendLine(); | ||
bool incompatiblityFound = false; | ||
|
||
foreach (var source in sourcePlatforms.Keys) | ||
{ | ||
Architecture actualPlatform = sourcePlatforms[source]; | ||
Framework actualFramework = sourceFrameworks[source]; | ||
bool isSettingIncompatible = IsSettingIncompatible(actualPlatform, chosenPlatform, actualFramework, chosenFramework); | ||
incompatibilityErrorFound = IsFrameworkRuntimeIncompatible(actualFramework, chosenFramework) || IsPlatformArchitectureIncompatible(actualPlatform, chosenPlatform); | ||
if (isSettingIncompatible) | ||
{ | ||
string incompatiblityMessage; | ||
|
@@ -632,15 +635,14 @@ public static IEnumerable<String> FilterCompatibleSources(Architecture chosenPla | |
incompatiblityMessage = string.Format(CultureInfo.CurrentCulture, OMResources.SourceIncompatible, onlyFileName, actualFramework.Version, actualPlatform); | ||
|
||
warnings.AppendLine(incompatiblityMessage); | ||
incompatiblityFound = true; | ||
incompatibilityFound = true; | ||
} | ||
else | ||
{ | ||
compatibleSources.Add(source); | ||
} | ||
} | ||
|
||
if (incompatiblityFound) | ||
if (incompatibilityFound || incompatibilityErrorFound) | ||
{ | ||
incompatibleSettingWarning = string.Format(CultureInfo.CurrentCulture, OMResources.DisplayChosenSettings, chosenFramework, chosenPlatform, warnings.ToString(), multiTargettingForwardLink); | ||
} | ||
|
@@ -685,5 +687,35 @@ private static bool IsFrameworkIncompatible(Framework sourceFramework, Framework | |
} | ||
return !sourceFramework.Name.Equals(targetFramework.Name, StringComparison.OrdinalIgnoreCase); | ||
} | ||
|
||
/// <summary> | ||
/// Returns true if source platform is incompatible with target platform. | ||
/// </summary> | ||
private static bool IsPlatformArchitectureIncompatible(Architecture sourcePlatform, Architecture targetPlatform) | ||
{ | ||
if (sourcePlatform == Architecture.X86 && targetPlatform == Architecture.X64 || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we check not equals for source and target platform? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are different platforms(X86, X64, ARM, AnyCPU) |
||
sourcePlatform == Architecture.X64 && targetPlatform == Architecture.X86) | ||
{ | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
|
||
/// <summary> | ||
/// Returns true if source Framework Runtime is incompatible with target Framework. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Framework and runtime are two different concepts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed |
||
/// </summary> | ||
private static bool IsFrameworkRuntimeIncompatible(Framework sourceFramework, Framework targetFramework) | ||
{ | ||
string sourceFrameworkName = sourceFramework.Name.Split(',')[0]; | ||
string targetFrameworkName = targetFramework.Name.Split(',')[0]; | ||
|
||
if(sourceFrameworkName.Equals(".NETFramework",StringComparison.OrdinalIgnoreCase) && targetFrameworkName.Equals(".NETCoreApp", StringComparison.OrdinalIgnoreCase) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens when we have NetStandard ? #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NetStandard will have no incompatibility with any of the other frameworks(NETFrameworks and NETCore). |
||
sourceFrameworkName.Equals(".NETCoreApp", StringComparison.OrdinalIgnoreCase) && targetFrameworkName.Equals(".NETFramework", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
return true; | ||
} | ||
return false; | ||
} | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -718,4 +718,7 @@ | |
<data name="InvalidLoggerArgument" xml:space="preserve"> | ||
<value>Logger argument '{0}' is not valid.</value> | ||
</data> | ||
<data name="ConflictInFrameworkPlatform" xml:space="preserve"> | ||
<value>Conflicts in framework/platform identifier of provided sources.</value> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change this to a more actionable message. @PBoraMSFT @pvlakshm Can you please approve. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @PBoraMSFT @pvlakshm Is this message "The given test sources target multiple frameworks/platforms that are incompatible. Please make sure the sources target the same framework and platform" okay? |
||
</data> | ||
</root> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -363,12 +363,17 @@ private bool UpdateRunSettingsIfRequired(string runsettingsXml, List<string> sou | |
|
||
var navigator = document.CreateNavigator(); | ||
|
||
var inferedFramework = inferHelper.AutoDetectFramework(sources, sourceFrameworks); | ||
var inferedFramework = inferHelper.AutoDetectFramework(sources, sourceFrameworks, out var isFrameworkIncompatible); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TryGetCompatibleFramework, and return true/false based on the detection #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Inferred #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How should be the method signature? |
||
Framework chosenFramework; | ||
var inferedPlatform = inferHelper.AutoDetectArchitecture(sources, sourcePlatforms); | ||
var inferedPlatform = inferHelper.AutoDetectArchitecture(sources, sourcePlatforms, out var isPlatformIncompatible); | ||
Architecture chosenPlatform; | ||
|
||
// Update frmaework and platform if required. For commandline scenario update happens in ArgumentProcessor. | ||
if (isFrameworkIncompatible || isPlatformIncompatible) | ||
{ | ||
throw new TestPlatformException(Resources.ConflictInFrameworkPlatform); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have separate messages? It just makes it clear which exactly caused the failure #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the message in the description #Resolved |
||
} | ||
|
||
// Update framework and platform if required. For commandline scenario update happens in ArgumentProcessor. | ||
bool updateFramework = IsAutoFrameworkDetectRequired(navigator, out chosenFramework); | ||
bool updatePlatform = IsAutoPlatformDetectRequired(navigator, out chosenPlatform); | ||
|
||
|
@@ -386,11 +391,15 @@ private bool UpdateRunSettingsIfRequired(string runsettingsXml, List<string> sou | |
settingsUpdated = true; | ||
} | ||
|
||
var compatibleSources = InferRunSettingsHelper.FilterCompatibleSources(chosenPlatform, chosenFramework, sourcePlatforms, sourceFrameworks, out var incompatibleSettingWarning); | ||
var compatibleSources = InferRunSettingsHelper.FilterCompatibleSources(chosenPlatform, chosenFramework, sourcePlatforms, sourceFrameworks, out var incompatibleSettingWarning, out var incompatibiltyErrorFound); | ||
|
||
if (!string.IsNullOrEmpty(incompatibleSettingWarning)) | ||
{ | ||
EqtTrace.Info(incompatibleSettingWarning); | ||
if(incompatibiltyErrorFound) | ||
{ | ||
throw new TestPlatformException(incompatibleSettingWarning); | ||
} | ||
ConsoleLogger.RaiseTestRunWarning(incompatibleSettingWarning); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid multiple outs ?
It just makes it unreadable and confusing.
Ideally, we should change this method to TryFilterCompatibleSources or Throw an exception and catch it in the caller. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way is to leave this method as it is and using
public static bool TryGetSettingIncompatibility(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks)
to return if there is any incompatility found. #ResolvedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split it into two functions as suggested
TryGetSettingIncompatibility(Architecture chosenPlatform, Framework chosenFramework, IDictionary<String, Architecture> sourcePlatforms, IDictionary<String, Framework> sourceFrameworks)
#Resolved