Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make assertions into a 1st class feature in the test framework #306

Open
NightOwl888 opened this issue Jul 7, 2020 · 3 comments
Open

Make assertions into a 1st class feature in the test framework #306

NightOwl888 opened this issue Jul 7, 2020 · 3 comments
Labels
design is:enhancement New feature or request pri:normal up-for-grabs This issue is open to be worked on by anyone
Milestone

Comments

@NightOwl888
Copy link
Contributor

We recently discovered that the assertions provided by NUnit do not provide any support for primitive types and in those cases will default to object which causes boxing/unboxing when doing assertions. This can be a huge drain on performance when this operation is done in a tight loop.

It is clear that end users will likely run into similar issues with testing that we did. While we have addressed the problem, we did so in 2 assertions types that are both internal. We need to create a public API for assertions that end users of the test framework can use.

  • Missing overloads need to be added to support all of the operations that NUnit does
  • Custom asserts that we created need to be expanded to include all primitive types
  • A logical way to make these asserts "native" in LuceneTestCase and all of its subclasses needs to be devised

In Lucene, LuceneTestCase simply subclassed Assert. However, due to a difference in naming conventions between NUnit and JUnit, this would seem odd. For example, instead of AssertEquals, the NUnit team named their method AreEqual. While this isn't such a problem, it would be inconsistent with all of the other custom assertions in the test framework (i.e. AssertAnalyzesTo).

In addition, API documentation should be provided for each of the assertions.

@NightOwl888 NightOwl888 added up-for-grabs This issue is open to be worked on by anyone design is:enhancement New feature or request pri:normal labels Jul 7, 2020
@NightOwl888 NightOwl888 added this to the 4.8.0 milestone Jul 7, 2020
@TheBeardedLlama
Copy link

Before putting any more work into nunit, I'd recommend giving xunit a go

I've always preferred it over nunit (and mstest)

@NightOwl888
Copy link
Contributor Author

The Lucene test framework is unique in that it is a component that is meant for use by end users as well as our own tests. I did some research, and I cannot find another example of this on the .NET platform - it is unprecedented. In Java, the test framework uses a custom runner called Randomized Testing that is built on top of JUnit using its extensibility. The framework is inheritance-based, uses its own random seed functionality, and has its own attributes it needs to scan for during the test life cycle. Putting in enough functionality to actually debug the random tests (by outputting the random seed that was used during a failure so it can be plugged back in to debug) is a problem that still hasn't been completely solved yet.

I asked the NUnit team, and they don't currently support the extensibility points to build your own test runner. So, in the spirit of not re-inventing the wheel we are trying to make do with only NUnit's features out of the box. Not all of the features of the test framework are supported this way (such as running the tests in a random order), but I believe we have enough of them to get by without our own test runner. NUnit is the only framework that comes close to being able to support an inheritance-based framework without a ton of research and tradeoffs.

In case you haven't been following along for the past few years, @conniey worked on converting to xUnit for several months back in 2016-2017 and ultimately came to the conclusion that it wasn't possible. But I believe she was trying to take advantage of xUnit's parallel features.

In 2019, we tried converting the test framework to both xUnit and MSTest in addition to NUnit.

MSTest was completely impossible to support due to the fact that it doesn't scan for its own attributes in base classes. Maybe that has changed, but it wasn't possible in 2019.

xUnit forces you to inject state into the constructor in order to get any control over what would ordinarily be [BeforeClass] and [AfterClass] attributes. This puts an unnecessary burden on end users who then have to pass your state object in order to inherit your base class. I did determine that it may be possible to make our tests run with xUnit by turning off its parallel feature and it may even be possible to support parallel testing by refactoring several of Lucene's classes to inject state rather than relying on static variables. However, I am not sure if all of the test framework features we need could be supported. xUnit wasn't designed with an inheritance-based test framework that is meant for 3rd parties in mind, and you have to manually do a lot of the things that are included right out of the box with NUnit, such as scanning for custom-made attributes.

Ultimately, xUnit will be a lot more work to support than NUnit, and the fact that xUnit doesn't support an inheritance-based model that "just works" without forcing end users to override your constructor makes it less desirable than NUnit.

So, after over a year of research and trial and err, we determined that:

  1. For the short term at least through to the 4.8.0 release, NUnit is our only viable choice of the top 3 test frameworks
  2. For the long term, it would be better to port Randomized Testing so we can fully support debugging random tests, but we would have to work closely with the NUnit team to ensure there is enough extensibility to support it

@NightOwl888
Copy link
Contributor Author

NightOwl888 commented Jan 19, 2024

Just an update on our NUnit integration.

We added the repeatable randomized testing functionality in #547, so now it is much easier to debug random tests. This was done primarily by implementing our own [TestFixture] attribute that enables running the functionality. This attribute is placed within LuceneTestCase so even if any test file has a using NUnit.Framework at the top, the compiler will choose our [TestFixture] instead of the one in NUnit as long as it is a subclass of LuceneTestCase.

The [SuppressCodecs] attribute functions inside of the TestRuleSetupAndRestoreClassEnv, which is executed by NUnit when it calls our LuceneTestCase methods that are decorated with the [OneTimeSetup] and [OneTimeTearDown] attributes. Unfortunately, this means that a user can disable this functionality by overriding these methods without calling the base class (we probably should address that).

We also added [AwaitsFix], [Nightly], [Weekly], and [Slow] attributes so we can classify tests accordingly. These attributes extend NUnit interfaces so NUnit will execute them. Some of them still have issues because they run the most time consuming bits of the test before the attribute has a chance to run. See #739.

We are still missing the [BadApple] attribute, which we don't really have a use for. This was for putting the tests into a "vault" in Jenkins if they fail frequently.

The [SuppressTempFileChecks] attribute still does not function, which we should address at some point. See: #898.

So, we are very much integrated with NUnit at this point, and I am not sure there is another test framework that can support all of this functionality (at least not without making our own fork of it).

That said, NUnit 4 has already shipped and it is not yet known how much work is involved with supporting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design is:enhancement New feature or request pri:normal up-for-grabs This issue is open to be worked on by anyone
Projects
None yet
Development

No branches or pull requests

2 participants