Skip to content

Commit

Permalink
Merge pull request #383 from tonyhallett/improve-settings-exception-m…
Browse files Browse the repository at this point in the history 
…essages

improve exception messages
  • Loading branch information
@tonyhallett
tonyhallett authored Jan 18, 2024
2 parents dc69d25 + 5f05fe8 commit 6494314
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 48 deletions.
105 changes: 75 additions & 30 deletions FineCodeCoverageTests/CoverageProject_Settings_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using FineCodeCoverageTests.Test_helpers;
using Moq;
using NUnit.Framework;
using StructureMap.AutoMocking;
using System;
using System.Collections.Generic;
using System.IO;
Expand Down Expand Up @@ -142,13 +143,21 @@ public async Task Should_Return_Using_VsBuild_When_No_Labelled_PropertyGroup(boo

public class SettingsMerger_Tests
{
private AutoMoqer mocker;
private SettingsMerger settingsMerger;

[SetUp]
public void SetUp()
{
mocker = new AutoMoqer();
settingsMerger = mocker.Create<SettingsMerger>();
}
[Test]
public void Should_Use_Global_Settings_If_No_Project_Level_Or_FCC_Settings_Files()
{
var mockAppOptions = new Mock<IAppOptions>(MockBehavior.Strict);
var appOptions = mockAppOptions.Object;

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(appOptions, new List<XElement>(), null);

Assert.AreSame(appOptions, mergedSettings);
Expand All @@ -161,7 +170,6 @@ public void Should_Overwrite_GlobalOptions_Bool_Properties_From_Settings_File()
mockAppOptions.SetupSet(o => o.IncludeReferencedProjects = true);
var appOptions = mockAppOptions.Object;

var settingsMerger = new SettingsMerger(null);
var settingsFileElement = CreateIncludeReferencedProjectsElement(true);
var mergedSettings = settingsMerger.Merge(appOptions, new List<XElement> { settingsFileElement}, null);

Expand All @@ -176,7 +184,6 @@ public void Should_Overwrite_GlobalOptions_Bool_Properties_From_Settings_File_In
mockAppOptions.SetupAllProperties();
var appOptions = mockAppOptions.Object;

var settingsMerger = new SettingsMerger(null);
var settingsFileElementTop = CreateIncludeReferencedProjectsElement(!last);
var settingsFileElementLast = CreateIncludeReferencedProjectsElement(last);
var mergedSettings = settingsMerger.Merge(
Expand All @@ -196,7 +203,6 @@ public void Should_Overwrite_GlobalOptions_Bool_Properties_From_Project(bool las
mockAppOptions.SetupAllProperties();
var appOptions = mockAppOptions.Object;

var settingsMerger = new SettingsMerger(null);
var settingsFileElement = CreateIncludeReferencedProjectsElement(!last);
var projectElement = CreateIncludeReferencedProjectsElement(last);
var mergedSettings = settingsMerger.Merge(
Expand All @@ -221,7 +227,6 @@ public void Should_Overwrite_Int_Properties()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> {},
Expand All @@ -244,7 +249,6 @@ public void Should_Overwrite_Enum_Properties()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -267,7 +271,6 @@ public void Should_Overwrite_String_Properties()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -293,7 +296,6 @@ public void Should_Overwrite_String_Array_By_Default()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -319,7 +321,6 @@ public void Should_Overwrite_String_Array_DefaultMerge_False()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -345,7 +346,6 @@ public void Should_Overwrite_String_Array_DefaultMerge_True_Property_Merge_false
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -371,7 +371,6 @@ public void Should_Overwrite_String_Array_DefaultMerge_Not_Bool()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -397,7 +396,6 @@ public void Should_Merge_String_Array_If_DefaultMerge()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -423,7 +421,6 @@ public void Should_Merge_If_Property_Element_Merge()
</Root>
");

var settingsMerger = new SettingsMerger(null);
var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
Expand All @@ -433,6 +430,54 @@ public void Should_Merge_If_Property_Element_Merge()
Assert.AreEqual(new string[] { "global", "1", "2" }, appOptions.Exclude);
}

[Test]
public void Should_Log_Failed_To_Get_Setting_From_Project_Settings_Exception_And_Not_Throw()
{
var mockAppOptions = new Mock<IAppOptions>();
mockAppOptions.SetupAllProperties();
var appOptions = mockAppOptions.Object;
var element = XElement.Parse($@"
<Root>
<OpenCoverRegister>
DefaultX
</OpenCoverRegister>
</Root>
");

var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { },
element);

var mockLogger = mocker.GetMock<ILogger>();
mockLogger.Verify(logger => logger.Log("Failed to get 'OpenCoverRegister' setting from project settings", It.IsAny<Exception>()));
Assert.AreEqual(mergedSettings.OpenCoverRegister, OpenCoverRegister.Default);
}

[Test]
public void Should_Log_Failed_To_Get_Setting_From_Settings_File_Exception_And_Not_Throw()
{
var mockAppOptions = new Mock<IAppOptions>();
mockAppOptions.SetupAllProperties();
var appOptions = mockAppOptions.Object;
var element = XElement.Parse($@"
<Root>
<OpenCoverRegister>
DefaultX
</OpenCoverRegister>
</Root>
");

var mergedSettings = settingsMerger.Merge(
appOptions,
new List<XElement> { element},
null);

var mockLogger = mocker.GetMock<ILogger>();
mockLogger.Verify(logger => logger.Log("Failed to get 'OpenCoverRegister' setting from settings file", It.IsAny<Exception>()));
Assert.AreEqual(mergedSettings.OpenCoverRegister, OpenCoverRegister.Default);
}

[Test]
public void Should_Not_Throw_If_Merge_Current_Null_String_Array_Type()
{
Expand Down Expand Up @@ -460,7 +505,7 @@ public void Should_Not_Throw_If_Merge_Current_Null_String_Array_Type()
}

[TestCaseSource(nameof(XmlConversionCases))]
public void Should_Convert_Xml_Value_Correctly(string propertyElement,string propertyName,object expectedConversion, bool expectedException)
public void Should_Convert_Xml_Value_Correctly(string propertyElement,string propertyName,object expectedConversion)
{
var settingsMerger = new SettingsMerger(new Mock<ILogger>().Object);
var settingsElement = XElement.Parse($"<Root>{propertyElement}</Root>");
Expand All @@ -474,10 +519,10 @@ public void Should_Convert_Xml_Value_Correctly(string propertyElement,string pro
[Test]
public void Should_Throw_For_Unsupported_Conversion()
{
var settingsMerger = new SettingsMerger(new Mock<ILogger>().Object);
var settingsElement = XElement.Parse($"<Root><PropertyType/></Root>");
var unsupported = typeof(PropertyInfo).GetProperty(nameof(PropertyInfo.PropertyType));
var expectedMessage = $"Cannot handle 'PropertyType' yet";

var expectedMessage = $"Unexpected settings type Type for setting PropertyType in settings merger GetValueFromXml";
Assert.Throws<Exception>(() => settingsMerger.GetValueFromXml(settingsElement, unsupported), expectedMessage);
}

Expand All @@ -503,31 +548,31 @@ string CreateElement(string elementName, string value)
var cases = new object[]
{
// boolean
new object[]{ CreateElement(hideFullyCovered, "true"),hideFullyCovered,true, false },
new object[]{ CreateElement(hideFullyCovered, "false"), hideFullyCovered, false, false },
new object[]{ CreateElement(hideFullyCovered, "bad"), hideFullyCovered, null, false },
new object[]{ CreateElement(hideFullyCovered, ""), hideFullyCovered, null, false },
new object[]{ CreateElement(hideFullyCovered, boolArray), hideFullyCovered, true, false },
new object[]{ CreateElement(hideFullyCovered, "true"),hideFullyCovered,true },
new object[]{ CreateElement(hideFullyCovered, "false"), hideFullyCovered, false },
new object[]{ CreateElement(hideFullyCovered, "bad"), hideFullyCovered, null },
new object[]{ CreateElement(hideFullyCovered, ""), hideFullyCovered, null },
new object[]{ CreateElement(hideFullyCovered, boolArray), hideFullyCovered, true },

// int
new object[]{ CreateElement(thresholdForCrapScore, "1"), thresholdForCrapScore, 1, false },
new object[]{ CreateElement(thresholdForCrapScore, "bad"), thresholdForCrapScore, null, false },
new object[]{ CreateElement(thresholdForCrapScore, ""), thresholdForCrapScore, null, false },
new object[]{ CreateElement(thresholdForCrapScore, "1"), thresholdForCrapScore, 1 },
new object[]{ CreateElement(thresholdForCrapScore, "bad"), thresholdForCrapScore, null },
new object[]{ CreateElement(thresholdForCrapScore, ""), thresholdForCrapScore, null },

// string
new object[]{ CreateElement(coverletConsoleCustomPath, "1"), coverletConsoleCustomPath, "1", false },
new object[]{ CreateElement(coverletConsoleCustomPath, "1"), coverletConsoleCustomPath, "1" },
// breaking change ( previous ignored )
new object[]{ CreateElement(coverletConsoleCustomPath, ""), coverletConsoleCustomPath, "", false },
new object[]{ CreateElement(coverletConsoleCustomPath, ""), coverletConsoleCustomPath, "" },

// string[]
new object[]{ CreateElement(exclude, stringArray), exclude, new string[] { "1","2"}, false },
new object[]{ CreateElement(exclude, ""), exclude, new string[] {}, false },
new object[]{ CreateElement(exclude, stringArray), exclude, new string[] { "1","2"} },
new object[]{ CreateElement(exclude, ""), exclude, new string[] {} },

// null for no property element
new object[]{ CreateElement(exclude, "true"), hideFullyCovered, null, false},
new object[]{ CreateElement(exclude, "true"), hideFullyCovered, null},

//exception for no type conversion
new object[]{ CreateElement(enumConversion, "No"), enumConversion, RunMsCodeCoverage.No, false }
new object[]{ CreateElement(enumConversion, "No"), enumConversion, RunMsCodeCoverage.No }

};

Expand Down
56 changes: 38 additions & 18 deletions SharedProject/Core/Model/SettingsMerger.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ private class SettingsElementDefaultMerge
{
public XElement SettingsElement { get; set; }
public bool DefaultMerge { get; set; }
public bool FromProjectSettings { get; internal set; }
}

private readonly PropertyInfo[] settingsPropertyInfos;
Expand All @@ -90,13 +91,27 @@ ILogger logger

}

public IAppOptions Merge(IAppOptions globalOptions, List<XElement> settingsFileElements, XElement projectSettingsElement)
public IAppOptions Merge(
IAppOptions globalOptions,
List<XElement> settingsFileElements,
XElement projectSettingsElement)
{
var settingsElementsWithDefaultMergeStrategy =
settingsFileElements.Select(e => new SettingsElementDefaultMerge { SettingsElement = e, DefaultMerge = settingsFileDefaultMerge }).ToList();
settingsFileElements.Select(e => new SettingsElementDefaultMerge {
SettingsElement = e,
DefaultMerge = settingsFileDefaultMerge,
FromProjectSettings = false
}).ToList();

if (projectSettingsElement != null)
{
settingsElementsWithDefaultMergeStrategy.Add(new SettingsElementDefaultMerge { SettingsElement = projectSettingsElement, DefaultMerge = projectSettingsDefaultMerge });
settingsElementsWithDefaultMergeStrategy.Add(
new SettingsElementDefaultMerge {
SettingsElement = projectSettingsElement,
DefaultMerge = projectSettingsDefaultMerge,
FromProjectSettings = true
}
);
}

if (settingsElementsWithDefaultMergeStrategy.Count != 0)
Expand All @@ -107,7 +122,11 @@ public IAppOptions Merge(IAppOptions globalOptions, List<XElement> settingsFileE
return globalOptions;
}

private void Merge(IAppOptions globalOptions, PropertyInfo settingPropertyInfo, List<SettingsElementDefaultMerge> settingsElementsWithDefaultMergeStrategy)
private void Merge(
IAppOptions globalOptions,
PropertyInfo settingPropertyInfo,
List<SettingsElementDefaultMerge> settingsElementsWithDefaultMergeStrategy
)
{
var canMerge = settingsMergeLogic.CanMerge(settingPropertyInfo.PropertyType);
if (canMerge)
Expand All @@ -119,29 +138,29 @@ private void Merge(IAppOptions globalOptions, PropertyInfo settingPropertyInfo,
var propertyElement = GetPropertyElement(settingsElement, settingPropertyInfo.Name);
if (propertyElement != null)
{
var fromProjectSettings = settingsElementWithDefaultMerge.FromProjectSettings;
var merge = GetMerge(defaultMerge, propertyElement);
if (merge)
{
Merge(globalOptions, settingPropertyInfo, settingsElement);
Merge(globalOptions, settingPropertyInfo, settingsElement,fromProjectSettings);
}
else
{
Overwrite(globalOptions, settingPropertyInfo, settingsElement);
Overwrite(globalOptions, settingPropertyInfo, settingsElement,fromProjectSettings);
}
}

}
}
else
{
var settingsElements = settingsElementsWithDefaultMergeStrategy.Select(x => x.SettingsElement);
Overwrite(globalOptions, settingPropertyInfo, settingsElements);
Overwrite(globalOptions, settingPropertyInfo, settingsElementsWithDefaultMergeStrategy);
}
}

private void Merge(IAppOptions globalOptions, PropertyInfo settingPropertyInfo, XElement settingsElement)
private void Merge(IAppOptions globalOptions, PropertyInfo settingPropertyInfo, XElement settingsElement,bool fromProjectSettings)
{
var value = TryGetValueFromXml(settingsElement, settingPropertyInfo);
var value = TryGetValueFromXml(settingsElement, settingPropertyInfo,fromProjectSettings);
if (value != null)
{
var currentValue = settingPropertyInfo.GetValue(globalOptions);
Expand Down Expand Up @@ -179,17 +198,17 @@ private bool GetDefaultMerge(bool defaultDefaultMerge, XElement root)
return defaultMergeAttribute.Value.ToLower() == "true";
}

private void Overwrite(IAppOptions globalOptions, PropertyInfo settingPropertyInfo, IEnumerable<XElement> settingsElements)
private void Overwrite(IAppOptions globalOptions, PropertyInfo settingPropertyInfo, IEnumerable<SettingsElementDefaultMerge> settingsElementsDefaultMerge)
{
foreach (var settingsElement in settingsElements)
foreach (var settingsElementDefaultMerge in settingsElementsDefaultMerge)
{
Overwrite(globalOptions, settingPropertyInfo, settingsElement);
Overwrite(globalOptions, settingPropertyInfo, settingsElementDefaultMerge.SettingsElement,settingsElementDefaultMerge.FromProjectSettings);
}
}

private void Overwrite(IAppOptions globalOptions, PropertyInfo settingPropertyInfo, XElement settingsElement)
private void Overwrite(IAppOptions globalOptions, PropertyInfo settingPropertyInfo, XElement settingsElement,bool fromProjectSettings)
{
var value = TryGetValueFromXml(settingsElement, settingPropertyInfo);
var value = TryGetValueFromXml(settingsElement, settingPropertyInfo,fromProjectSettings);
if (value != null)
{
settingPropertyInfo.SetValue(globalOptions, value);
Expand All @@ -201,15 +220,16 @@ private XElement GetPropertyElement(XElement settingsElement, string propertyNam
return settingsElement.Descendants().FirstOrDefault(x => x.Name.LocalName.Equals(propertyName, StringComparison.OrdinalIgnoreCase));
}

private object TryGetValueFromXml(XElement settingsElement, PropertyInfo property)
private object TryGetValueFromXml(XElement settingsElement, PropertyInfo property,bool fromProjectSettings)
{
try
{
return GetValueFromXml(settingsElement, property);
}
catch (Exception exception)
{
logger.Log($"Failed to override '{property.Name}' setting", exception);
var from = fromProjectSettings ? "project settings" : "settings file";
logger.Log($"Failed to get '{property.Name}' setting from {from}", exception);
}
return null;
}
Expand Down Expand Up @@ -372,7 +392,7 @@ internal object GetValueFromXml(XElement settingsElement, PropertyInfo property)

else
{
throw new Exception($"Cannot handle '{property.PropertyType.Name}' yet");
throw new Exception($"Unexpected settings type '{property.PropertyType.Name}' for setting {property.Name} in settings merger GetValueFromXml");
}
return null;

Expand Down

0 comments on commit 6494314

Please sign in to comment.