The unit tests need to be rearchitected

robertmclaws commented 2 years ago

Assemblies Affected

ASP.NET Core OData 7.x and later

Describe the Problem

Over the past several months, I have been experiencing a level of instability with this project that makes it difficult to rely on from build to build. As an example, there have been several documented instances in the last few months of package reference failures spanning multiple releases, and now I'm tracking down issues with Batching and a NullReferenceException inside Microsoft.AspNetCore.OData 7.6.1's ODataResourceDeserializationHelpers in were not in previous builds.

I believe the problem stems from the fact that the unit tests are entirely too complicated to maintain. I don't know if it's still this way now, but when I rearchitected the OData 8.x branch before its release, I noticed the following issues:

No fewer than 3 different ways to spin up unit tests
Each individual unit test has a ton of setup and boilerplate work before it can actually get to the things it's testing
There are not really clear distinctions between true unit tests and integration tests
There are not enough tests for unhappy paths

That cognitive load has a massive impact on maintainability and gives a false sense of security on code coverage.

We experienced the same thing on the Restier project several years ago and as a result I developed the Breakdance platform to make WebApi and OData testing easier.

Suggested Remediation

Now that Restier has gone GA and it comes with a built-in testing framework (Microsoft.Restier.Breakdance), I believe it is time to look at implementing Breakdance in the entire OData stack moving forward. It would eliminate thousands of lines of unnecessary code, give the team an opportunity to build true unit tests that cover more edge-cases per method, and let the team spend more time developing tests that can provide better stability guarantees.

Additional Context

Here's an example of how easy an integration test can be with Breakdance: https://github.com/OData/RESTier/blob/main/src/Microsoft.Restier.Tests.AspNet/FeatureTests/ActionTests.cs

Here's an example of leveraging DI container outputs between releases to maintain compatibility: https://github.com/OData/RESTier/blob/main/src/Microsoft.Restier.Tests.Legacy/LegacyDependencyInjectionTests.cs

Here's an example of how we use Breakdance to test Breakdance: https://github.com/OData/RESTier/blob/main/src/Microsoft.Restier.Tests.Breakdance/RestierBreakdanceTestBase_CoreTests.cs

julealgon commented 2 years ago

I believe the problem stems from the fact that the unit tests are entirely too complicated to maintain.

What is the source of the complication?

The reason I ask this is because I'm very skeptical of introducing entire "test frameworks" into the mix. The fact that you ended up in that situation to me seems to point to a very hard to test library: the fix to that is not creating very hard to maintain and custom testing logic, but to simplify the library itself so testing it becomes easier.

I've not looked into this Breakdance project, but my upfront tendency is to be extremely put off by it on principle that we are introducing complications to what should be simple to do using standard/native testing frameworks, mocking and integration testing tools.

Now, I'm obviously not part of the OData team (just an enthusiast of the library), but I'd personally be completely against introducing "a custom test framework" to the project unless there was an insanely strong reason for it and even then, probably if it was really the only viable option.

robertmclaws commented 2 years ago

@julealgon Appreciate the input! This issue was more for the dev team to consider, as they have considerably more context about Restier's place in the OData ecosystem, and why Microsoft shipped a test framework specifically for Restier.

OData / AspNetCoreOData