NightOwl888 commented on code in PR #897:
URL: https://github.com/apache/lucenenet/pull/897#discussion_r1457846884
##########
src/Lucene.Net.Tests/Util/Packed/TestEliasFanoDocIdSet.cs:
##########
@@ -22,11 +22,12 @@ namespace Lucene.Net.Util.Packed
using DocIdSetIterator = Lucene.Net.Search.DocIdSetIterator;
+ [TestFixture]
Review Comment:
FYI - a word of caution here. We have our own implementation of
`TestFixtureAttribute`, which is how we provide repeatable randomized testing.
If you declare it like this, it will correctly call our implementation (which
[exists inside
`LuceneTestCase`](https://github.com/apache/lucenenet/blob/85458d3c85822f2e01da434f3df012c65e1514d0/src/Lucene.Net.TestFramework/Support/Util/LuceneTestCase.TestFixtureAttribute.cs)).
Just be sure not to include the `NUnit.Framework` namespace. Technically,
leaving the attribute off with NUnit is fine because of attribute inheritance.
https://github.com/apache/lucenenet/blob/85458d3c85822f2e01da434f3df012c65e1514d0/src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs#L91-L145
##########
src/Lucene.Net.Tests/Util/BaseSortTestCase.cs:
##########
@@ -69,6 +67,7 @@ public int Compare(Entry a, Entry b)
public virtual void AssertSorted(Entry[] original, Entry[] sorted)
{
Assert.AreEqual(original.Length, sorted.Length);
+ // LUCENENET: this code differs significantly from the original
Java test which used Arrays.sort().
Entry[] stableSorted = original.OrderBy(e => e, new
StableEntryComparer()).ToArray();
Review Comment:
https://github.com/apache/lucenenet/blob/85458d3c85822f2e01da434f3df012c65e1514d0/src/Lucene.Net.Tests.Grouping/TestGrouping.cs#L545-L553
In general, we should be using `ArrayUtil.TimSort()` in these cases. However,
this is special case where we can probably use `Array.Sort()`, since it is
meant for testing our sorter. Need to verify that works, though -
`Array.Sort()` calls the comparer even if values are the same. This is okay for
most scenarios, but we had a few tests fail due to this difference.
LINQ is much slower because it allocates the entire array rather than simply
swapping the elements. And since this is in a base TestCase, it could have some
real impact on our testing performance. Since `StableEntryComparer` has no
arguments, we can also add a static property/field to it and call
`StableEntryComparer.Default` so the instance is reused in tests. Even though
this isn't production code, it costs us development time waiting for slow tests
to run.
##########
src/Lucene.Net.Tests/Util/TestInPlaceMergeSorter.cs:
##########
@@ -1,5 +1,3 @@
-using Assert = Lucene.Net.TestFramework.Assert;
Review Comment:
See my comment on TestPackedInts.
##########
src/Lucene.Net.Tests/Util/BaseSortTestCase.cs:
##########
@@ -44,12 +44,10 @@ public virtual int CompareTo(Entry other)
}
private readonly bool stable;
- private readonly Random random;
Review Comment:
Just FYI, this was done in an early attempt to use xUnit for testing.
https://github.com/apache/lucenenet/pull/177/files (although this isn't the
specific PR that changed this line, it shows broadly how the project was
changed). We made another attempt to use xUnit at a later point (which requires
decoupling from use of static variables), but neither attempt was successful.
xUnit support is no longer considered necessary and may not even be feasible
due to our later extensions of NUnit attributes to make the random tests
repeatable.
##########
src/Lucene.Net.Tests/Util/TestCollectionUtil.cs:
##########
@@ -4,7 +4,6 @@
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using JCG = J2N.Collections.Generic;
-using Assert = Lucene.Net.TestFramework.Assert;
Review Comment:
See my comment on TestPackedInts.
##########
src/Lucene.Net.Tests/Util/TestTimSorter.cs:
##########
@@ -17,6 +17,7 @@ namespace Lucene.Net.Util
* limitations under the License.
*/
+ [TestFixture]
Review Comment:
See my comment on `TestEliasFanoDocIdSet`.
##########
src/Lucene.Net.Tests/Util/StressRamUsageEstimator.cs:
##########
@@ -2,7 +2,6 @@
using NUnit.Framework;
using System;
using System.Globalization;
-using Assert = Lucene.Net.TestFramework.Assert;
Review Comment:
See my comment on TestPackedInts.
##########
src/Lucene.Net.Tests/Util/TestDocIdBitSet.cs:
##########
@@ -1,4 +1,3 @@
-using Assert = Lucene.Net.TestFramework.Assert;
Review Comment:
See my comment on TestPackedInts.
##########
src/Lucene.Net.Tests/Util/TestBroadWord.cs:
##########
@@ -26,6 +26,7 @@ public class TestBroadWord : LuceneTestCase
{
private void TstRank(long x)
{
+ // LUCENENET: using J2N PopCount extension instead of
Long.bitCount()
Review Comment:
Just FYI - PopCount() calls into hardware intrinsics on .NET 6+.
##########
src/Lucene.Net.Tests/Util/TestSetOnce.cs:
##########
@@ -1,10 +1,13 @@
using J2N.Threading;
using NUnit.Framework;
-using RandomizedTesting.Generators;
using System;
using System.Globalization;
using Assert = Lucene.Net.TestFramework.Assert;
+#if !NET6_0_OR_GREATER
Review Comment:
See my comment on `Test2BFST`.
##########
src/Lucene.Net.Tests/Util/TestBytesRefHash.cs:
##########
@@ -192,7 +194,7 @@ public virtual void TestCompact()
/// <summary>
/// Test method for
- /// <seealso
cref="Lucene.Net.Util.BytesRefHash#sort(java.util.Comparer)"/>.
+ /// <seealso
cref="Lucene.Net.Util.BytesRefHash.Sort(IComparer<BytesRef>)" />.
Review Comment:
FYI - to properly link in the XML docs, we should be using `{` and `}`. For
example: `<see cref="Lucene.Net.Util.BytesRefHash.Sort(IComparer{BytesRef})"
/>`. Also, we should be using `<see>` instead of `<seealso>` for inline links.
`<seealso>` is only for links to other members that appear at the bottom of the
docs. It doesn't matter here, though, because the tests don't have any
generated documentation.
##########
src/Lucene.Net.Tests/Util/TestWAH8DocIdSet.cs:
##########
@@ -212,6 +216,8 @@ public virtual void AssertEquals(int numBits, OpenBitSet
ds1, WAH8DocIdSet ds2)
[Test]
public virtual void TestIntersection()
{
+ // LUCENENET: using OpenBitSet instead of BitSet in this test, see
OpenBitSet documentation for rationale
Review Comment:
FYI - Do note that although the Javadoc says that `OpenBitSet` is faster
than `BitSet`, to my knowledge no benchmarks have been run in .NET. We can
leave this as is, but do note that J2N has an implementation of `BitSet` that
matches the one in the JDK. In many cases, the switch to `OpenBitSet` was done
prior to there being an Apache Harmony ported alternative.
##########
src/Lucene.Net.Tests/Util/TestIntroSorter.cs:
##########
@@ -1,5 +1,3 @@
-using Assert = Lucene.Net.TestFramework.Assert;
Review Comment:
See my comment on TestPackedInts.
##########
src/Lucene.Net.Tests/Util/Packed/TestPackedInts.cs:
##########
@@ -7,7 +7,6 @@
using System.Collections.Generic;
using System.Globalization;
using JCG = J2N.Collections.Generic;
-using Assert = Lucene.Net.TestFramework.Assert;
Review Comment:
Please put this back. NUnit asserts are extremely slow due to boxing. We do
this to override their implementation without making loads of code changes.
See: https://github.com/apache/lucenenet/issues/306
Note that even if there are no asserts in the file (making the import
unnecessary), we still would prefer to do so to ensure that any future calls to
`Assert` use the faster methods.
##########
src/Lucene.Net.Tests/Util/Fst/Test2BFST.cs:
##########
@@ -1,11 +1,14 @@
using Lucene.Net.Support;
using NUnit.Framework;
-using RandomizedTesting.Generators;
using System;
using Assert = Lucene.Net.TestFramework.Assert;
using Console = Lucene.Net.Util.SystemConsole;
using Int64 = J2N.Numerics.Int64;
+#if !NET6_0_OR_GREATER
Review Comment:
> NOTE: Instead of tying our source to specific releases of .NET, we are
using features. See:
https://github.com/apache/lucenenet/blob/85458d3c85822f2e01da434f3df012c65e1514d0/Directory.Build.targets.
Generally, we shouldn't be using the `NET...` compilation symbols going
forward. If there are any of these statements in the existing codebase, we
should probably replace them (provided they don't apply to .NET Framework,
which won't be changing). It is preferable to maintain the entire set of
features in one place than to have it strewn throughout the entire solution.
That said, in this specific case we should call the static method explicitly
using
[`RandomExtensions.NextInt64()`](https://github.com/NightOwl888/RandomizedTesting/blob/ded728157ec18d2f9377b919e3cc3355d5730367/src/RandomizedTesting.Generators/Support/RandomExtensions.cs#L119).
The .NET implementation is not guaranteed to be the same on different
platforms. However, the one in `RandomizedTesting` is the implementation used
in Apache Harmony, and will produce the same results across all platforms. This
is important so the tests that fail in CI are repeatable even if debugging on a
different platform than where the failure happened.
##########
src/Lucene.Net.Tests/Util/TestBytesRefArray.cs:
##########
@@ -114,11 +114,10 @@ public virtual void TestAppendIterator()
}
for (int i = 0; i < 2; i++)
{
- IBytesRefEnumerator iterator = list.GetEnumerator();
+ IBytesRefIterator iterator = list.GetIterator();
Review Comment:
`IBytesRefIterator` will be removed prior to the 4.8.0 release. We should be
using `IBytesRefEnumerator` here.
https://github.com/apache/lucenenet/blob/85458d3c85822f2e01da434f3df012c65e1514d0/src/Lucene.Net/Util/BytesRefIterator.cs#L79
##########
src/Lucene.Net.Tests/Util/TestMathUtil.cs:
##########
@@ -58,10 +57,11 @@ internal static long RandomLong()
}
// slow version used for testing
- private static bool TryGetGcd(long a, long b, out long result)
+ // LUCENENET: semantics changed to a Try pattern as BigInteger could
overflow long
+ private static bool TryGetGcd(long l1, long l2, out long result)
{
result = 0;
- var c = System.Numerics.BigInteger.GreatestCommonDivisor(a, b);
+ var c = System.Numerics.BigInteger.GreatestCommonDivisor(l1, l2);
Review Comment:
Hmm...these variable names look way too much like the numbers 11 and 12.
Perhaps a and b would be better, as they are easier to identify as variables. I
see they made them this way in Java, but it could be confusing when debugging.
##########
src/Lucene.Net.Tests/Util/TestNamedSPILoader.cs:
##########
@@ -56,4 +56,4 @@ public virtual void TestAvailableServices()
Assert.IsTrue(codecs.Contains("Lucene46"));
}
}
-}
\ No newline at end of file
+}
Review Comment:
Hmm...I noticed there are changes to many files that only affect the end of
the file. Perhaps we should look into finding a way to add configuration
(possibly to Git) to ensure all contributors provide files with the same ending?
##########
src/Lucene.Net.Tests/Util/TestQueryBuilder.cs:
##########
@@ -57,8 +57,7 @@ public virtual void TestBoolean()
expected.Add(new TermQuery(new Term("field", "foo")),
Occur.SHOULD);
expected.Add(new TermQuery(new Term("field", "bar")),
Occur.SHOULD);
QueryBuilder builder = new QueryBuilder(new MockAnalyzer(Random));
- Assert.IsTrue(expected.Equals(builder.CreateBooleanQuery("field",
"foo bar")));
- //Assert.AreEqual(expected, builder.CreateBooleanQuery("field",
"foo bar"));
+ Assert.AreEqual(expected, builder.CreateBooleanQuery("field", "foo
bar"));
Review Comment:
Good catch
##########
src/Lucene.Net.Tests/Util/TestPForDeltaDocIdSet.cs:
##########
@@ -20,6 +20,7 @@ namespace Lucene.Net.Util
* limitations under the License.
*/
+ [TestFixture]
Review Comment:
See my comment on `TestEliasFanoDocIdSet`.
##########
src/Lucene.Net.Tests/Util/Test2BPagedBytes.cs:
##########
@@ -1,10 +1,13 @@
using Lucene.Net.Diagnostics;
using Lucene.Net.Store;
using NUnit.Framework;
-using RandomizedTesting.Generators;
using System;
using Assert = Lucene.Net.TestFramework.Assert;
+#if !NET6_0_OR_GREATER
Review Comment:
See my comment on `Test2BFST`.
##########
src/Lucene.Net.Tests/Util/TestMergedIterator.cs:
##########
@@ -3,7 +3,10 @@
using System.Collections.Generic;
using JCG = J2N.Collections.Generic;
using Assert = Lucene.Net.TestFramework.Assert;
-using RandomizedTesting.Generators;
+
+#if !NET6_0_OR_GREATER
Review Comment:
See my comment on `Test2BFST`.
--
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
