Added DisposeAfterSuite/DisposeAfterTest feature to test framework

[NightOwl888]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Added DisposeAfterSuite/DisposeAfterTest feature to test framework

Fixes #1094

Description

This adds the LuceneTestCase.DisposeAfterTest() and LuceneTestCase.DisposeAfterSuite() methods to the test framework that exist in Lucene 4.8.0.

It also removes the LuceneTestCase.SuiteFailureMarker property that wasn't implemented. NUnit already tracks the result state of the tests in NUnit.Framework.Internal.TestExectionContext.CurrentContext.TestResult.ResultState and NUnit.Framework.TestContext.CurrentContext.Result.Outcome in a context-sensitive and thread safe manner, so there is no need to reinvent the wheel with this. For the same reason, we don't need the TestFailureMarker property or TestRuleMarkFailure class, which are all part of this feature.

The new feature also revealed that there were some directories that were not being disposed in Lucene.Net.Tests.Grouping which were also addressed.

For some reason TestFsts.TestRealTerms() does not reliably cleanup its line file docs temp file on Windows, so this adds the [SuppressTempFileChecks] attribute to suppress the issue when it happens (for now). At some point we should have a look at this, but it is not a blocker for the release.

[paulirwin]

Thoughts on using Lazy<ConcurrentBag<IDisposable>> here with one of the thread-safe constructor options? That would remove the need for the locking. I would imagine in this case we don't need to worry about the "uninterruptible"-ness, but let me know if we do. I also suggest ConcurrentBag in the case that the order doesn't matter, but if it does, we might need to think about using ConcurrentStack instead, because you might want it to go in opposite order for disposal. Or ConcurrentQueue would allow for insertion-order disposal.

[NightOwl888]

I was partially following the upstream code. https://github.com/randomizedtesting/randomizedtesting/blob/master/randomized-runner/src/main/java/com/carrotsearch/randomizedtesting/RandomizedContext.java#L150-L160.

The DisposeResources() method will always be called exactly once on each test and once on each suite (class), so I suppose the lock is not strictly necessary, for now. We took a very different approach, though. Our RandomizedContext instance is stored inside of the NUnit test or suite instance. So, it is not very likely multiple threads will access the same instance of RandomizedContext and if so, they will likely be threads applying to a single test. It will return to the main thread to dispose.

Disposal order is FIFO, since they also used a List<T> in Java.

https://github.com/randomizedtesting/randomizedtesting/blob/master/randomized-runner/src/main/java/com/carrotsearch/randomizedtesting/RandomizedContext.java#L59C11-L60

However, they are removing the list from the collection in closeResources() inside of the lock before removing anything from the list. So, to be on the safe side, we should probably nullify that field after grabbing the instance and then dispose outside of the lock.

[NightOwl888]

I ended up using your suggestion of Lazy<ConcurrentQueue<DisposableResourceInfo>> after porting the ClosableResourceInfo class from randomizedtesting.

DisposableResourceInfo keeps track of the details of the caller of LuceneTestCase.DisposeAfterSuite() (including the stack trace) so it can be used to log the caller details when an exception is thrown from Dispose(). The cheapest way I could find to do this was to instantiate a StackTrace instance at the point where we want to record the details and wait until after an exception is thrown to call ToString() on it to allocate the string. Let me know if you can think of a more efficient way.

Also note that I didn't port the part of randomizedtesting that gives each thread a unique name (yet). So at present, ThreadName is always null.

[paulirwin]

I might have been somewhat misinterpreted about Lazy<T> here, so added clarifying comments. I think this code can be simplified further and be made more thread-safe at the same time, as the coalesce is not thread-safe as-is.

[paulirwin]

I think this is now "double-lazy" - and the coalesce below removes the thread-safety guarantees of Lazy<T>. I think this should be definitely initialized to a new Lazy<T>(() => ...), either with the default thread-safety mode or an explicit one, rather than setting it to null. That way when .Value is accessed, it will run the initializer callback if it hasn't been created in a thread-safe way.

This will also simplify the code below, because the property ToDisposeAtEnd can then just be => toDisposeAtEnd.Value;

[paulirwin]

This can be simplified and still be thread-safe if the Lazy is non-nullable above. This line could be removed, and replace toDispose with toDisposeAtEnd below this.

[NightOwl888]

The primary concern is about object lifetime. It is more important to ensure that the list of disposable objects is allowed to go out of scope than it is to ensure it is lazily created. The lifetime of RandomizedContext is from the test discovery phase to the end of the test run and we need to ensure these disposable instances go out of scope after the test or class is finished executing. Calling Clear() after each test and/or class would be expensive, so it is better to let the list go out of scope without clearing it by setting the field to null.

I think that using a lock with a nullable List<T> field is simpler. These aren't the only features that use the lock in the upstream code. It would allow the other features of RandomizedContext to be added without having to go back and rework it later to use a lock again.

In addition, there won't be any lock contention caused by the test framework because NUnit is managing these events and they do not happen simultaneously. Lock contention could only happen if the user explicitly calls the DisposeAfterTest() and/or DisposeAfterSuite() methods in parallel. But for users, the TearDown() and OneTimeTearDown() methods are generally better options than these for calling Dispose() on disposable objects.

Thoughts?

[NightOwl888]

Just to clarify, there is 1 RandomizedContext instance per class and 1 RandomizedContext instance per test. So, when interacting with RandomizedContext.CurrentContext, the instance that is retrieved is different depending on which test or class is currently being executed. What I am trying to achieve is to let the disposable instances that are associated with that specific instance of RandomizedContext go out of scope as soon as the test is done with them. Neither the disposable objects or the list instance is used again after that point until the end of the test run.

[paulirwin]

It didn't seem to me from reading the code like it needed to be set back to null, you would just dispose of all objects in the queue. If you then needed to dispose of more objects, they could be added to the same queue if the instance is shared and then disposed when cleared out again. But if you think it needs to be set to null, I defer to your judgement on that.