NightOwl888 commented on issue #1017:
URL: https://github.com/apache/lucenenet/issues/1017#issuecomment-2571635427
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<hr> 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 <hr> 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.<br><br>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.<br><br>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.<br><br>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<hr>
parentChainCallRule<br>parentChainCallRule.setupCalled<br>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<hr> threadAndTestNameRule<br>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](https://lucene.apache.org/core/4_8_0/test-framework/index.html)) | 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).<br><br>This
is not called anywhere by Lucene, it is meant for users. It is probably useful
and worth having. |
| closeAfterSuite()<hr> 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).<br><br>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. |
## Features: For Review
|
**Feature Name**
|
**Summary**
| **Notes** |
|----------------------------------|---------------------------------------------------------------------------------------------------------------------|-------------------------------|
| TestRuleIgnoreAfterMaxFailures <hr> SYSPROP_MAXFAILURES <br>
SYSPROP_FAILFAST <br> ignoreAfterMaxFailuresDelegate <br>
ignoreAfterMaxFailures <br> replaceMaxFailureRule() <br> 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).
<br><br> 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 <hr> 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.<br><br>By-name
list of ignored types like loggers etc | |
| RunListenerPrintReproduceInfo <hr>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.<br><br>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<br><br>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<hr> WithNestedTests | A JUnit RunListener that detects
suite/test failures. Lucene needs it because failures due to thread leaks
happen outside of any rule contexts.<hr>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. <br><br>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 <a
href="https://builds.apache.org/job/Lucene-BadApples-trunk-java7/";>"vault" plan
in Jenkins</a>. | 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 <hr> STATIC_LEAK_IGNORED_TYPES | These
property keys will be ignored in verification of altered properties.<br><br>A
TestRule that ensures system properties remain unmodified by the nested
Statement. This can be applied both at suite level and at test
level.<br><br>This rule requires appropriate security permission to read and
write system properties System#getProperties() if running under a security
manager. | SystemPropertiesInvariantRule is in randomizedtesting.<br><br>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<hr> 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...<br><br>Pulled from <a
href="http://www.javapuzzlers.com";>Java Puzzlers</a>. 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.<br><br>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. |
--
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
