You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by conniey <gi...@git.apache.org> on 2016/07/15 16:32:34 UTC
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
GitHub user conniey opened a pull request:
https://github.com/apache/lucenenet/pull/172
Removing/limiting uses of static variables and methods in Testcases
This PR would allow an easier transition to using xUnit. xUnit by default, runs its tests in parallel and asynchronously. The current use of mutable static fields limits in test cases the ability to utilise this.
* Changing existing static variables to static readonly so we are explicit that these variables are set once and do not change.
* Change `OLD_FORMAT_IMPERSONATION_IS_ACTIVE` to non-static field since other tests share and use this variable
* Change `ClassEnvRule` to non-static since every test class generates its own rule during test
* Change dependent static methods in LuceneTestCase to non-static because their state depended on the state of these non-static variables
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/conniey/lucenenet staticLuceneTestCase
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucenenet/pull/172.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #172
----
commit 93acfd936b64d602e3e59d56c9d4b53e2820214b
Author: Connie Yau <co...@microsoft.com>
Date: 2016-07-11T23:33:47Z
Changing static variables to static readonly
commit 655fa1a02f325551936c7b0388963b05204d296a
Author: Connie Yau <co...@microsoft.com>
Date: 2016-07-12T12:26:26Z
Changing OLD_FORMAT_IMPERSONATION_IS_ACTIVE to non-static property
commit d0e3bfbb791ddd12eaf60bdc2a501df85a1c29d9
Author: Connie Yau <co...@microsoft.com>
Date: 2016-07-12T12:32:51Z
Changing NewField methods to non-static
commit d45358a5faff8ab04a0613750684ba0ced992af6
Author: Connie Yau <co...@microsoft.com>
Date: 2016-07-12T14:06:47Z
Fixing errors from making NewField and OLD_FORMAT_IMPERSONATION_IS_ACTIVE non-static
commit 6cc02fc6468dca463bfb2c63020f6ce377601638
Author: Connie Yau <co...@microsoft.com>
Date: 2016-07-12T14:13:53Z
Making ClassEnvRule non-static
commit e757edfecd638bd1ec78a0175efec90b7e57d259
Author: Connie Yau <co...@microsoft.com>
Date: 2016-07-13T22:19:06Z
Making NewSearcher to a non-static method
commit b94adfeb96f0c8ebe934ba37a89f16b68bb9dc8b
Author: Connie Yau <co...@microsoft.com>
Date: 2016-07-15T16:20:45Z
Fixing test errors with teardown/setup
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71072042
--- Diff: src/Lucene.Net.Tests.Queries/TestCustomScoreQuery.cs ---
@@ -334,11 +334,11 @@ private void VerifyResults(float boost, IndexSearcher s, IDictionary<int, float>
assertEquals("queries should have same #hits", h1.Count, h4CustomAdd.Count);
assertEquals("queries should have same #hits", h1.Count, h5CustomMulAdd.Count);
- QueryUtils.Check(Random(), q1, s, Rarely());
- QueryUtils.Check(Random(), q2, s, Rarely());
- QueryUtils.Check(Random(), q3, s, Rarely());
- QueryUtils.Check(Random(), q4, s, Rarely());
- QueryUtils.Check(Random(), q5, s, Rarely());
+ QueryUtils.Check(Random(), q1, s, Rarely(), NewSearcher);
+ QueryUtils.Check(Random(), q1, s, Rarely(), NewSearcher);
--- End diff --
q2
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71071991
--- Diff: src/Lucene.Net.TestFramework/Search/QueryUtils.cs ---
@@ -442,15 +445,17 @@ private class CollectorAnonymousInnerClassHelper2 : Collector
private int[] LastDoc;
private AtomicReader[] LastReader;
private IList<AtomicReaderContext> Context;
+ private readonly Func<IndexReader, bool, IndexSearcher> NewSearcher;
- public CollectorAnonymousInnerClassHelper2(Query q, IndexSearcher s, float maxDiff, int[] lastDoc, AtomicReader[] lastReader, IList<AtomicReaderContext> context)
+ public CollectorAnonymousInnerClassHelper2(Query q, IndexSearcher s, float maxDiff, int[] lastDoc, AtomicReader[] lastReader, IList<AtomicReaderContext> context, Func<IndexReader, bool, IndexSearcher> newSearcher)
{
this.q = q;
this.s = s;
this.MaxDiff = maxDiff;
this.LastDoc = lastDoc;
this.LastReader = lastReader;
this.Context = context;
+ NewSearcher = newSearcher;
--- End diff --
NewSearcherFunc would be a more appropriate name I think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #172: Removing/limiting uses of static variables and methods...
Posted by conniey <gi...@git.apache.org>.
Github user conniey commented on the issue:
https://github.com/apache/lucenenet/pull/172
Closing this PR because I created an improved one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by conniey <gi...@git.apache.org>.
Github user conniey commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71186208
--- Diff: src/Lucene.Net.Tests/core/Index/TestBinaryDocValuesUpdates.cs ---
@@ -1109,25 +1109,55 @@ public virtual void TestUpdateBinaryDVFieldWithSameNameAsPostingField()
[Test]
public virtual void TestUpdateOldSegments()
{
- Codec[] oldCodecs = new Codec[] { new Lucene40RWCodec(), new Lucene41RWCodec(), new Lucene42RWCodec(), new Lucene45RWCodec() };
+ OLD_FORMAT_IMPERSONATION_IS_ACTIVE = true;
+ Codec[] oldCodecs = new Codec[] {
+ new Lucene40RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE),
+ new Lucene41RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE),
+ new Lucene42RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE),
+ new Lucene45RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE)
+ };
Directory dir = NewDirectory();
- bool oldValue = OLD_FORMAT_IMPERSONATION_IS_ACTIVE;
// create a segment with an old Codec
IndexWriterConfig conf = NewIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(Random()));
conf.SetCodec(oldCodecs[Random().Next(oldCodecs.Length)]);
- OLD_FORMAT_IMPERSONATION_IS_ACTIVE = true;
IndexWriter writer = new IndexWriter(dir, conf);
Document doc = new Document();
doc.Add(new StringField("id", "doc", Store.NO));
doc.Add(new BinaryDocValuesField("f", ToBytes(5L)));
writer.AddDocument(doc);
writer.Dispose();
+ dir.Dispose();
+
+ OLD_FORMAT_IMPERSONATION_IS_ACTIVE = false;
--- End diff --
I had forgotten to remove it when I was originally testing this test failure. I removed it now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71072025
--- Diff: src/Lucene.Net.Tests.Join/Lucene.Net.Tests.Join.csproj ---
@@ -31,7 +31,7 @@
</PropertyGroup>
<ItemGroup>
<Reference Include="nunit.framework, Version=2.6.3.13283, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77, processorArchitecture=MSIL">
- <HintPath>..\packages\NUnit.2.6.3\lib\nunit.framework.dll</HintPath>
+ <HintPath>..\..\packages\NUnit.2.6.3\lib\nunit.framework.dll</HintPath>
--- End diff --
can you confirm this was intentional?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by conniey <gi...@git.apache.org>.
Github user conniey commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71184894
--- Diff: src/Lucene.Net.TestFramework/Search/QueryUtils.cs ---
@@ -442,15 +445,17 @@ private class CollectorAnonymousInnerClassHelper2 : Collector
private int[] LastDoc;
private AtomicReader[] LastReader;
private IList<AtomicReaderContext> Context;
+ private readonly Func<IndexReader, bool, IndexSearcher> NewSearcher;
- public CollectorAnonymousInnerClassHelper2(Query q, IndexSearcher s, float maxDiff, int[] lastDoc, AtomicReader[] lastReader, IList<AtomicReaderContext> context)
+ public CollectorAnonymousInnerClassHelper2(Query q, IndexSearcher s, float maxDiff, int[] lastDoc, AtomicReader[] lastReader, IList<AtomicReaderContext> context, Func<IndexReader, bool, IndexSearcher> newSearcher)
{
this.q = q;
this.s = s;
this.MaxDiff = maxDiff;
this.LastDoc = lastDoc;
this.LastReader = lastReader;
this.Context = context;
+ NewSearcher = newSearcher;
--- End diff --
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71072173
--- Diff: src/Lucene.Net.TestFramework/Index/RandomIndexWriter.cs ---
@@ -85,24 +85,24 @@ public static IndexWriter MockIndexWriter(Directory dir, IndexWriterConfig conf,
/// <summary>
/// create a RandomIndexWriter with a random config: Uses TEST_VERSION_CURRENT and MockAnalyzer </summary>
- public RandomIndexWriter(Random r, Directory dir)
- : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, new MockAnalyzer(r)))
- {
- }
+ //public RandomIndexWriter(Random r, Directory dir)
+ // : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, new MockAnalyzer(r)))
+ //{
+ //}
/// <summary>
/// create a RandomIndexWriter with a random config: Uses TEST_VERSION_CURRENT </summary>
- public RandomIndexWriter(Random r, Directory dir, Analyzer a)
- : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, a))
- {
- }
+ //public RandomIndexWriter(Random r, Directory dir, Analyzer a)
+ // : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, a))
+ //{
+ //}
/// <summary>
/// create a RandomIndexWriter with a random config </summary>
- public RandomIndexWriter(Random r, Directory dir, LuceneVersion v, Analyzer a)
- : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, v, a))
- {
- }
+ //public RandomIndexWriter(Random r, Directory dir, LuceneVersion v, Analyzer a)
+ // : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, v, a))
+ //{
+ //}
--- End diff --
Why was removing these necessary? can't you just replace the calls to `LuceneTestCase.NewIndexWriterConfig` with just `new IndexWriterConfig` - or make that method threadsafe?
Since this PR makes us diverge from the original codebase, we want to try and minimize the amount of changes this will cause - and need to be maintained or commented properly so future updates / ports don't override it. By leaving the methods here the way they originally were (and fixing the thread safety issues they I assume pose) we can reduce the amount of changes in the PR significantlly. WDYT?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by conniey <gi...@git.apache.org>.
Github user conniey closed the pull request at:
https://github.com/apache/lucenenet/pull/172
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by conniey <gi...@git.apache.org>.
Github user conniey commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71185299
--- Diff: src/Lucene.Net.Tests.Join/Lucene.Net.Tests.Join.csproj ---
@@ -31,7 +31,7 @@
</PropertyGroup>
<ItemGroup>
<Reference Include="nunit.framework, Version=2.6.3.13283, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77, processorArchitecture=MSIL">
- <HintPath>..\packages\NUnit.2.6.3\lib\nunit.framework.dll</HintPath>
+ <HintPath>..\..\packages\NUnit.2.6.3\lib\nunit.framework.dll</HintPath>
--- End diff --
Yes. It was trying to find the nunit package in the `src` folder before instead of the repository root.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet issue #172: Removing/limiting uses of static variables and methods...
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on the issue:
https://github.com/apache/lucenenet/pull/172
Thanks @conniey , looks good! I left a few comments - the main idea is to minimize the amount of non-Java compatible changes we make and to document well the ones we have to make.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by conniey <gi...@git.apache.org>.
Github user conniey commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71185454
--- Diff: src/Lucene.Net.Tests.Queries/TestCustomScoreQuery.cs ---
@@ -334,11 +334,11 @@ private void VerifyResults(float boost, IndexSearcher s, IDictionary<int, float>
assertEquals("queries should have same #hits", h1.Count, h4CustomAdd.Count);
assertEquals("queries should have same #hits", h1.Count, h5CustomMulAdd.Count);
- QueryUtils.Check(Random(), q1, s, Rarely());
- QueryUtils.Check(Random(), q2, s, Rarely());
- QueryUtils.Check(Random(), q3, s, Rarely());
- QueryUtils.Check(Random(), q4, s, Rarely());
- QueryUtils.Check(Random(), q5, s, Rarely());
+ QueryUtils.Check(Random(), q1, s, Rarely(), NewSearcher);
+ QueryUtils.Check(Random(), q1, s, Rarely(), NewSearcher);
--- End diff --
Thanks! Good catch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by synhershko <gi...@git.apache.org>.
Github user synhershko commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71072078
--- Diff: src/Lucene.Net.Tests/core/Index/TestBinaryDocValuesUpdates.cs ---
@@ -1109,25 +1109,55 @@ public virtual void TestUpdateBinaryDVFieldWithSameNameAsPostingField()
[Test]
public virtual void TestUpdateOldSegments()
{
- Codec[] oldCodecs = new Codec[] { new Lucene40RWCodec(), new Lucene41RWCodec(), new Lucene42RWCodec(), new Lucene45RWCodec() };
+ OLD_FORMAT_IMPERSONATION_IS_ACTIVE = true;
+ Codec[] oldCodecs = new Codec[] {
+ new Lucene40RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE),
+ new Lucene41RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE),
+ new Lucene42RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE),
+ new Lucene45RWCodec(OLD_FORMAT_IMPERSONATION_IS_ACTIVE)
+ };
Directory dir = NewDirectory();
- bool oldValue = OLD_FORMAT_IMPERSONATION_IS_ACTIVE;
// create a segment with an old Codec
IndexWriterConfig conf = NewIndexWriterConfig(TEST_VERSION_CURRENT, new MockAnalyzer(Random()));
conf.SetCodec(oldCodecs[Random().Next(oldCodecs.Length)]);
- OLD_FORMAT_IMPERSONATION_IS_ACTIVE = true;
IndexWriter writer = new IndexWriter(dir, conf);
Document doc = new Document();
doc.Add(new StringField("id", "doc", Store.NO));
doc.Add(new BinaryDocValuesField("f", ToBytes(5L)));
writer.AddDocument(doc);
writer.Dispose();
+ dir.Dispose();
+
+ OLD_FORMAT_IMPERSONATION_IS_ACTIVE = false;
--- End diff --
why do you set this here? If I read the original code correctly, this isn't needed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] lucenenet pull request #172: Removing/limiting uses of static variables and ...
Posted by conniey <gi...@git.apache.org>.
Github user conniey commented on a diff in the pull request:
https://github.com/apache/lucenenet/pull/172#discussion_r71187179
--- Diff: src/Lucene.Net.TestFramework/Index/RandomIndexWriter.cs ---
@@ -85,24 +85,24 @@ public static IndexWriter MockIndexWriter(Directory dir, IndexWriterConfig conf,
/// <summary>
/// create a RandomIndexWriter with a random config: Uses TEST_VERSION_CURRENT and MockAnalyzer </summary>
- public RandomIndexWriter(Random r, Directory dir)
- : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, new MockAnalyzer(r)))
- {
- }
+ //public RandomIndexWriter(Random r, Directory dir)
+ // : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, new MockAnalyzer(r)))
+ //{
+ //}
/// <summary>
/// create a RandomIndexWriter with a random config: Uses TEST_VERSION_CURRENT </summary>
- public RandomIndexWriter(Random r, Directory dir, Analyzer a)
- : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, a))
- {
- }
+ //public RandomIndexWriter(Random r, Directory dir, Analyzer a)
+ // : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, LuceneTestCase.TEST_VERSION_CURRENT, a))
+ //{
+ //}
/// <summary>
/// create a RandomIndexWriter with a random config </summary>
- public RandomIndexWriter(Random r, Directory dir, LuceneVersion v, Analyzer a)
- : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, v, a))
- {
- }
+ //public RandomIndexWriter(Random r, Directory dir, LuceneVersion v, Analyzer a)
+ // : this(r, dir, LuceneTestCase.NewIndexWriterConfig(r, v, a))
+ //{
+ //}
--- End diff --
Is that sufficient to replace it with a `new IndexWriterConfig`? The original static method does a lot more set-up.... and since I made LuceneTestCase non-static (since it depends on the non-static variable ClassEnvRule), I wasn't sure how to incorporate this call.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---