NightOwl888 commented on code in PR #1101:
URL: https://github.com/apache/lucenenet/pull/1101#discussion_r1914822512
##########
src/Lucene.Net/Support/ExceptionHandling/StackTraceHelper.cs:
##########
@@ -1,7 +1,9 @@
using System;
using System.Diagnostics;
+using System.IO;
+using System.Runtime.CompilerServices;
-namespace Lucene.Net.Util
+namespace Lucene.Net.Support
Review Comment:
Let's just keep this in `Lucene.Net.Util`. We may end up needing this to be
public at some point.
##########
src/Lucene.Net.Tests.AllProjects/Support/ExceptionHandling/TestStackTraceHelper.cs:
##########
@@ -0,0 +1,54 @@
+using Lucene.Net.Attributes;
+using Lucene.Net.Util;
+using NUnit.Framework;
+using System.IO;
+
+namespace Lucene.Net.Support.ExceptionHandling
+{
+ /*
+ * 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.
+ */
+
+ [TestFixture, LuceneNetSpecific]
+ public class TestStackTraceHelper : LuceneTestCase
+ {
+ [Test]
+ public void TestDoesStackTraceContainMethod()
Review Comment:
Let's add a couple of negative tests here (checking for `false`).
##########
src/Lucene.Net.Tests.AllProjects/Support/ExceptionHandling/TestExceptionExtensions.cs:
##########
@@ -375,6 +377,73 @@ public void
TestIsIllegalArgumentException_TestEnvironment(Type exceptionType, b
}
}
+ private class MyException : Exception
+ {
+ public MyException(string message) : base(message)
+ {
+ }
+ }
+
+ private class MyException2 : Exception
+ {
+ public MyException2(string message) : base(message)
+ {
+ }
+ }
+
+ [Test]
+ public void TestPrintStackTrace()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
Review Comment:
`typeof(MyException).FullName` can never be `null` here. Please remove the
coalesce and exception.
##########
src/Lucene.Net.TestFramework/Index/ThreadedIndexingAndSearchingTestCase.cs:
##########
@@ -536,8 +535,8 @@ public override void Run()
catch (Exception t) when (t.IsThrowable())
{
Console.WriteLine(Thread.CurrentThread.Name + ": hit
exc");
- outerInstance.m_failed.Value = (true);
- Console.WriteLine(t.ToString());
+ outerInstance.m_failed.Value = true;
+ t.PrintStackTrace(Console.Out);
Review Comment:
Upstream `PrintStackTrace()` occurred on the line before
`failed.set(true);`, not the line after. Let's update this to match.
##########
src/Lucene.Net.Tests.Analysis.Common/Analysis/Util/TestBufferedCharFilter.cs:
##########
@@ -730,6 +732,7 @@ public void Test_Ready()
/**
* @tests java.io.BufferedReader#reset()
*/
+ [Test, LuceneNetSpecific]
Review Comment:
Good catch!
##########
src/Lucene.Net/Support/ExceptionHandling/StackTraceHelper.cs:
##########
@@ -22,14 +24,14 @@ namespace Lucene.Net.Util
/// <summary>
/// LUCENENET specific class to normalize stack trace behavior between
different .NET Framework and .NET Standard 1.x,
- /// which did not support the StackTrace class.
+ /// which did not support the StackTrace class, and provide some
additional functionality.
/// </summary>
- public static class StackTraceHelper
+ internal static class StackTraceHelper
Review Comment:
Let's be defensive about guarding the method arguments against `null` in
this class. Since this is currently internal, we could just use
`Debug.Assert()` so there is no runtime cost. This will also allow us to use
`#nullable enable` on this file.
##########
src/Lucene.Net.Tests.Analysis.Common/Analysis/Util/TestBufferedCharFilter.cs:
##########
@@ -713,6 +713,8 @@ public void Test_ReadLine()
/**
* @tests java.io.BufferedReader#ready()
*/
+ [Test, LuceneNetSpecific]
+ [Ignore("Test was previously ignored by missing Test attribute, does
not work")] // LUCENENET TODO: fix test
Review Comment:
We should use the `[AwaitsFix]` attribute here, not `[Ignore]`, and we
should open an issue about it to put in the attribute.
##########
src/Lucene.Net.Tests.AllProjects/Support/ExceptionHandling/TestExceptionExtensions.cs:
##########
@@ -375,6 +377,73 @@ public void
TestIsIllegalArgumentException_TestEnvironment(Type exceptionType, b
}
}
+ private class MyException : Exception
+ {
+ public MyException(string message) : base(message)
+ {
+ }
+ }
+
+ private class MyException2 : Exception
+ {
+ public MyException2(string message) : base(message)
+ {
+ }
+ }
+
+ [Test]
+ public void TestPrintStackTrace()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
+ Assert.IsTrue(str.Contains("Test exception"));
+ Assert.IsTrue(str.Contains(nameof(TestPrintStackTrace)));
+ }
+
+ [Test]
+ public void TestPrintStackTrace_WithSuppressed()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ try
+ {
+ throw new MyException2("Suppressed exception");
+ }
+ catch (Exception suppressed)
+ {
+ e.AddSuppressed(suppressed);
+ }
+
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
Review Comment:
`Contains()` uses the culture of the current thread to make the comparison.
Please be explicit and use `StringComparison.Ordinal` in these five calls.
##########
src/Lucene.Net.Tests/Store/TestLockFactory.cs:
##########
@@ -100,7 +100,7 @@ public virtual void TestRAMDirectoryNoLocking()
}
catch (Exception e) when (e.IsException())
{
- e.printStackTrace();
+ e.PrintStackTrace();
Review Comment:
Upstream printed to `System.out`, let's do the same.
##########
src/Lucene.Net.Tests.AllProjects/Support/ExceptionHandling/TestExceptionExtensions.cs:
##########
@@ -375,6 +377,73 @@ public void
TestIsIllegalArgumentException_TestEnvironment(Type exceptionType, b
}
}
+ private class MyException : Exception
+ {
+ public MyException(string message) : base(message)
+ {
+ }
+ }
+
+ private class MyException2 : Exception
+ {
+ public MyException2(string message) : base(message)
+ {
+ }
+ }
+
+ [Test]
+ public void TestPrintStackTrace()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
+ Assert.IsTrue(str.Contains("Test exception"));
+ Assert.IsTrue(str.Contains(nameof(TestPrintStackTrace)));
+ }
+
+ [Test]
+ public void TestPrintStackTrace_WithSuppressed()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ try
+ {
+ throw new MyException2("Suppressed exception");
+ }
+ catch (Exception suppressed)
+ {
+ e.AddSuppressed(suppressed);
+ }
+
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
+ Assert.IsTrue(str.Contains("Test exception"));
+ Assert.IsTrue(str.Contains(nameof(TestPrintStackTrace)));
+ Assert.IsTrue(str.Contains(typeof(MyException2).FullName ?? throw
new InvalidOperationException("Type name is null")));
Review Comment:
`typeof(MyException2).FullName` can never be `null` here. Please remove the
coalesce and exception.
##########
src/Lucene.Net.Tests/Index/TestIndexWriterOutOfFileDescriptors.cs:
##########
@@ -146,8 +146,7 @@ public virtual void Test()
if (Verbose)
{
Console.WriteLine("TEST: iter=" + iter + ":
exception");
- Console.WriteLine(ioe.ToString());
- Console.Write(ioe.StackTrace);
+ ioe.PrintStackTrace(Console.Out);
Review Comment:
The upstream code doesn't include the `System.out` parameter, thus writing
to `System.err`. Let's change to match.
##########
src/Lucene.Net.Tests.AllProjects/Support/ExceptionHandling/TestExceptionExtensions.cs:
##########
@@ -375,6 +377,73 @@ public void
TestIsIllegalArgumentException_TestEnvironment(Type exceptionType, b
}
}
+ private class MyException : Exception
+ {
+ public MyException(string message) : base(message)
+ {
+ }
+ }
+
+ private class MyException2 : Exception
+ {
+ public MyException2(string message) : base(message)
+ {
+ }
+ }
+
+ [Test]
+ public void TestPrintStackTrace()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
Review Comment:
`Contains()` uses the culture of the current thread to make the comparison.
Please be explicit and use `StringComparison.Ordinal` in these three calls.
##########
src/Lucene.Net.Tests.Analysis.Common/Analysis/Core/TestRandomChains.cs:
##########
@@ -933,7 +933,7 @@ private T CreateComponent<T>(ConstructorInfo ctor, object[]
args, StringBuilder
if (Verbose)
{
Console.WriteLine("Ignoring IAE/UOE from ctor:");
- //cause.printStackTrace(System.err);
+ ite.InnerException.PrintStackTrace(Console.Error);
Review Comment:
The check on line 927 looks suspicious. We should probably be using the
`IsIllegalArgumentException()` and `IsUnsupportedOperationException()`
extension methods to match upstream. We can also further align the code by
setting `ite.InnerException` to a local variable named `cause`.
I will work this out and add any changes to #1058.
##########
src/Lucene.Net/Support/ExceptionHandling/StackTraceHelper.cs:
##########
@@ -22,14 +24,14 @@ namespace Lucene.Net.Util
/// <summary>
/// LUCENENET specific class to normalize stack trace behavior between
different .NET Framework and .NET Standard 1.x,
Review Comment:
Let's update this documentation, since we no longer support .NET Standard
1.x and this class no longer normalizes anything.
`DoesStackTraceContainMethod()` overloads make porting a bit easier at the
cost of some performance (see
#https://github.com/apache/lucenenet/issues/1098#issuecomment-2585607178). What
are your thoughts on putting this code back inline the way it was upstream and
eliminating these two methods?
##########
src/Lucene.Net.Tests.AllProjects/Support/ExceptionHandling/TestExceptionExtensions.cs:
##########
@@ -375,6 +377,73 @@ public void
TestIsIllegalArgumentException_TestEnvironment(Type exceptionType, b
}
}
+ private class MyException : Exception
+ {
+ public MyException(string message) : base(message)
+ {
+ }
+ }
+
+ private class MyException2 : Exception
+ {
+ public MyException2(string message) : base(message)
+ {
+ }
+ }
+
+ [Test]
+ public void TestPrintStackTrace()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
+ Assert.IsTrue(str.Contains("Test exception"));
+ Assert.IsTrue(str.Contains(nameof(TestPrintStackTrace)));
+ }
+
+ [Test]
+ public void TestPrintStackTrace_WithSuppressed()
+ {
+ using var sw = new StringWriter();
+
+ try
+ {
+ throw new MyException("Test exception");
+ }
+ catch (Exception e)
+ {
+ try
+ {
+ throw new MyException2("Suppressed exception");
+ }
+ catch (Exception suppressed)
+ {
+ e.AddSuppressed(suppressed);
+ }
+
+ e.PrintStackTrace(sw);
+ }
+
+ var str = sw.ToString();
+
+ Assert.IsTrue(str.Contains(typeof(MyException).FullName ?? throw
new InvalidOperationException("Type name is null")));
Review Comment:
`typeof(MyException).FullName` can never be `null` here. Please remove the
coalesce and exception.
--
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
