Task: Review Lucene.Net.TestFramework for Missing Features, Incomplete Features and Inconsistencies

[NightOwl888]

Is there an existing issue for this?

• I have searched the existing issues

Task description

While it is presumed that we have enough of the test framework ported for the 4.8.0 release, we don't currently have an inventory of the features that are not ported or only partially working. For example, there are properties like LuceneTestCase.SuiteFailureMarker that are public but currently do not function that should either be made internal or functional before the release. There are also "after suite" cleanup features that are not ported (#898) that seem like they may be useful or even critical for fixing issues like #271.

For informational purposes, here are the open issues for the test framework at the time of this writing:

Known Test Framework Incomplete Features

• Port RandomizedTesting Test Runner #264
• Make assertions into a 1st class feature in the test framework #306
• Task: Finish [SuppressTempFileChecks] attribute functionality #898

Known Test Framework Bugs/Inconsistencies

• [Slow] attribute runs the code in BeforeClass() and AfterClass() even when the tests:slow property is set to false #739
• Fix test name reporting when test is in a base class #923
• Change failed test message to use lucene.testsettings.json instead of .runsettings #999
• Upgrade NUnit to 4.x #1001
• Task: Rename BeforeClass() to OneTimeSetUp() and AfterClass() to OneTimeTearDown() to match NUnit Conventions #1016

---

This task is to analyze the test framework to determine which features exist that we don't have. It will take some analysis to work out where a feature begins and ends, but once that is determined, we should use it to generate a list of missing features. From that list, we should then open an issue for each feature (assuming there isn't already an issue for it) so we can prioritize it and assign it a milestone.
EDIT: Let's make the deliverable of this task the initial list. From there, we will decide whether each feature deserves its own issue or should be scrapped. That list can simply be posted as a comment here.

We should also be on the lookout for features that are incomplete or inconsistent and open issues for those, as well.

If there are any notes that come from this analysis that could be helpful for us to decide on how difficult it is to port a feature or how valuable it is, please add them to the corresponding issue.

NOTE: The main places to look are in LuceneTestCase and the missing classes in the Lucene.Net.Util namespace (i.e. "TestRule" classes).

NOTE: Some of the randomized-testing runner classes were ported to Lucene.Net.TestFramework (at least at one point), so if there are types that are from that library, they can be excluded from the features list because they are covered by #264. We have a substitute for the randomized-testing runner in the Support/Util folder and don't intend to port the entire library for the 4.8.0 release. It is a considerable amount of extra work to make those features general enough to work outside of the context of the test framework. Although, we may be missing tests from randomized-testing that would help to ensure our substitute is working as expected that could be considered "features".

[NightOwl888]

Below is the list of features we are missing in the test framework, which is up for discussion about whether or not we need these features. Some I thought were clear, but feel free to make a case if you disagree. There are a few additional features listed that I think would be useful, but are not necessarily required. There are some others that are probably the source of some of the bugs we are seeing.

Note that I am listing the features mostly with their Java names. This does not mean we have to implement it exactly how it is in Java because while NUnit provides a rough approximation of the features that JUnit does, it doesn't implement them the same way and does not necessarily provide a 1-to-1 correlation in terms of how things can be extended. So, even though I am listing the TestRule classes, we could ultimately end up with a completely different implementation with similar functionality.

