lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 commented on a change in pull request #287: Implemenation of "System Properties" in .NET (addresses #254)
Date Fri, 15 May 2020 00:04:45 GMT

NightOwl888 commented on a change in pull request #287:
URL: https://github.com/apache/lucenenet/pull/287#discussion_r425083597



##########
File path: src/Lucene.Net.TestFramework/luceneTestSettings.json
##########
@@ -2,7 +2,8 @@
     "analysis.data.dir": "",
     "egothor.stemmer.charset": "UTF-8",
     "kuromoji.data.dir": "",
-    "lucene:version": "4.8.1",
+    "lucene.version": "4.8.1",

Review comment:
       In .NET, we do not use `lucene.version` as a system property. This is passed into `dotnet
build` via command line or set by creating a `version.json` file. Please remove it.
   
   Also, please comment out the one test that uses it with a comment `LUCENENET specific:
Not used because we are configuring version number via MSBuild property in .NET`.

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -698,6 +698,23 @@ public virtual void TearDown()
         [CLSCompliant(false)]
         protected IConfigurationBuilder configurationBuilder;
 
+        public static string TestSettingsFileNameJson = "luceneTestSettings.json";
+        public static string TestSettingsFileNameXml = "luceneTestSettings.xml";

Review comment:
       Please eliminate this
   
   Note that just because we are eliminating this, doesn't mean we should also eliminate the
