NightOwl888 commented on code in PR #1084:
URL: https://github.com/apache/lucenenet/pull/1084#discussion_r1901575061
##########
src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs:
##########
@@ -3126,19 +3126,20 @@ private static void CleanupTemporaryFiles()
}
catch (Exception e) when (e.IsIOException())
{
- // Type suiteClass =
RandomizedContext.Current.GetTargetType;
- // if
(suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
- // {
- Console.Error.WriteLine("WARNING: Leftover undeleted
temporary files " + e.Message);
- return;
- // }
+ Type suiteClass = this.GetType();
Review Comment:
Please change this method back to static and use
`RandomizedContext.CurrentContext.CurrentTest.TypeInfo.Type` to get the class
type.
##########
src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs:
##########
@@ -3126,19 +3126,20 @@ private static void CleanupTemporaryFiles()
}
catch (Exception e) when (e.IsIOException())
{
- // Type suiteClass =
RandomizedContext.Current.GetTargetType;
- // if
(suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
- // {
- Console.Error.WriteLine("WARNING: Leftover undeleted
temporary files " + e.Message);
- return;
- // }
+ Type suiteClass = this.GetType();
+ if
(suiteClass.GetCustomAttribute<SuppressTempFileChecksAttribute>(inherit: true)
is { } suppressAttr)
+ {
+ Console.Error.WriteLine($"WARNING: Leftover undeleted
temporary files (bugUrl: {suppressAttr.BugUrl}): {e.Message}");
Review Comment:
We may also be able to use
`RandomizedContext.CurrentContext.CurrentTest.TypeInfo.Type` to back the
`LuceneTestCase.TestType` property. This property corresponds to the
`getTestClass()` method in Lucene. It is implemented, but this looks like a
better approach than using the instance of the test class to call `.GetType()`
at runtime, though it may need some verification to ensure the lifetime and
type are the same before making the switch.
##########
src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs:
##########
@@ -3126,19 +3126,20 @@ private static void CleanupTemporaryFiles()
}
catch (Exception e) when (e.IsIOException())
{
- // Type suiteClass =
RandomizedContext.Current.GetTargetType;
- // if
(suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
- // {
- Console.Error.WriteLine("WARNING: Leftover undeleted
temporary files " + e.Message);
- return;
- // }
+ Type suiteClass = this.GetType();
+ if
(suiteClass.GetCustomAttribute<SuppressTempFileChecksAttribute>(inherit: true)
is { } suppressAttr)
Review Comment:
I am not sure if it is appropriate, but there is a
`RandomizedContext.CurrentContext.CurrentTest.GetCustomAttributes<TAttr>(bool)`
method we might be able to use here. If so, we don't need to get the type above.
##########
src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs:
##########
@@ -3126,19 +3126,20 @@ private static void CleanupTemporaryFiles()
}
catch (Exception e) when (e.IsIOException())
{
- // Type suiteClass =
RandomizedContext.Current.GetTargetType;
- // if
(suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
- // {
- Console.Error.WriteLine("WARNING: Leftover undeleted
temporary files " + e.Message);
- return;
- // }
+ Type suiteClass = this.GetType();
+ if
(suiteClass.GetCustomAttribute<SuppressTempFileChecksAttribute>(inherit: true)
is { } suppressAttr)
+ {
+ Console.Error.WriteLine($"WARNING: Leftover undeleted
temporary files (bugUrl: {suppressAttr.BugUrl}): {e.Message}");
Review Comment:
Note that this code is not intended to always run. However, the
`LuceneTestCase.SuiteFailureMarker` property has not been implemented. So, this
is currently hard coded to always run. That property was set by some
setup/teardown rule in Lucene and provided more info than a simple pass/fail:
https://github.com/apache/lucene/blob/releases/lucene-solr/4.8.0/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleMarkFailure.java.
Note that we have an `AbstractBeforeAfterRule` that is sort of set up for
this, but it is not chained together with a decorator pattern like it was in
Lucene, instead it uses inheritance. It is also may not be wired in the same
way as in Lucene, being that it uses the "before assembly" and "after assembly"
path rather than the "before class" and "after class" path and "before method"
and "after method" path. It would be good to run the code in Lucene to
determine which level to put this because it is not very clear what "after
suite" in Lucene correspon
ds to.
We can use `NUnit.Framework.Internal.TestResult` to provide the data of
`LuceneTestCase.SuiteFailureMarker`, which would provide both exception info
and pass/fail info. Although, we should create a wrapper class so we own the
API and can add functionality that NUnit doesn't have down the road. The
instance of that class is available from the static property
`TestExecutionContext.CurrentContext.CurrentResult` which is also being read in
the `TearDown()` method (it also demonstrates how to check for a failure). The
name `SuiteFailureMarker` doesn't make much sense in .NET, though. Perhaps we
should just call it `TestResult` (similar to the return type) and add a comment
to indicate that it corresponds to `SuiteFailureMarker` in Java.
Note that if we need to move this "after assembly", we will need to somehow
store this `NUnit.Framework.Internal.TestResult` at a higher level in
`RandomziedContext`, generating a suite result status (in a similar way NUnit
generates the suite result status for a class full of tests and nested classes).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
