NightOwl888 opened a new issue, #1087: URL: https://github.com/apache/lucenenet/issues/1087
### Is there an existing issue for this? - [X] I have searched the existing issues ### Task description In Lucene, it was not possible to override the `BeforeClass`, `AfterClass`, `Before`, or `After` methods. This was enforced by the compiler by declaring the methods static. In Lucene.NET, we have defined these methods as virtual instance methods. This because NUnit calls the methods that implement the `[SetUp]`, `[OneTimeSetUp]`, `[TearDown]` and `[OneTimeTearDown]` attributes in the reverse order with respect to inheritance as JUnit and it was the quickest way to fix the calling order issue (and in turn, we were able to debug the tests sooner). For `SetUp()`, the `LuceneTestCase` base class needs to be called first, so this approach allows the most derived class to call the root base class by using `base.SetUp()` and cascading it through each level of inheritance. While this works for our internal purposes, it allows users to override the methods and not call the base class. This can disable critical functionality for setting up or tearing down the test framework, which ideally the user would not be able to do. I have analyzed NUnit thoroughly (in the past). Since these methods are called during the execution phase of the test run (which NUnit gives us no control over), there is no way to change the order. At least not without creating our own fork of NUnit. However, I thought of a way we might be able to fix this post-NUnit. 1. Declare the methods `SetUp()`, `OneTimeSetUp()`, `TearDown()` and `OneTimeTearDown()` as static as they were in Lucene using the original NUnit `[SetUp]`, `[OneTimeSetUp]`, `[TearDown]` and `[OneTimeTearDown]` attributes. These should be renamed beginning with a double underscore and be given the `EditorBrowsable(EditorBrowsableState.Never)]` attribute. 2. Create a set of `[SetUp]`, `[OneTimeSetUp]`, `[TearDown]` and `[OneTimeTearDown]` attributes that are nested inside of the `LuceneTestCase` class for users. These attributes will be owned by our test framework and unknown to NUnit. We can add an `Order` property that can optionally be set to define which order to execute them in when 2 or more are defined in the same class. This allows users to use attributes with names they are familiar with and as long as they don't fully qualify them, they will be our attributes rather than NUnit's. We use this approach already on `[TestFixtureAttribute]`. 3. (optional) Create a set of `[BeforeSetUp]`, `[BeforeOneTimeSetUp]`, `[AfterTearDown]` and `[AfterOneTimeTearDown]` attributes that give the user the option to execute code prior to setup and after teardown. Note that using custom attributes based off of NUnit interfaces may be an alternative to this (basically, if you need to run something before `SetUp()` or `OneTimeSetUp()` or after `TearDown()` or `OneTimeTearDown()`, create an attribute to do it and decorate either the class or the test, as appropriate). 4. When NUnit calls the `SetUp()`, `OneTimeSetUp()`, `TearDown()` and `OneTimeTearDown()` methods in `LuceneTestCase`, find the attributes for the current class and all classes that it inherits, order them appropriately, and execute them. ### Pros 1. Allows subclasses to use either static or instance methods. 2. Closely aligns with Lucene. 3. Creates a layer of indirection that we can use to potentially address other issues, such as #739. It will be straightforward to conditionally stop the call to the actual setup and tear down methods based on a custom attribute if we are calling these methods instead of NUnit. ### Cons 1. Less transparency in the calling order, since inheritance is easy to understand. 2. The compiler displays a warning when there is a static method with the same name in a base class and a derived class. Note that we should analyze how the Reflection calls are done in NUnit so we can optimize. It might make sense to piggyback off of the test discovery phase (`[TestFixtureAttribute]`) to store the metadata about where the attributes are located and even the appropriate order to call them in, which can be stored in the `RandomizedContext` class instance (which only applies to the currently running test). After it is implemented, we should also revert the existing `SetUp()`, `OneTimeSetUp()`, `TearDown()` and `OneTimeTearDown()` methods of subclasses to the way they were in Lucene. Most of them were static, and several of them had names other than these (although, we should stick with these naming conventions instead of the before/after conventions that were used in Java - see #1016). > The way this is supposed to work is that if a user subclasses `LuceneTestCase`, it will opt in to all of the features of the test framework, so we can rewrite the rules on how it functions that are differently than NUnit's normal rules. This is an all or nothing proposition, the user can only opt out by not subclassing `LuceneTestCase`. If a test class does not subclass `LuceneTestCase`, it will just be a "normal" NUnit test and won't have access to any of the test framework functionality. > > Maybe someday when we have it stable enough, we can create a separate [RandomizedTesting](https://github.com/NightOwl888/RandomizedTesting/tree/main/src/RandomizedTesting) package that actually does most of what the [randomized-testing](https://github.com/randomizedtesting/randomizedtesting) project does (see #257). When that is the case, there will be a base class of `LuceneTestCase` named `RandomizedTest` and the generic extensions to NUnit (such as these custom attributes) will move up a level to `RandomizedTest` so they can be reused in by other projects to implement randomized testing. -- 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.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