related extension method. Leave it in place in case end users want to configure XML files
that way.

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -774,28 +791,29 @@ public virtual void BeforeClass()
             try
             {
 
-#if NETSTANDARD
-                string currentPath = System.AppContext.BaseDirectory;
-#else
-                string currentPath = AppDomain.CurrentDomain.BaseDirectory;
-#endif
+                //#if NETSTANDARD

Review comment:
       Please remove this commented code block.

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -698,6 +698,23 @@ public virtual void TearDown()
         [CLSCompliant(false)]
         protected IConfigurationBuilder configurationBuilder;
 
+        public static string TestSettingsFileNameJson = "luceneTestSettings.json";

Review comment:
       Please eliminate this

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -49,8 +50,18 @@ public virtual void DirectoryScanTest()
         [Test]
         public virtual void DirectoryCrawlTest()
         {
-            Assert.AreEqual(3, this.configurationBuilder.Sources.Count);
+            Assert.AreEqual(4, ((MicrosoftExtensionsConfigurationFactory)ConfigurationFactory).builder.Sources.Count);
             Assert.Pass();
         }
+
+        [Test]
+        public virtual void TestHashCodeReadProperty()
+        {
+            Assert.AreEqual(0xf6a5c420, (uint)StringHelper.Murmurhash3_x86_32(new BytesRef("foo"),
0));
+            // Hashes computed using murmur3_32 from https://code.google.com/p/pyfasthash
+            Assert.AreEqual(0xcd018ef6, (uint)StringHelper.Murmurhash3_x86_32(new BytesRef("foo"),
StringHelper.GOOD_FAST_HASH_SEED));
+        }
+
+

Review comment:
       Please create tests to test directory caching to ensure settings from one directory
don't bleed over into another directory.

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationFactory.cs
##########
@@ -0,0 +1,101 @@
+´╗┐using System;
+using System.Collections.Generic;
+using System.Security;
+using System.Text;
+using System.Threading;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Primitives;
+
+namespace Lucene.Net.Configuration
+{
+    public class DefaultConfigurationFactory : IConfigurationFactory
+    {
+        private readonly bool ignoreSecurityExceptionsOnRead;
+        private bool initialized = false;
+        protected object m_initializationLock = new object();
+        private IConfiguration configuration;
+
+        public DefaultConfigurationFactory(bool ignoreSecurityExceptionsOnRead)
+        {
+            this.ignoreSecurityExceptionsOnRead = ignoreSecurityExceptionsOnRead;
+        }
+
+        [CLSCompliant(false)]
+        public virtual IConfiguration CreateConfiguration()
+        {
+            return EnsureInitialized();
+        }
+
+        /// <summary>
+        /// Ensures the <see cref="Initialize"/> method has been called since the
+        /// last application start. This method is thread-safe.
+        /// </summary>
+        [CLSCompliant(false)]
+        protected IConfiguration EnsureInitialized()
+        {
+            return LazyInitializer.EnsureInitialized(ref this.configuration, ref this.initialized,
ref this.m_initializationLock, () =>
+            {
+                this.configuration = Initialize();
+                return this.configuration;
+            });
+        }
+
+        /// <summary>
+        /// Initializes the dependencies of this factory.
+        /// </summary>
+        [CLSCompliant(false)]
+        protected virtual IConfiguration Initialize()
+        {
+            return new DefaultConfiguration(this.ignoreSecurityExceptionsOnRead);
+        }
+    }
+
+    internal class DefaultConfiguration : IConfiguration

Review comment:
       Please add a configurable prefix parameter for all environment variables and default
it to "lucene:".

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -698,6 +698,23 @@ public virtual void TearDown()
         [CLSCompliant(false)]
         protected IConfigurationBuilder configurationBuilder;
 
+        public static string TestSettingsFileNameJson = "luceneTestSettings.json";
+        public static string TestSettingsFileNameXml = "luceneTestSettings.xml";
+        public static string[] TestSettingsCommandLineArgs = null;
+        [CLSCompliant(false)]
+        public static IConfigurationFactory ConfigurationFactory { get; set; } = new MicrosoftExtensionsConfigurationFactory(TestSettingsCommandLineArgs,
TestSettingsFileNameJson, TestSettingsFileNameXml);

Review comment:
       Please change to use the new `TestConfigurationFactory` class
   
   ```c#
   public static IConfigurationFactory ConfigurationFactory { get; set; } = new TestConfigurationFactory();
   ```

##########
File path: src/Lucene.Net.TestFramework/luceneTestSettings.json
##########
@@ -2,7 +2,8 @@
     "analysis.data.dir": "",

Review comment:
       Can we change these to a `:` without nesting them?
   
   Please rename this one to be `smartcn:data:dir` (or `smartcn.data.dir`), since we ended
up with nested default conventions here instead of a common directory, which didn't make sense
in .NET.

##########
File path: src/Lucene.Net/Support/Configuration/DefaultConfigurationFactory.cs
##########
@@ -36,7 +35,8 @@ protected IConfiguration EnsureInitialized()
         {
             return LazyInitializer.EnsureInitialized(ref this.configuration, ref this.initialized,
ref this.m_initializationLock, () =>
             {
-                return Initialize();
+                this.configuration = Initialize();

Review comment:
       Please leave the setting of the configuration field up to the [`LazyInitializer` class](https://docs.microsoft.com/en-us/dotnet/api/system.threading.lazyinitializer.ensureinitialized?view=netcore-3.1).
Do not set it here, just return the result of `Initialize()` and let the `ref this.configuration`
get set through `EnsureInitialized`.

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -698,6 +698,23 @@ public virtual void TearDown()
         [CLSCompliant(false)]
         protected IConfigurationBuilder configurationBuilder;

Review comment:
       Please eliminate this

##########
File path: src/Lucene.Net.TestFramework/Support/Configuration/MicrosoftExtensionsConfigurationFactory.cs
##########
@@ -24,26 +24,48 @@ namespace Lucene.Net.Configuration
 
     public class MicrosoftExtensionsConfigurationFactory : DefaultConfigurationFactory

Review comment:
       It occurred to me after analyzing the features available in NUnit that the caching
model is completely wrong if we will be loading a configuration per directory. We need to
adjust caching of `IConfiguration` to allow a cache for each test directory, which is at odds
for allowing the user to configure the test directory and builder. Instead we should be using
the `NUnit.Framework.TestContext` object to dictate this based on what test assembly is currently
running. Since the configuration loads all of the settings for a given directory (even if
some of them come from a parent directory), we can use the `NUnit.Framework.TestContext.CurrentContext.TestDirectory`
as a cache key for that directory. Any test running in a different directory should have its
own cache entry.
   
   Here is a `TestConfigurationFactory` that accounts for this caching and ensures the cache
key is derived from the current context of the NUnit test.
   
   ```c#
       public class TestConfigurationFactory : IConfigurationFactory
       {
           private readonly ConcurrentDictionary<string, IConfiguration> configurationCache
= new ConcurrentDictionary<string, IConfiguration>();
   
           public string JsonTestSettingsFileName { get; set; } = "lucene.testsettings.json";
   
           [CLSCompliant(false)]
           public IConfiguration CreateConfiguration()
           {
               // LUCENENET NOTE: We cache the configuration once per directory to ensure
that child and
               // sibling directories that have overridden settings are accounted for. Therefore,
the
               // following will each have their own contextual based configuration settings.
               //
               // C:\Projects\Foo\test\TestProject1
               // C:\Projects\Foo\test\TestProject2
               // C:\Projects\Foo\test\TestProject2\TestGroup1
               // C:\Projects\Foo\test\TestProject2\TestGroup2
               //
   
               string testDirectory =
   #if TESTFRAMEWORK_NUNIT
                   NUnit.Framework.TestContext.CurrentContext.TestDirectory;
   #elif NETSTANDARD
                   System.AppContext.BaseDirectory;
   #else
                   AppDomain.CurrentDomain.BaseDirectory;
   #endif
   
               return configurationCache.GetOrAdd(testDirectory, (key) =>
               {
                   return new ConfigurationBuilder()
                       .AddEnvironmentVariables(prefix: "lucene:") // Use a custom prefix
to only load Lucene.NET settings
                       .AddJsonFilesFromRootDirectoryTo(currentPath: key, JsonTestSettingsFileName)
   #if TESTFRAMEWORK_NUNIT
                       .AddNUnitTestRunSettings(testDirectory: key)
   #endif
                       .Build();
               });
           }
       }
   ```
   
   `AddNUnitTestRunSettings` is a new extension method that creates a new instance of a new
class named `NUnitTestRunSettingsConfigurationBuilder`. 
   
   Based on new information about [TestRunParameters](https://docs.microsoft.com/en-us/visualstudio/test/configure-unit-tests-by-using-a-dot-runsettings-file?view=vs-2019#testrunparameters)
being an established convention, please create a custom `IConfigurationProvider` that reads
settings from [`NUnit.Framework.TestContext.CurrentContext.Test.Parameters`](https://github.com/nunit/nunit/issues/720#issuecomment-118024033)
(or perhaps `NUnit.Framework.TestContext.Parameters`? - you need to investigate which is the
best option). I think the best way to implement `NUnitTestRunSettingsConfigurationBuilder`
would be to closely follow the implementation of the [`EnvironmentVariablesConfigurationProvider`](https://github.com/aspnet/Configuration/blob/master/src/Config.EnvironmentVariables/EnvironmentVariablesConfigurationProvider.cs),
which is also reading the settings from static state much like how we will read the settings
from NUnit. This will cover both the case of a `.runsettings` file or RunSettings parameters
that are passed to dotnet test/dotnet vstest via command line.
   
   Let's continue down the path of using a JSON file, if it is present, but lets change the
name to be `lucene.testsettings.json` to be more in line with the runsettings convention.
It would be best to have a file that "just works" when running tests as opposed to having
to supply the file name at the command line (as would be the case with a runsettings file),
so we can allow using this JSON file to make configuring settings more user-friendly. However,
TestRunSettings must have the final say because it can be configured via command line or via
file.
   
   > **NOTE:** It might be possible to use the `AddCommandLine(IConfigurationBuilder, IDictionary<string,
string>)` method instead of creating a custom configuration provider for NUnit. It would
require a method that converts an `NUnit.Framework.TestContext.CurrentContext` object (reading
TestRunSettings) into a dictionary of key value pairs to pass as the `IDictionary<string,
string>` parameter.

##########
File path: src/Lucene.Net.Tests.TestFramework/Configuration/TestSystemProperties.cs
##########
@@ -49,8 +50,18 @@ public virtual void DirectoryScanTest()
         [Test]
         public virtual void DirectoryCrawlTest()
         {
-            Assert.AreEqual(3, this.configurationBuilder.Sources.Count);
+            Assert.AreEqual(4, ((MicrosoftExtensionsConfigurationFactory)ConfigurationFactory).builder.Sources.Count);

Review comment:
       Please create a mock configuration to test the directory scanning, don't use actual
components to do it.

##########
File path: src/Lucene.Net.TestFramework/Lucene.Net.TestFramework.csproj
##########
@@ -65,12 +65,40 @@
     <PackageReference Include="System.Threading.Thread" Version="$(SystemThreadingThreadPackageVersion)"
/>
   </ItemGroup>
 
+  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="3.1.3" />

Review comment:
       Please use an MSBuild variable for version (configured in the `build/Dependencies.props`
file) for all `PackageReference` elements. Also, please follow the convention of configuring
`PackageReference` names in alphabetical order.

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -698,6 +698,23 @@ public virtual void TearDown()
         [CLSCompliant(false)]
         protected IConfigurationBuilder configurationBuilder;
 
+        public static string TestSettingsFileNameJson = "luceneTestSettings.json";
+        public static string TestSettingsFileNameXml = "luceneTestSettings.xml";
+        public static string[] TestSettingsCommandLineArgs = null;

Review comment:
       Please eliminate this

##########
File path: src/Lucene.Net.Tests.TestFramework/Lucene.Net.Tests.TestFramework.csproj
##########
@@ -41,12 +41,26 @@
   <Import Project="$(SolutionDir)build/TestReferences.Common.targets" />
 
   <ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp3.1' ">
-    <PackageReference Include="System.Net.Primitives" Version="$(SystemNetPrimitivesPackageVersion)"/>
+    <PackageReference Include="System.Net.Primitives" Version="$(SystemNetPrimitivesPackageVersion)"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="3.1.3" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="3.1.3"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables"
Version="3.1.3" />
+
   </ItemGroup>
 
   <ItemGroup Condition=" '$(TargetFramework)' == 'net48' ">
     <Reference Include="System" />
     <Reference Include="Microsoft.CSharp" />
+    <PackageReference Include="Microsoft.Extensions.Configuration" Version="3.1.3" />
+    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="3.1.3"
/>
+    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables"
Version="3.1.3" />
+
+  </ItemGroup>
+
+  <ItemGroup>
+    <None Update="luceneTestSettings.json">
+      <CopyToOutputDirectory>Always</CopyToOutputDirectory>

Review comment:
       Please ensure loading the config files works whether `CopyToOutputDirectory` is enabled
or not.

##########
File path: src/Lucene.Net/Support/Configuration/IConfigurationFactory.cs
##########
@@ -0,0 +1,35 @@
+´╗┐using Microsoft.Extensions.Configuration;
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace Lucene.Net.Configuration
+{
+    /*
+     * Licensed to the Apache Software Foundation (ASF) under one or more
+     * contributor license agreements.  See the NOTICE file distributed with
+     * this work for additional information regarding copyright ownership.
+     * The ASF licenses this file to You under the Apache License, Version 2.0
+     * (the "License"); you may not use this file except in compliance with
+     * the License.  You may obtain a copy of the License at
+     *
+     *     http://www.apache.org/licenses/LICENSE-2.0
+     *
+     * Unless required by applicable law or agreed to in writing, software
+     * distributed under the License is distributed on an "AS IS" BASIS,
+     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+     * See the License for the specific language governing permissions and
+     * limitations under the License.
+     */
+    public interface IConfigurationSettingsFactory

Review comment:
       Please remove this

##########
File path: src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
##########
@@ -692,6 +694,27 @@ public virtual void TearDown()
         public static IDocValuesFormatFactory DocValuesFormatFactory { get; set; } = new
TestDocValuesFormatFactory();
         public static IPostingsFormatFactory PostingsFormatFactory { get; set; } = new TestPostingsFormatFactory();
 
+        // Configuration builder for Microsoft Extensions Configuration
+        [CLSCompliant(false)]
+        protected IConfigurationBuilder configurationBuilder;
+
+        public static string TestSettingsFileNameJson = "luceneTestSettings.json";
+        public static string TestSettingsFileNameXml = "luceneTestSettings.xml";
+        public static string[] TestSettingsCommandLineArgs = null;
+        [CLSCompliant(false)]
+        public static IConfigurationFactory ConfigurationFactory { get; set; } = new MicrosoftExtensionsConfigurationFactory(TestSettingsCommandLineArgs,
TestSettingsFileNameJson, TestSettingsFileNameXml);
+#if NETSTANDARD

Review comment:
       Please remove this block




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message