From f1c4d0baf886078e9a02babcff728d112c3b57be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20C=C3=A1ceres?= Date: Sat, 19 Aug 2023 20:41:37 +0200 Subject: [PATCH] Disable logging by default in `Configuration.cs` (#362) - Replace `GeneralSettings.DisableLogging` with `GeneralSettings.EnableLogging` and set it to `false` in `Configuration.cs` - Map it manually on startup to avoid https://github.com/dotnet/runtime/issues/89732 This should avoid the issue of the binary crashing without `appsettings.json`. **This doesn't mean it should be used without it**, it will swallow all errors and arnings, but yeah, you can. - Add test to ensure that `appsettings.json` are `Configuration.cs` are sync'ed - Adjust `appsettings` values to Config ones --- .github/workflows/ci.yml | 34 +++++++++++++++- src/Lynx.Cli/Program.cs | 13 +++++-- src/Lynx.Cli/appsettings.json | 6 +-- src/Lynx/Configuration.cs | 4 +- tests/Lynx.Test/Categories.cs | 14 +++++++ tests/Lynx.Test/ConfigurationTest.cs | 58 ++++++++++++++++++++++++++++ tests/Lynx.Test/Lynx.Test.csproj | 3 +- 7 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 tests/Lynx.Test/ConfigurationTest.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8cf26dff3..dedd4c005 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -147,7 +147,7 @@ jobs: - name: Build run: dotnet build -c ${{ matrix.configuration }} /p:DeterministicBuild=true - - name: Run CI tests + - name: Run ${{ matrix.category }} tests run: dotnet test -c ${{ matrix.configuration }} --no-build --collect:"XPlat Code Coverage" -v normal - name: '[Ubuntu] Generate test coverage report' @@ -239,6 +239,38 @@ jobs: run: dotnet test -c Release --no-build --filter "TestCategory=${{ matrix.category }}" -v normal # --collect:"XPlat Code Coverage" https://github.com/coverlet-coverage/coverlet/issues/1192 + other-tests: + runs-on: ${{ matrix.os }} + + strategy: + matrix: + os: [ubuntu-latest, windows-latest, macOS-latest] + category: [Configuration] + fail-fast: false + + steps: + - uses: actions/checkout@v3 + + - name: Setup .NET + uses: actions/setup-dotnet@v3 + with: + dotnet-version: ${{ env.DOTNET_VERSION }} + + - name: Nuget cache + uses: actions/cache@v3 + with: + path: + ~/.nuget/packages + key: ${{ runner.os }}-nuget-${{ hashFiles('**/packages.lock.json') }} + restore-keys: | + ${{ runner.os }}-nuget- + + - name: Build + run: dotnet build -c Release + + - name: Run ${{ matrix.category }} tests + run: dotnet test -c Release --no-build --filter "TestCategory=${{ matrix.category }}" -v normal --collect:"XPlat Code Coverage" + winning-at-chess: runs-on: ${{ matrix.os }} diff --git a/src/Lynx.Cli/Program.cs b/src/Lynx.Cli/Program.cs index 7f3d409f1..26d8f7700 100644 --- a/src/Lynx.Cli/Program.cs +++ b/src/Lynx.Cli/Program.cs @@ -20,10 +20,17 @@ .AddEnvironmentVariables() .Build(); -config.GetRequiredSection(nameof(EngineSettings)).Bind(Configuration.EngineSettings); -config.GetRequiredSection(nameof(GeneralSettings)).Bind(Configuration.GeneralSettings); +config.GetSection(nameof(EngineSettings)).Bind(Configuration.EngineSettings); +config.GetSection(nameof(GeneralSettings)).Bind(Configuration.GeneralSettings); -if (!Configuration.GeneralSettings.DisableLogging) +// TODO remove when .NET sdk includes https://github.com/dotnet/runtime/issues/89732 +var generalConfig = config.GetSection(nameof(GeneralSettings)); +if (bool.TryParse(generalConfig[nameof(Configuration.GeneralSettings.EnableLogging)], out var enableLogging)) +{ + Configuration.GeneralSettings.EnableLogging = enableLogging; +} + +if (Configuration.GeneralSettings.EnableLogging) { LogManager.Configuration = new NLogLoggingConfiguration(config.GetSection("NLog")); } diff --git a/src/Lynx.Cli/appsettings.json b/src/Lynx.Cli/appsettings.json index f7c6cb53f..7cd127489 100644 --- a/src/Lynx.Cli/appsettings.json +++ b/src/Lynx.Cli/appsettings.json @@ -1,7 +1,7 @@ { // Settings that affect the executable behavior "GeneralSettings": { - "DisableLogging": false // Completely disables logging, both console and file. Alternatively, one or more "NLog.rules" can be removed/tweaked + "EnableLogging": true // logging can be completely disablesd, both console and file, setting this to false. Alternatively, one or more "NLog.rules" can be removed/tweaked }, // Settings that affect the engine behavior @@ -9,7 +9,7 @@ "DefaultMaxDepth": 5, "CoefficientBeforeKeyMovesBeforeMovesToGo": 1.5, "KeyMovesBeforeMovesToGo": 10, - "CoefficientAfterKeyMovesBeforeMovesToGo": 0.9, + "CoefficientAfterKeyMovesBeforeMovesToGo": 0.95, "TotalMovesWhenNoMovesToGoProvided": 100, "FixedMovesLeftWhenNoMovesToGoProvidedAndOverTotalMovesWhenNoMovesToGoProvided": 20, "FirstTimeLimitWhenNoMovesToGoProvided": 120000, @@ -17,7 +17,7 @@ "SecondTimeLimitWhenNoMovesToGoProvided": 30000, "SecondCoefficientWhenNoMovesToGoProvided": 2, "MinSecurityTime": 1000, - "CoefficientSecurityTime": 0.95, + "CoefficientSecurityTime": 0.90, "MinDepth": 4, "MaxDepth": 128, //"MinMoveTime": 1000, diff --git a/src/Lynx/Configuration.cs b/src/Lynx/Configuration.cs index 30d462890..83160ca36 100644 --- a/src/Lynx/Configuration.cs +++ b/src/Lynx/Configuration.cs @@ -75,7 +75,7 @@ public static int Hash public sealed class GeneralSettings { - public bool DisableLogging { get; set; } = false; + public bool EnableLogging { get; set; } = false; } public sealed class EngineSettings @@ -133,7 +133,7 @@ public sealed class EngineSettings #endregion /// - /// Min. time left in the clock if all decision time is used befire is used over that decision time + /// Min. time left in the clock if all decision time is used before is used over that decision time /// public int MinSecurityTime { get; set; } = 1_000; diff --git a/tests/Lynx.Test/Categories.cs b/tests/Lynx.Test/Categories.cs index 7081c9c40..9fad3adc1 100644 --- a/tests/Lynx.Test/Categories.cs +++ b/tests/Lynx.Test/Categories.cs @@ -6,9 +6,23 @@ public static class Categories public const string LongRunning = "LongRunning"; + /// + /// Need to run in isolation, since other tests might modify values + /// + public const string Configuration = "Configuration"; + + /// + /// Can't be run since it'd take way too long for regular CI + /// public const string TooLong = "TooLongToBeRun"; + /// + /// Can't be run since no prunning is required + /// public const string NoPruning = "RequireNoPruning"; + /// + /// Can't be run since our engine is simply not good enough yet + /// public const string NotGoodEnough = "NotGoodEnough"; } diff --git a/tests/Lynx.Test/ConfigurationTest.cs b/tests/Lynx.Test/ConfigurationTest.cs new file mode 100644 index 000000000..75b6a8757 --- /dev/null +++ b/tests/Lynx.Test/ConfigurationTest.cs @@ -0,0 +1,58 @@ +using Microsoft.Extensions.Configuration; +using NUnit.Framework; +using System.Text.Json; +using System.Text.Json.Nodes; + +namespace Lynx.Test; + +[Explicit] +[Category(Categories.Configuration)] +[NonParallelizable] +public class ConfigurationTest +{ + [Test] + public void SynchronizedAppSettings() + { + var config = new ConfigurationBuilder() + .AddJsonFile("appsettings.json", optional: true, reloadOnChange: false) + .Build(); + + var engineSettingsSection = config.GetRequiredSection(nameof(EngineSettings)); + Assert.IsNotNull(engineSettingsSection); + +#pragma warning disable IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code + var serializedEngineSettingsConfig = JsonSerializer.Serialize(Configuration.EngineSettings); + var jsonNode = JsonSerializer.Deserialize(serializedEngineSettingsConfig); +#pragma warning restore IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code + Assert.IsNotNull(jsonNode); + +#pragma warning disable IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code - using sourcegenerator + engineSettingsSection.Bind(Configuration.EngineSettings); +#pragma warning restore IL2026 // Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code + + var reflectionProperties = Configuration.EngineSettings.GetType().GetProperties( + System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance); + Assert.GreaterOrEqual(reflectionProperties.Length, 35); + + var originalCulture = Thread.CurrentThread.CurrentCulture; + Thread.CurrentThread.CurrentCulture = System.Globalization.CultureInfo.InvariantCulture; + + Assert.Multiple(() => + { + foreach (var property in reflectionProperties) + { + if (property.PropertyType == typeof(int[])) + { + continue; + } + + var sourceSetting = jsonNode![property.Name]!.ToString().ToLowerInvariant(); + var configSetting = property.GetValue(Configuration.EngineSettings)!.ToString()!.ToLowerInvariant(); + + Assert.AreEqual(sourceSetting, configSetting, $"Error in {property.Name} ({property.PropertyType}): (Configuration.cs) {sourceSetting} != {configSetting} (appSettings.json)"); + } + + Thread.CurrentThread.CurrentCulture = originalCulture; + }); + } +} diff --git a/tests/Lynx.Test/Lynx.Test.csproj b/tests/Lynx.Test/Lynx.Test.csproj index dbaa67057..45b7413db 100644 --- a/tests/Lynx.Test/Lynx.Test.csproj +++ b/tests/Lynx.Test/Lynx.Test.csproj @@ -2,6 +2,7 @@ false + true @@ -16,7 +17,7 @@ - +