Testing async method call from constructor

Question

I have a project where I want to build a more sophisticated ToDo list - basically a personal project management system. I'm just starting out with the project, and I'd like some feedback on whether my test methods are OK, as I'm pretty new to TDD. So far, for the main page that lists the daily ToDo items, I've got the following ViewModel:

public class MainVM : ViewModelBase
{
    private IEnumerable<ToDoItem> _toDoItems;
    private IRepository<ToDoItem> _toDoItemRepo;
    private bool _dataIsLoaded;

    public bool DataIsLoaded
    {
        get { return _dataIsLoaded; }
        set { Set(ref _dataIsLoaded, value, true); }
    }

    public IEnumerable<ToDoItem> ToDoItems
    {
        get { return _toDoItems; }
        set { Set(ref _toDoItems, value); }
    }

    public MainVM(IRepository<ToDoItem> toDoItemRepo)
    {
        _toDoItemRepo= toDoItemRepo;
        LoadData().ContinueWith(t => FinishedLoadingData(t));
    }

    private void FinishedLoadingData(Task loadTask)
    {
        switch (loadTask.Status)
        {
            case TaskStatus.RanToCompletion:
                DataIsLoaded = true;
                break;
            default:
                DataIsLoaded = false;
                break;
        }
    }

    public async Task LoadData()
    {
        if (!DataIsLoaded)
            ToDoItems= await _toDoItemRepo.GetAsync();
    }
}

As you can see, I want to load my data when the ViewModel is created.

I'm using the Set method from the MVVMLight toolkit to set the DataIsLoaded property- it sets the field behind and raises the PropertyChanged event for me.

I have the following test set up to test for a successful load:

    public void DataIsLoaded_is_true_if_loading_task_ran_to_completion()
    {
        AutoResetEvent testTrigger = new AutoResetEvent(false);

        TaskCompletionSource<IEnumerable<ToDoItem>> taskCompletion = new TaskCompletionSource<IEnumerable<ToDoItem>>();
        taskCompletion.SetResult(null);

        Mock<IRepository<ToDoItem>> repoMock = new Mock<IRepository<ToDoItem>>();
        repoMock.Setup(s => s.GetAsync()).Returns(taskCompletion.Task);

        MainVM vm = new MainVM(repoMock.Object);
        vm.PropertyChanged += (s, e) =>
        {
            if (e.PropertyName == nameof(MainVM.DataIsLoaded))
                testTrigger.Set();
        };

        testTrigger.WaitOne(5000);

        Assert.IsTrue(vm.DataIsLoaded);
    }

Now, I can see that there is a potential race condition here. The LoadData method could complete before the event handler is attached. As an alternative, I could use the Set method that broadcasts a message to MVVMLight's message handler in my ViewModel, and change my test method to this:

    [Test]
    public void DataIsLoaded_is_true_if_loading_task_ran_to_completion()
    {
        AutoResetEvent testTrigger = new AutoResetEvent(false);

        TaskCompletionSource<IEnumerable<ToDoItem>> taskCompletion = new TaskCompletionSource<IEnumerable<ToDoItem>>();
        taskCompletion.SetResult(null);

        Mock<IRepository<ToDoItem>> repoMock = new Mock<IRepository<ToDoItem>>();
        repoMock.Setup(s => s.GetAsync()).Returns(taskCompletion.Task);

        Messenger.Default.Register<PropertyChangedMessage<bool>>(
           this,
           message =>
           {
               if (message.PropertyName == nameof(MainVM.DataIsLoaded))
               {
                   testTrigger.Set();
               }
           });
        MainVM vm = new MainVM(repoMock.Object);

        testTrigger.WaitOne(5000);

        Assert.IsTrue(vm.DataIsLoaded);
    }

Now I don't have a race condition, but I'm a bit annoyed that I'm broadcasting a message that I don't really need for my code to work. I could wrap the whole thing in an pre-processor directive, of course, but that's just clunky.

Is there a better way to go about the whole thing?

Answer 1

Trying to load the data asynchronously in the constructor is a bad idea and in my opinion goes against what the constructor is for.

Constructors cannot be async, and asynchronous initialization can be seen as an implementation detail.

I suggest separating the initialization and data loading out ...

public class MainVM : ViewModelBase {
    private readonly IRepository<ToDoItem> repository;
    private IEnumerable<ToDoItem> items;    
    private bool _dataIsLoaded;
    private bool loading = false;

    public bool DataIsLoaded {
        get { return _dataIsLoaded; }
        set { Set(ref _dataIsLoaded, value, true); }
    }

    public IEnumerable<ToDoItem> ToDoItems {
        get { return items; }
        set { Set(ref items, value); }
    }

    public MainVM(IRepository<ToDoItem> repository) {
        this.repository = repository;
    }

    public async Task LoadData() {
        if (loading) return;

        if (!DataIsLoaded) {
            try {
                loading = true;
                ToDoItems = await repository.GetAsync();
                loading = false;
                DataIsLoaded = true; // task Ran To Completion
            } catch (Exception ex) {
                loading = false;
                //TODO: Log error
                DataIsLoaded = false; // task did not complete
            }
        }
    }
}

... and have whatever is binding/using to the view model (ie the view, another model, a test, etc) load the data after initialization.

This allows the testing to be much simpler and to the point

For example

[TestClass]
public class MainVmTests {
    [TestMethod]
    public async Task DataIsLoaded_is_true_if_loading_task_ran_to_completion() {
        //Arrange
        var repoMock = new Mock<IRepository<ToDoItem>>();
        repoMock.Setup(_ => _.GetAsync()).Returns(Task.FromResult(Enumerable.Empty<ToDoItem>()));
        var subject = new MainVM(repoMock.Object);

        //Act
        await subject.LoadData();

        //Assert
        Assert.IsTrue(subject.DataIsLoaded);
        Assert.IsNotNull(subject.ToDoItems);
    }

    [TestMethod]
    public async Task DataIsLoaded_is_false_if_loading_task_failed() {
        //Arrange
        var repoMock = new Mock<IRepository<ToDoItem>>();
        repoMock.Setup(_ => _.GetAsync()).Throws(new AggregateException());
        var subject = new MainVM(repoMock.Object);

        //Act
        await subject.LoadData();

        //Assert
        Assert.IsFalse(subject.DataIsLoaded);
        Assert.IsNull(subject.ToDoItems);
    }
}