Also note that whether or not a feature is required may depend on whether or not we implement randomizedtesing (#264). So, that decision could impact this feature list a lot.

The goal is to move everything to either Needed or Not Needed and then we can open up a GitHub issue for each feature that is Needed.

Features: Needed

Feature Name	Summary	Notes
Test Rule Chaining (#1088)	This controls how suite-level and test-level rules are nested. It is important that all rules declared in LuceneTestCase are executed in proper order if they depend on each other.	The JUnit classes should ideally be in the RandomizedTesting namepace, but whether this functionality exists internally in Lucene.Net.TestFramework or is made public and put into RandomizedTesting will need to be decided. Taking control over NUnit to call our chains before everything else and after everything else is a considerable challenge and it may not be possible to do it this way without our own test adapter.
TestRuleMarkFailure SuiteFailureMarker	Suite failure marker (any error in the test or suite scope). A rule for marking failed tests and suites.	I think we can use NUnit.Framework.Internal.TestResult (which is available from TestExecutionContext.CurrentContext .CurrentResult) to get this data from NUnit.
TemporaryFilesCleanupRule tempDirBase	Cleans up files after running a test. This always runs whether there is an exception or not.	This functionality has been implemented as the LuceneTestCase.CleanupTemporaryFiles() method, but should be changed if we do the Test Rule Chaining. NOTE: The tempDirBase is always set to null. This seems like a Lucene bug.
StaticFieldsInvariantRule	A TestRule that ensures static, reference fields of the suite class (and optionally its superclasses) are cleaned up after a suite is completed. This is helpful in finding out static memory leaks (a class references something huge but is no longer used).	StaticFieldsInvariantRule is from randomizedtesting. We don't have any functionality that cleans up static fields that inherit LuceneTestCase. This sounds important to keep memory from leaking.
NoClassHooksShadowingRule	Don't allow BeforeClass and AfterClass hook shadowing as it is most likely a user error. JUnit rules for shadowed hook methods are weird.	NoClassHooksShadowingRule is in randomizedtesting. This sounds like something we can make a code analyzer for so this will fail at design time. It is only required because the OneTimeSetUp and OneTimeTearDown methods in Lucene are static and can be shadowed. We currently use inheriance, but are considering reverting to the original design. See #1087. However, we can probably use a Roslyn code analyzer for this so this can be enforced at compile time. Perhaps this could be based on a custom attribute to mark methods that should be scanned for this rule.
TestRuleSetupTeardownChained parentChainCallRule parentChainCallRule.setupCalled parentChainCallRule.tearDownCalled	Make sure LuceneTestCase#setUp() and LuceneTestCase#tearDown() were invoked even if they have been overriden. We assume nobody will call these out of non-overriden methods (they have to be public by contract, unfortunately). The top-level methods just set a flag that is checked upon successful execution of each test case.	This can be enforced using a code analyzer. It is important to do if we don't make these methods static (they are virtual in Lucene). See #1087.
TestRuleThreadAndTestName threadAndTestNameRule IsTestThread	Saves the executing thread and method name of the test case.	We can probably leave the test name up to NUnit using NUnit.Framework.TestContext.CurrentContext .Test.MethodName. But detecting whether the calling thread is the same as the test thread is not currently implemented. Neither is asserting that it is not null.
TestRuleSetupAndRestoreInstanceEnv	Prepares and restores LuceneTestCase at instance level (fine grained junk that doesn't fit anywhere else).	This only sets the BooleanQuery.MaxClauseCount before every test and restores the original value when the test is finished. Some tests do this explicitly, but I suspect not having it done globally is causing some test conditions to differ from Lucene (bugs).
TestRuleFieldCacheSanity	This rule will fail the test if it has insane field caches. (docs)	This is the only thing that calls LuceneTestCase.AssertSaneFieldCaches(), but it is currently not implemented. We should be calling this from the TearDown() method (in the right order) or from a rule chain that is setup with the correct calling order if we implement rule chaining.
closeAfterTest()	Registers a Closeable resource that should be closed after the test completes.	It probably makes sense in .NET to make methods for both Dispose() and Close(). Dispose() is definitely useful to have. Note this was entirely implemented by randomizedtesting (RandomizedContext). This is not called anywhere by Lucene, it is meant for users. It is probably useful and worth having.
closeAfterSuite() CloseableDirectory	Registers a Closeable resource that should be closed after the suite (usually class) completes.	It probably makes sense in .NET to make methods for both Dispose() and Close(). Dispose() is definitely useful to have. Note this was entirely implemented by randomizedtesting (RandomizedContext). This is called by LuceneTestCase.wrapDirectory() in Lucene. We are skipping this call in .NET, which is probably leaking resources (maybe even file handles). This may be a contributor to the cascade failures we were seeing during testing. We definitely need this.
Compound Random Seed	Test seeds are generated based off of a fixture seed. A compound seed combines both seeds into a single string for repeatability.	This is the approach that is used in randomizedtesting. Implementing the random seed this way would make seeds that can be used across updates to tests. Currently, if tests are added or removed, it could affect the repeatability of the seeds.
SuppressTempFileChecksAttribute (#898)		Already in progress.
[Repeat] attribute	A custom [Repeat] attribute that is guaranteed to repeat with the same seed.	We currently use NUnit's repeat attribute for debugging and in some cases, for CI. This use case appears to work, but has not been thoroughly tested to ensure repeatability. At the very least, we need tests, but we may need a custom attribute to pull this off. NUnit has an ITimeout interface that is undocumented and may or may not serve this purpose (subclassing TimeoutAttribute has already proven not to function).

Features: For Review

Feature Name	Summary	Notes
TestRuleIgnoreAfterMaxFailures SYSPROP_MAXFAILURES SYSPROP_FAILFAST ignoreAfterMaxFailuresDelegate ignoreAfterMaxFailures replaceMaxFailureRule() TestRuleDelegate	Ignore tests after hitting a designated number of initial failures. This is truly a "static" global singleton since it needs to span the lifetime of all test classes running inside this JVM (it cannot be part of a class rule). Optionally skips all of the remaining tests after the first failure.	We may be able to utilize the LuceneTestCase.SetUpFixture or LuceneTestFrameworkInitializer to manage this with NUnit.
STATIC_LEAK_THRESHOLD STATIC_LEAK_IGNORED_TYPES	Max 10mb of static data stored in a test suite class after the suite is complete. Prevents static data structures leaking and causing OOMs in subsequent tests. By-name list of ignored types like loggers etc
RunListenerPrintReproduceInfo JENKINS_LARGE_LINE_DOCS_FILE	A suite listener printing a "reproduce string". This ensures test result events are always captured properly even if exceptions happen at initialization or suite/ hooks level.	RunListenerPrintReproduceInfo is a JUnit run listener. This prints similar information that we do during a test failure. However, we should review this because there are some things we are not printing out that could be helpful.
TestRuleIgnoreTestSuites	This rule will cause the suite to be assumption-ignored if the test class implements a given marker interface and a special property is not set. This is a workaround for problems with certain JUnit containers (IntelliJ) which automatically discover test suites and attempt to run nested classes that we use for testing the test framework itself.
NoInstanceHooksOverridesRule	Don't allow Before and After hook overrides as it is most likely a user error and will result in superclass methods not being called (requires manual chaining).	NoInstanceHooksOverridesRule is in randomizedtesting If we implement nested attributes with the same name as NUnit, we can largely avoid this. See #1087. If we need this, I think this would be best enforced using a Roslyn code analyzer (build failure) rather than runtime checks.
FailureMarker WithNestedTests	A JUnit RunListener that detects suite/test failures. Lucene needs it because failures due to thread leaks happen outside of any rule contexts. An abstract test class that prepares nested test classes to run. A nested test class will assume it's executed under control of this class and be ignored otherwise. The purpose of this is so that nested test suites don't run from IDEs like Eclipse (where they are automatically detected).
BadAppleAttribute	Attribute for tests that fail frequently and should be moved to a "vault" plan in Jenkins.	Being that we have AwaitsFixAttribute, this seems like redundant functionality (unless these were never intended to be fixed). Maybe there is a use case for it, though.

Features: For Review (Not in Lucene)

Feature Name	Summary	Notes
RandomizedTesting (#264)	Dependency of Lucene test-framework that extends JUnit.	There may be some compelling reasons for implementing this. See: https://chatgpt.com/share/677a832d-3424-8005-97d7-1c024c68302d
Portable Random Seeds	Random seeds that produce the same result in both .NET and Java.	This would be very useful for debugging if we can achieve it. However, the benefit may not be worth the cost of pulling it off.
Enforce Custom Asserts	Provide build warnings for using NUnit asserts due to performance problems.	Our Assert class performs much better for collections and numeric types than NUnit does. Enforcing its usage using a Roslyn code analyzer would save us work of having to hunt down violations manually. This would be useful whether or not we finish #306, but it would be better if we finished that task first (or even consider moving it over to RandomizedTesting or another package).
Disallow using NUnit TestFixtureAttribute	Enforce the use of the custom TestFixtureAttribute when subclassing LuceneTestCase	Using NUnit's TestFixtureAttribute will bypass some of the test framework setup, which will affect repeatability of random tests and probably cause other problems. Users should get build failures if they attempt this. They are free to use it on classes that do not extend LuceneTestCase, though.
Disallow using NUnit OneTimeSetUpAttribute and OneTimeTearDownAttribute	Enforce the use of the custom OneTimeSetUpAttribute and OneTimeTearDownAttribute when subclassing LuceneTestCase	Using NUnit's OneTimeSetUpAttribute or OneTimeTearDownAttribute will cause setup problems with the test framework because NUnit could call the user's class first. Users should get build failures if they attempt this. They are free to use it on classes that do not extend LuceneTestCase, though. This depends on #1087.

Features: Not Needed

Feature Name	Summary	Notes
SystemPropertiesInvariantRule STATIC_LEAK_IGNORED_TYPES	These property keys will be ignored in verification of altered properties. A TestRule that ensures system properties remain unmodified by the nested Statement. This can be applied both at suite level and at test level. This rule requires appropriate security permission to read and write system properties System#getProperties() if running under a security manager.	SystemPropertiesInvariantRule is in randomizedtesting. The values are { "user.timezone", "java.rmi.server.randomIDs" }, so this doesn't appear very useful in .NET since we don't have BCL system properties and time zone is not something that can be set on a given thread. Also, our system properties are readonly inside of the application (we only provide a default value if the system property is not supplied on the outside of the application), so there is no danger of a user overwriting them at runtime.
TestRuleStoreClassName ClassNameRule	Stores the suite name so you can retrieve it from getTestClass()	I don't think this is needed because NUnit tracks the name of the test class and keeps it in scope already.
TestRuleAssertionsRequired	Verifies that the caller passed the system property to enable assertions.	Being that the default setting when the test framework is engaged is true and we also fixed the tests so it is possible to run them (skipping some) with asserts disabled, we don't need this unless it is required by randomizedtesting for some reason. It is unlikely many users of the test framework will even be aware of this feature.
LuceneJUnit3MethodProvider	Backwards compatible test* method provider (public, non-static).	This is a helper class to provide JUnit3 naming functionality to later versions of JUnit to scan for tests (by name, not by attribute). Java-specific. Not needed.
QuickPatchThreadsFilter	Last minute patches. TODO: remove when integrated in system filters in rr.	This appears to be Java-specific and temporary. It was only used by SOLR. We don't need this.
RemoveUponClose	A Closeable that attempts to remove a given file/folder.	This removes a specific file, but only if the test was successful. Not sure what the point is. There are no references and it is internal. We don't need it.
Rethrow	Sneaky: rethrowing checked exceptions as unchecked ones. Eh, it is sometimes useful... Pulled from Java Puzzlers. see "http://www.amazon.com/Java-Puzzlers-Traps-Pitfalls-Corner/dp/032133678X"	This only applies to Java because in .NET we don't have checked exceptions. We don't need it. In all cases we can remove the try/catch altogether.
TestSecurityManager	A SecurityManager that prevents tests calling System#exit(int). Only the test runner itself is allowed to exit the JVM. Use this with -Djava.security.manager= org.apache.lucene.util.TestSecurityManager.	We don't need this because we don't have our own custom test runner. Given that a stand alone console app to run tests is not likely something that will ever exist even if we do implement randomizedtesting, we probably don't need it.