NightOwl888 commented on code in PR #914:
URL: https://github.com/apache/lucenenet/pull/914#discussion_r1493331959
##########
src/Lucene.Net.Tests/Search/Spans/TestSpans.cs:
##########
@@ -320,7 +373,8 @@ private void TstNextSpans(Spans spans, int doc, int start,
int end)
[Test]
public virtual void TestSpanOrEmpty()
{
- Spans spans = OrSpans(new string[0]);
+ // LUCENENET: Using Array.Empty<T>() instead of new string[0] to
reduce allocations
+ Spans spans = OrSpans(Array.Empty<string>());
Review Comment:
`Array.Empty<T>` was added in .NET 4.6. We only recently changed our minimum
target framework from `net45` to `net462`. As a result, all existing code is
using `Arrays.Empty<T>` instead of `Array.Empty<T>`.
We should probably keep using `Arrays.Empty<T>` consistently until there is
a concerted effort to change all of these calls to `Array.Empty<T>`. No
objections if you wish to open a new issue for this task, but it goes beyond
the scope of this PR. The task would also involve removing the
`Arrays.Empty<T>` method, backing static class, and `FEATURE_ARRAYEMPTY` from
the codebase.
##########
src/Lucene.Net.Tests/Search/Spans/TestSpans.cs:
##########
@@ -556,6 +609,7 @@ public virtual void TestSpanNots()
[Test]
[Description("LUCENENET-597")]
+ [LuceneNetSpecific]
Review Comment:
Any ideas what the string returned here is supposed to represent?
Java: `spanNear([f:lucene, f:net, f:solr], 0, true)`
.NET: `SpanNear([f:lucene, f:net, f:solr], 0, True)`
This looks like something that may be copied and pasted somewhere to me, but
it may just be a textual description. If the former, we should probably at
least strive to match the case of the boolean. I am guessing the name is the
correct casing for .NET, though.
Of course, we are probably using upper case for all booleans converted to
string in .NET, because that is what .NET does by default. So, whatever the
fix, it should be applied consistently in all queries.
##########
src/Lucene.Net.Tests/Search/BaseTestRangeFilter.cs:
##########
@@ -208,8 +198,8 @@ public virtual void TestPad()
string bb = Pad(b);
string label = a + ":" + aa + " vs " + b + ":" + bb;
Assert.AreEqual(aa.Length, bb.Length, "i=" + i + ": length of
" + label);
- Assert.IsTrue(System.String.Compare(aa, bb,
System.StringComparison.Ordinal) < 0, "i=" + i + ": compare less than " +
label);
+ Assert.IsTrue(string.Compare(aa, bb, StringComparison.Ordinal)
< 0, "i=" + i + ": compare less than " + label);
Review Comment:
Just FYI - there is a `CompareToOrdinal()` extension method that is a
shortcut to `string.Compare(aa, bb, StringComparison.Ordinal)`. Not critical
that we use it everywhere, though.
##########
src/Lucene.Net.Tests/Search/Spans/TestSpansAdvanced.cs:
##########
@@ -79,16 +82,9 @@ public override void SetUp()
[TearDown]
public override void TearDown()
{
- if (reader != null)
- {
- reader.Dispose();
- }
-
- if (mDirectory != null)
- {
- mDirectory.Dispose();
- mDirectory = null;
- }
+ reader.Dispose();
+ mDirectory.Dispose();
Review Comment:
Please change this to `mDirectory?.Dispose();`.
##########
src/Lucene.Net.Tests/Search/Spans/TestSpansAdvanced.cs:
##########
@@ -79,16 +82,9 @@ public override void SetUp()
[TearDown]
public override void TearDown()
{
- if (reader != null)
- {
- reader.Dispose();
- }
-
- if (mDirectory != null)
- {
- mDirectory.Dispose();
- mDirectory = null;
- }
+ reader.Dispose();
Review Comment:
Please change this to `reader?.Dispose();`. We may bomb before reaching the
line to create the reader and this will hide the original failure message.
##########
src/Lucene.Net.Tests/Search/TestBooleanOr.cs:
##########
@@ -200,9 +200,9 @@ public virtual void TestBooleanScorerMax()
FixedBitSet hits = new FixedBitSet(docCount);
AtomicInt32 end = new AtomicInt32();
- ICollector c = new CollectorAnonymousClass(this, scorer, hits,
end);
+ ICollector c = new CollectorAnonymousClass(hits, end);
- while (end < docCount)
+ while (end.Value < docCount)
Review Comment:
The implicit operator is overloaded here, so calling `.Value` is
unnecessary. However, a case could be made either way as to which is more
readable (this aligns more closely with Java).
##########
src/Lucene.Net.Tests/Search/TestSimpleExplanationsOfNonMatches.cs:
##########
@@ -28,10 +26,10 @@ public class TestSimpleExplanationsOfNonMatches :
TestSimpleExplanations
/// <summary>
/// Overrides superclass to ignore matches and focus on non-matches
/// </summary>
- /// <seealso cref= CheckHits#checkNoMatchExplanations </seealso>
+ /// <seealso
cref="Lucene.Net.Search.CheckHits.CheckNoMatchExplanations" />
Review Comment:
`CheckHits` unnecessarily calls `Convert.ToInt32(object?, CultureInfo?)` in
5 places, causing boxing. Please fix. In these cases, we don't need the method
call at all because we are converting `int` to `int`.
##########
src/Lucene.Net.Tests/Search/TestScorerPerf.cs:
##########
@@ -63,34 +63,35 @@ public virtual void CreateDummySearcher()
s = NewSearcher(r);
}
- public virtual void CreateRandomTerms(int nDocs, int nTerms, double
power, Directory dir)
- {
- int[] freq = new int[nTerms];
- terms = new Term[nTerms];
- for (int i = 0; i < nTerms; i++)
- {
- int f = (nTerms + 1) - i; // make first terms less frequent
- freq[i] = (int)Math.Ceiling(Math.Pow(f, power));
- terms[i] = new Term("f", char.ToString((char)('A' + i)));
- }
-
- IndexWriter iw = new IndexWriter(dir,
NewIndexWriterConfig(TEST_VERSION_CURRENT, new
MockAnalyzer(Random)).SetOpenMode(OpenMode.CREATE));
- for (int i = 0; i < nDocs; i++)
- {
- Document d = new Document();
- for (int j = 0; j < nTerms; j++)
- {
- if (Random.Next(freq[j]) == 0)
- {
- d.Add(NewStringField("f", terms[j].Text,
Field.Store.NO));
- //System.out.println(d);
- }
- }
- iw.AddDocument(d);
- }
- iw.ForceMerge(1);
- iw.Dispose();
- }
+ // LUCENENET: unused so commented out here, only used by commented-out
code below
+ // public virtual void CreateRandomTerms(int nDocs, int nTerms, double
power, Directory dir)
+ // {
+ // int[] freq = new int[nTerms];
+ // terms = new Term[nTerms];
Review Comment:
`internal Term[] terms` is now generating warnings because it is unused.
Please fix.
##########
src/Lucene.Net.Tests/Support/Threading/TestUninterruptableMonitor.cs:
##########
@@ -33,7 +33,7 @@ public class TestUninterruptableMonitor : LuceneTestCase
{
private class TransactionlThreadInterrupt : ThreadJob
Review Comment:
Please correct the class name. It should be `TransactionalThreadInterrupt`.
##########
src/Lucene.Net.Tests/Search/TestBooleanOr.cs:
##########
@@ -200,9 +200,9 @@ public virtual void TestBooleanScorerMax()
FixedBitSet hits = new FixedBitSet(docCount);
AtomicInt32 end = new AtomicInt32();
- ICollector c = new CollectorAnonymousClass(this, scorer, hits,
end);
+ ICollector c = new CollectorAnonymousClass(hits, end);
- while (end < docCount)
+ while (end.Value < docCount)
{
int inc = TestUtil.NextInt32(Random, 1, 1000);
end.AddAndGet(inc);
Review Comment:
In Java, this was `end.GetAndAdd(inc)`.
Since this was likely done because at one point the method didn't exist, we
should do a sweep to ensure that all calls to `GetAndAdd()`, `AddAndGet()`,
`IncrementAndGet()`, `GetAndIncrement()`, `DecrementAndGet()`, and
`GetAndDecrement()` match Lucene (for both `AtomicInt32` and `AtomicInt64`). We
may not have caught this everywhere it matters. This should be a separate task
outside of the scope of this PR, we just need a new issue about it.
##########
src/Lucene.Net.Tests/Search/TestCachingWrapperFilter.cs:
##########
@@ -207,29 +207,19 @@ public virtual void TestNullDocIdSet()
IndexReader reader =
SlowCompositeReaderWrapper.Wrap(DirectoryReader.Open(dir));
Review Comment:
The `IndexReader` constructor is:
https://github.com/apache/lucenenet/blob/b1476aee4fe21131899c1f43b2e06e25971b3ebe/src/Lucene.Net/Index/IndexReader.cs#L84-L90
We can remove the check and error message, since it is declared `private
protected`. But we will need a `LUCENENET specific` comment to indicate we
removed it in the .NET version and enforced it at the compiler level. There may
also be tests to confirm this exception is thrown.
This can be another issue that is outside of the scope of this PR.
##########
src/Lucene.Net.Tests/Search/TestBoolean2.cs:
##########
@@ -225,7 +229,7 @@ public virtual void TestQueries07()
query.Add(new TermQuery(new Term(field, "w3")), Occur.MUST_NOT);
query.Add(new TermQuery(new Term(field, "xx")), Occur.MUST_NOT);
query.Add(new TermQuery(new Term(field, "w5")), Occur.MUST_NOT);
- int[] expDocNrs = new int[] { };
+ int[] expDocNrs = Array.Empty<int>(); // LUCENENET: instead of new
int[] { };
Review Comment:
See my comment on TestSpans.cs
##########
src/Lucene.Net.Tests/Search/Spans/TestSpansAdvanced.cs:
##########
@@ -131,7 +127,7 @@ protected internal virtual void
DoTestBooleanQueryWithSpanQueries(IndexSearcher
/// <summary>
/// Checks to see if the hits are what we expected.
- ///
+ ///
/// LUCENENET specific
Review Comment:
This comment is out of date, and can be removed.
We can probably also go back to making this method static, since this was
only done to access the instance level Similarity property that no longer
exists.
##########
src/Lucene.Net.Tests/Search/TestBooleanQuery.cs:
##########
@@ -182,9 +182,6 @@ public virtual void TestDeMorgan()
IndexSearcher searcher = NewSearcher(multireader);
Assert.AreEqual(0, searcher.Search(query, 10).TotalHits);
-
- Task foo = new Task(TestDeMorgan);
-
TaskScheduler es = TaskScheduler.Default;
searcher = new IndexSearcher(multireader, es);
Review Comment:
Would it make sense to add an overload here that accepts a cancellation
token?
##########
src/Lucene.Net.Tests/Search/TestCachingCollector.cs:
##########
@@ -63,9 +67,14 @@ public override long GetCost()
private class NoOpCollector : ICollector
{
+ // LUCENENET: static instances to reduce allocations
+ public static readonly NoOpCollector FalseInstance = new
NoOpCollector(false);
Review Comment:
Due to the fact that this serves as a demo to users, it would not be a good
practice to make this static, since typical collectors will have state (even if
this one does not).
##########
src/Lucene.Net.Tests/Search/TestCachingCollector.cs:
##########
@@ -31,7 +31,11 @@ public class TestCachingCollector : LuceneTestCase
private class MockScorer : Scorer
{
- internal MockScorer()
+ // LUCENENET: static instance to reduce allocations
+ public static readonly MockScorer Default = new MockScorer();
Review Comment:
Due to the fact that this serves as a demo to users, it would not be a good
practice to make this static, since typical scorers will have state (even if
this one does not).
##########
src/Lucene.Net.Tests/Search/TestBooleanScorer.cs:
##########
@@ -125,13 +125,10 @@ public override bool Score(ICollector c, int maxDoc)
private sealed class CollectorAnonymousClass : ICollector
Review Comment:
Due to covariance issues with `Lucene.Net.Grouping`, we changed `Collector`
from an abstract class to an interface. However, we also have a static
`Collector` class that is only used to put constants that are not possible to
put on an interface in .NET. We should probably change that static class back
to an abstract class that implements `ICollector` and leave `ICollector` on all
of the methods throughout the codebase. This makes it easier for users who are
copying code from Java to implement a collector.
However, we need an analyzer to ensure that none of our internal code
actually accepts `Collector` as an argument.
This can be a new issue that is done outside of the scope of this PR.
Do note that we could potentially get rid of the `ICollector` interface
altogether if the `GroupingSearch` class were divided into 3 separate classes
instead of having main `Search` methods that deal with `object` closing types.
I would really like to get rid of the non-generic `IDictionary` and
`ICollection` usages in `Lucene.Net.Grouping` and `Lucene.Net.Queries` and
replace them with generics prior to the release. J2N doesn't fully support
non-generic collection interfaces, so it is best not to use them at all.
@rclabo did some analysis in this area and added some tests (as demos), so
he would be a big help to accomplish this.
##########
src/Lucene.Net.Tests/Search/TestCustomSearcherSort.cs:
##########
@@ -251,7 +245,7 @@ internal RandomGen(TestCustomSearcherSort outerInstance,
Random random)
// Just to generate some different Lucene Date strings
internal virtual string LuceneDate
- => DateTools.TimeToString((DateTime.Now.Ticks /
TimeSpan.TicksPerMillisecond) + random.Next() - int.MinValue,
DateResolution.DAY);
+ => DateTools.TimeToString((@base.Ticks /
TimeSpan.TicksPerMillisecond) + random.Next() - int.MinValue,
DateResolution.DAY);
Review Comment:
This is not correct to generate a date for ticks. The default now assumes to
use milliseconds since Unix epoch (as it is in Lucene). Please use:
```c#
internal virtual string LuceneDate
=> DateTools.TimeToString(DateTools.TicksToUnixTimeMilliseconds(@base) +
random.Next() - int.MinValue, DateResolution.DAY);
```
It is possible that the following would be equivalent, but it would need to
be tested to confirm:
```c#
internal virtual string LuceneDate
=> DateTools.TimeToString(J2N.Time.GetMillisecondsSinceUnixEpoch(@base)
+ random.Next() - int.MinValue, DateResolution.DAY);
```
```c#
internal virtual string LuceneDate
=> DateTools.TimeToString(@base.Ticks + random.Next() - int.MinValue,
DateResolution.DAY, NumericRepresentation.TICKS);
```
The `DateTools` representation for input `long` and output `long` is now
consistent. In Lucene.Net 3.x it was expecting ticks as milliseconds as input
and was outputting ticks. We have built in legacy support for
`TICKS_AS_MILLISECONDS`, but we shouldn't be using it in code going forward.
##########
src/Lucene.Net.Tests/Search/TestFieldCacheRangeFilter.cs:
##########
@@ -501,7 +501,7 @@ public virtual void TestFieldCacheRangeFilterFloats()
int numDocs = reader.NumDocs;
float minIdO = Convert.ToSingle(minId + .5f);
- float medIdO = Convert.ToSingle((float)minIdO + ((maxId - minId))
/ 2.0f);
+ float medIdO = Convert.ToSingle(minIdO + ((maxId - minId)) / 2.0f);
Review Comment:
FYI - we need to be careful to run these tests on x86 (.NET Framework) to
ensure this doesn't cause issues. I ran this 3x on Azure DevOps and it seems to
be okay. IIRC, there were issues with losing precision when leaving it up to
the compiler to do an implicit `float` > `double` cast, which is fixed by
casting to `double` explicitly.
##########
src/Lucene.Net.Tests/Search/TestComplexExplanationsOfNonMatches.cs:
##########
@@ -28,10 +26,10 @@ public class TestComplexExplanationsOfNonMatches :
TestComplexExplanations
/// <summary>
/// Overrides superclass to ignore matches and focus on non-matches
/// </summary>
- /// <seealso cref= CheckHits#checkNoMatchExplanations </seealso>
+ /// <seealso cref="CheckHits.CheckNoMatchExplanations" />
Review Comment:
Without specifying the parameters, this links to the wrong overload.
##########
src/Lucene.Net.Tests/Search/TestPhraseQuery.cs:
##########
@@ -63,22 +64,22 @@ public override void BeforeClass()
base.BeforeClass();
directory = NewDirectory();
- Analyzer analyzer = new AnalyzerAnonymousClass();
+ Analyzer analyzer = AnalyzerAnonymousClass.Default; // LUCENENET:
using static instance
Review Comment:
This will never be reused so we can revert it back to a constructor call for
clarity.
##########
src/Lucene.Net.Tests/Search/TestMultiThreadTermVectors.cs:
##########
@@ -60,7 +58,7 @@ public override void SetUp()
customType.StoreTermVectors = true;
for (int i = 0; i < numDocs; i++)
{
- Documents.Document doc = new Documents.Document();
+ Document doc = new Document();
Field fld = NewField("field", English.Int32ToEnglish(i),
customType);
Review Comment:
This is duplicate functionality:
https://github.com/apache/lucenenet/blob/b1476aee4fe21131899c1f43b2e06e25971b3ebe/src/Lucene.Net.TestFramework/Util/English.cs
https://github.com/apache/lucenenet/blob/b1476aee4fe21131899c1f43b2e06e25971b3ebe/src/Lucene.Net.Benchmark/Support/Util/EnglishNumberFormatExtensions.cs
The first was actually part of the test framework and the second is a
workaround because ICU4N didn't support rule-based number formatting until
recently. `Lucene.Net.Benchmark` only uses English language, though, so we
don't need to rely on ICU4N for this even though we now [have
RuleBasedNumberFormat
support](https://github.com/NightOwl888/ICU4N/blob/v60.1.0-alpha.402/src/ICU4N/Support/FormatNumberRuleBasedExtension.cs).
ICU4N 60.1.0-alpha.402 only has `RuleBasedNumberFormat` support for `net6.0`
and `net7.0`, but the main branch has support for `net451`+.
I suppose this one is also out of scope for this PR.
##########
src/Lucene.Net.Tests/Support/TestCase.cs:
##########
@@ -1,50 +0,0 @@
-namespace Lucene.Net
-{
- /*
- * 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.
- */
-
- /// <summary>
- /// Support for junit.framework.TestCase.getName().
- /// {{Lucene.Net-2.9.1}} Move to another location after LUCENENET-266
- /// </summary>
- public static class TestCase // LUCENENET specific: CA1052 Static holder
types should be Static or NotInheritable
- {
- public static string GetName()
- {
- return GetTestCaseName(false);
- }
-
- public static string GetFullName()
- {
- return GetTestCaseName(true);
- }
-
- static string GetTestCaseName(bool fullName)
- {
- System.Diagnostics.StackTrace stackTrace = new
System.Diagnostics.StackTrace();
- for (int i = 0; i < stackTrace.FrameCount; i++)
- {
- System.Reflection.MethodBase method =
stackTrace.GetFrame(i).GetMethod();
- object[] testAttrs =
method.GetCustomAttributes(typeof(NUnit.Framework.TestAttribute), false);
- if (testAttrs != null && testAttrs.Length > 0)
- if (fullName) return method.DeclaringType.FullName + "." +
method.Name;
Review Comment:
This looks like it is something like what we need to show the test name.
However, we need the type where the method is running, not the declaring type.
We should aim to fix this in the Test Framework, possibly in the
`LuceneTestCase` class.
##########
src/Lucene.Net.Tests/Search/TestFilteredQuery.cs:
##########
@@ -46,7 +46,7 @@ namespace Lucene.Net.Search
/// <summary>
/// FilteredQuery JUnit tests.
///
- /// <p>Created: Apr 21, 2004 1:21:46 PM
Review Comment:
We have been converting these to `<para/>`. Let's do that here, too.
##########
src/Lucene.Net.Tests/Search/TestQueryRescorer.cs:
##########
@@ -363,7 +357,7 @@ public virtual void TestRandom()
//System.out.println("numHits=" + numHits + " reverse=" + reverse);
TopDocs hits = s.Search(new TermQuery(new Term("field", "a")),
numHits);
- TopDocs hits2 = new QueryRescorerAnonymousClass3(this, new
FixedScoreQuery(idToNum, reverse))
+ TopDocs hits2 = new QueryRescorerAnonymousClass3(new
FixedScoreQuery(idToNum, reverse))
Review Comment:
Below we are missing the `CultureInfo.InvariantCulture` parameter for the
following lines:
https://github.com/paulirwin/lucene.net/blob/test-review-j-s/src/Lucene.Net.Tests/Search/TestQueryRescorer.cs#L376-L377
Negative integers will use localized minus signs, so to match Java we must
use this. Please fix and be sure to check for other string-to-number and
number-to-string conversions. I am going to create a new task for this because
there are many and several of them have common names like `ToString()` and we
just need to check the numeric/date data types. The IDE tools are inadequate
for pinpointing them, plus we have custom ones in J2N.
We also are unnecessarily calling the pass-through methods such as
`Convert.ToInt32(int)`. Ideally, we wouldn't have these calls in production
code. Having them in the tests isn't such a big deal.
And we will have new `ReadOnlySpan<char>` and `Span<char>` overloads of
`TryFormat/Parse/TryParse` in J2N that we will be able to take advantage of for
better performance.
##########
src/Lucene.Net.Tests/Search/TestFieldCacheRewriteMethod.cs:
##########
@@ -54,29 +54,12 @@ public virtual void TestEquals()
Assert.AreEqual(a1, a2);
Assert.IsFalse(a1.Equals(b));
- a1.MultiTermRewriteMethod = (new FieldCacheRewriteMethod());
- a2.MultiTermRewriteMethod = (new FieldCacheRewriteMethod());
- b.MultiTermRewriteMethod = (new FieldCacheRewriteMethod());
+ a1.MultiTermRewriteMethod = new FieldCacheRewriteMethod();
+ a2.MultiTermRewriteMethod = new FieldCacheRewriteMethod();
+ b.MultiTermRewriteMethod = new FieldCacheRewriteMethod();
Assert.AreEqual(a1, a2);
Assert.IsFalse(a1.Equals(b));
QueryUtils.Check(a1);
}
-
-
-
- #region TestSnapshotDeletionPolicy
- // LUCENENET NOTE: Tests in a base class are not pulled into the
correct
Review Comment:
I know we discussed this before, but I think in CI we still have an issue
with these. When we get a test failure in Azure DevOps, NUnit displays only the
base class, not the class where the failure occurred. This can make it
difficult to work out which of the subclasses caused the failure, especially if
there are multiple levels.
We do need an issue created to investigate whether we can improve the
reporting that NUnit does to show the subclass (perhaps by overriding the name
of the test to include the fully qualified name or class-qualified name), but
we shouldn't remove these until we have a replacement that allows us to
differentiate between these tests.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
