3
\$\begingroup\$

I have stumbled across these unit tests in a code review that are using in memory db:

private DatabaseContext _context;
private Fixture _fixture;

[SetUp]
public void Setup()
{

    _fixture = new Fixture();
    _fixture.Customize(new AutoNSubstituteCustomization());

    var options = new DbContextOptionsBuilder<DatabaseContext>()
    .UseInMemoryDatabase(databaseName: "testdb")
    .Options;
    _context = new DatabaseContext(options);
}

[TearDown]
public void CleanUp()
{
    var context = _context;
    if (context == null || context.Database.ProviderName != "Microsoft.EntityFrameworkCore.InMemory")
    {
        return;
    }

    context.Database.EnsureDeleted();
    _context = null;
}

#region EmptyDB
[Test]
public void Test1()
{
    // Setup
    var logger = _fixture.Freeze<ILogger<UserRepository>>();

    var userRepo = new UserRepository(_context, logger);

    var userViews = new List<UserView>();
    userViews.AddRange(_fixture.CreateMany<UserView>(10));

    // ACT
    userRepo.UpdateUsers(userViews, CancellationToken.None).GetAwaiter().GetResult();

    // ASSERT
    Assert.AreEqual(10, _context.Users.CountAsync().GetAwaiter().GetResult());
}

[Test]
public void Test2()
{
    // Setup
    var logger = _fixture.Freeze<ILogger<UserRepository>>();

    var userRepo = new UserRepository(_context, logger);

    var identityViews = new List<IdentityView>();
    _fixture.Register<IEnumerable<UserView>>(() =>
    {
        return new UserView[] { new UserView("fish") };
    });
    userViews.AddRange(_fixture.CreateMany<UserView>(10));

    // ACT
    userRep.UpdateUsers(userViews, CancellationToken.None).GetAwaiter().GetResult();

    // ASSERT
    Assert.AreEqual(10, _context.Users.CountAsync().GetAwaiter().GetResult());
}

As you can see, the tests are using the same in memory db, which I really don't like. I also don't like the new UserRepository(_context, logger). Is it a bad practice to use the new-keyword like this?

I would prefer something like this instead:

[Test]
public void Test1()
{
    // Setup
    var provider = RegisterServices();  
    var logger = _fixture.Freeze<ILogger<UserRepository>>();
    var userRepo = provider.GetRequiredService<IUserRepository>();

    var userViews = new List<UserView>();
    userViews.AddRange(_fixture.CreateMany<UserView>(10));

    // ACT
    userRepo.UpdateUsers(userViews, CancellationToken.None).GetAwaiter().GetResult();

    // ASSERT
    Assert.AreEqual(10, _context.Users.CountAsync().GetAwaiter().GetResult());
}

private ServiceProvider RegisterServices([CallerMemberName] string memberName = "")
{
    var services = new ServiceCollection();

    services.AddDbContext<IDatabaseContext, DatabaseContext>(options =>
        options.UseInMemoryDatabase(memberName));

    services.AddPersistence("https://localhost");       

    return services.BuildServiceProvider();
}

As you can see, I have added a RegisterService method that takes the calling test as a parameter, and then uses this to create the inmemorydb. I really like this because you are isolating your tests more this way. I also think it's cleaner to read.

How would you guys do in this case? Is the first approach the way to go, or is my approach the more "right" way to do it? Or is it another better and more best practice way to do it?

I just want to know your opinions about this and about the two approaches above.

\$\endgroup\$
  • 1
    \$\begingroup\$ To anyone down voting or voting to close, while the first block of code is indeed copied, the second block is the posters attempt to rewrite it. They are looking for a comparison. \$\endgroup\$ – pacmaninbw May 31 at 21:15
0
\$\begingroup\$

Generally speaking whenever we are about to write unit tests we should follow the F.I.R.S.T. principles. It is an acronym, which stand for:

  • Fast: The execution time should be measured in milliseconds. If it takes a second or two then it can be considered as slow and should be revised.
  • Isolated: Each test is self-contained and does not rely on any other data. They can be run in any order.
  • Repeatable: The test runs must be repeated over and over again. In each and every run they should either pass every time or always fail.
  • Self-validating: There is no need for human interpretation whether or not the test succeeded or failed. The test result should be self-explanatory.
  • Timely: The code and related tests should be written in almost the same time. A new code without relevant unit tests should not be deployed.

Let's examine these ideas for your proposals:

Single database and cleanup

  • Fast: If for whatever reason a previous cleanup phase missed / failed then your database will have some trash data in it, which might impact the performance of your database operations. Executing the cleanup during setup and teardown might solve the problem, but it will definitely have performance impact.
  • Isolated: They are sharing the same database, so they are not truly isolated. It might be the case that they can't run in parallel, because ordering might matter.
  • Repeatable: Because they are using the same database, that's why order might affect the result of your assertions. In case of MSTest you can define ordering but if you need to use them that means your tests are not really isolated.
  • Self-validating: Because there is a chance for race condition your test results are non-deterministic, which means human-intervention is needed to interpret several resultsets, reproduce the issues (if it is even possible) and fix them.
  • Timely: It is irrelevant in our discussion

Separate database for each test case

  • Fast: Creating new in-memory database for each test should not impose performance penalty onto tests if there not too many tables and constraints.
  • Isolated: Separate databases are used for each test, means no shared resource is being used, which helps isolation.
  • Repeatable: Because each and every time you run your test against a brand new database, there won't be any trash data, which could cause race condition.
  • Self-validating: By being deterministic, no human intervention is needed to understand the test results.
  • Timely: It is irrelevant in our discussion

If you don't want to examine the test data manually, then you don't really need use the test name in the database name. You can use any random value:

int jitter = idGenerator.Next();
var condigBuilder = DbContextOptionsBuilder<TestContext>()
    .UseInMemoryDatabase(databaseName: $"TestDb{jitter}") 
    .Options;

or

Guid jitter = Guid.NewGuid();
var condigBuilder = DbContextOptionsBuilder<TestContext>()
    .UseInMemoryDatabase(databaseName: $"TestDb{jitter}") 
    .Options;
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.