3
\$\begingroup\$

I am currently writing my first TDD application. The project is in Xamarin.Forms and tested in xUnit. I am also using Autofac and Moq.

The first class below is testing second class (VM). The VM is:

  • LoadGroups method is populating collection of group names (strings),
  • LoadFromFile method is loading from CSV file string with data and creates collection of objects,
  • PopulateDb method takes collection of objects and seeds DB with them.

I wonder if maybe more experienced developers will have any comments or suggestions regarding the code or architecture, before I will continue with next View Models, to avoid corrections.

Test class:

public class MainPageViewModelTests
    {
        List<Phrase> phrases;
        private MainPageViewModel _viewModel;
        private Mock<IPhraseEditViewModel> _phraseEditViewModelMock;
        private Mock<IMainDataProvider> _mainDataProviderMock;
        public MainPageViewModelTests()
        {
            //instances
            phrases = new List<Phrase>
            {
                new Phrase { Category = "newCat1", Definition = "newDef1", Group = "newGr1", Learned = false, Name = "newName1", Priority = "newPrio1", Id = 7 }
            };
            _phraseEditViewModelMock = new Mock<IPhraseEditViewModel>();
            _mainDataProviderMock = new Mock<IMainDataProvider>();

            //setup
            _mainDataProviderMock.Setup(dp => dp.GetGroups())
                .Returns(new List<string>
                {
          "Group #1",
          "Group #2",
          "Group #3"
             });
            _mainDataProviderMock.Setup(dp => dp.PickUpFile())
                .ReturnsAsync("goodData.csv");
            _mainDataProviderMock.Setup(dp => dp.GetStreamFromCSV("goodData.csv"))
                 .Returns("Name|Definition|Category|Group|Priority\nname1 |def1|cat1|gr1|prio1\nname2 |def2|cat2|gr2|prio2");
            _mainDataProviderMock.Setup(dp => dp.GetStreamFromCSV("emptyData.csv"))
                .Returns("");

            //VM instance
            _viewModel = new MainPageViewModel(_mainDataProviderMock.Object, CreatePhraseEditViewModel);
        }

        private IPhraseEditViewModel CreatePhraseEditViewModel() //method for creating PhraseEditVM
        {
            var phraseEditViewModelMock = new Mock<IPhraseEditViewModel>();
            phraseEditViewModelMock.Setup(vm => vm.LoadPhrase(It.IsAny<int>()))
              .Callback<int?>(phraseId =>
              {
                  phraseEditViewModelMock.Setup(vm => vm.Phrase)
            .Returns(new Phrase());
              });
            _phraseEditViewModelMock = phraseEditViewModelMock; //field = var(!!)
            return phraseEditViewModelMock.Object;
        }

        [Fact]
        public void LoadGroups_ShouldLoadOnce_True()
        {
            _viewModel.LoadGroups(); //loads groups twice
            _viewModel.LoadGroups();

            Assert.Equal(3, _viewModel.Groups.Count); //counts how many groups are loaded
        }
        [Fact]
        public void LoadGroups_ShouldLoad_True()
        {
            _viewModel.LoadGroups(); //loads collection of groups (from setup)
            Assert.Equal(3, _viewModel.Groups.Count); //counts groups
            var phrase = _viewModel.Groups[0];
            Assert.NotNull(phrase);
            Assert.Equal("Group #1", phrase); //compares group name
        }
        [Fact]
        public void AddPhrase_ShouldBeExecuted_True()
        {
            _viewModel.PhraseEdit = false; //set up PhraseEdit prop
            _viewModel.AddPhraseCommand.Execute(null); // executes command
            Assert.True(_viewModel.PhraseEdit); //verifies PhraseEdit prop
            _phraseEditViewModelMock.Verify(vm => vm.LoadPhrase(null), Times.Once); //counts loaded phrases
        }
        [Fact]
        public void LoadFromFile_ShouldConvertReturnedCorrectFormatString_ReturnsPhraseList()
        {
            _viewModel.LoadFromFile("goodData.csv"); //loads phrases from the file
            Assert.Equal(2, _viewModel.LoadedPhrases.Count); //counts loaded phrases from the file
            var phrase = _viewModel.LoadedPhrases[0];
            Assert.NotNull(phrase); //checks if phrase is not null, below compares props
            Assert.Equal("name1", phrase.Name);
            Assert.Equal("def1", phrase.Definition);
            Assert.Equal("cat1", phrase.Category);
            Assert.Equal("gr1", phrase.Group);
            Assert.Equal("prio1", phrase.Priority);
        }
        [Fact]
        public void PopulateDb_ShouldSeedDbWithPhrases_CallsDpSavePhrase()
        {
            _viewModel.LoadedPhrases = phrases; //populates collection
            _viewModel.PopulateDb(_viewModel.LoadedPhrases); //populates Db with phase list - 1 item
            _mainDataProviderMock.Verify(dp => dp.SavePhrase(It.IsAny<Phrase>()), Times.Once); //counts saved phrases
        }
        [Fact]
        public void LoadFile_ShouldBeExecuted_CallsOnLoadFileExecute()
        {
            _viewModel.LoadFile.Execute(null); //execute command
            Assert.Equal(2, _viewModel.LoadedPhrases.Count()); //counts loaded phrases from the file
            Assert.Equal(3, _viewModel.Groups.Count); //counts loaded groups
            _mainDataProviderMock.Verify(dp => dp.SavePhrase(It.IsAny<Phrase>()), Times.AtLeast(2)); //counts saved phrases
        }
        [Fact]
        public void PopulateDb_ShouldSeedDbOnce_True()
        {
            _viewModel.LoadedPhrases = phrases; //populates collection
            _viewModel.PopulateDb(_viewModel.LoadedPhrases); //seeds Db twice
            _viewModel.PopulateDb(_viewModel.LoadedPhrases);
            _mainDataProviderMock.Verify(dp => dp.SavePhrase(It.IsAny<Phrase>()), Times.Once); //should seed only once
        }
        [Fact]
        public void LoadFromFile_WithFilePathParameterIsNull_ReturnsEmptyCollection()
        {
            List<Phrase> expected = new List<Phrase>();
            expected.Clear(); //expectations
            List<Phrase> method = _viewModel.LoadFromFile("");//loads phrases from the file with empty path parameter
            _viewModel.LoadFromFile(""); //loads phrases from the file with empty path string
            Assert.Empty(_viewModel.LoadedPhrases); // check if LoadedPhrases is empty
            Assert.Equal(expected, method); //compare expectations with method returns
        }
        [Fact]
        public void PopulateDb_GetsEmptyCollectionParameter_DoesNothing()
        {
            _viewModel.LoadedPhrases.Clear(); //collection is empty
            _viewModel.PopulateDb(_viewModel.LoadedPhrases); //PopulateDb with empty collection
            _mainDataProviderMock.Verify(dp => dp.SavePhrase(It.IsAny<Phrase>()), Times.Never); //with empty collection SavePhrase runs never
        }
        [Fact]
        public void LoadFromFile_GetsPathToEmptyFile_ReturnsEmptyCollection()
        {
            List<Phrase> expected = new List<Phrase>();
            expected.Clear(); //expectations
            List<Phrase> method = _viewModel.LoadFromFile("emptyData.csv"); //loads phrases from the file with empty content
            Assert.Empty(_viewModel.LoadedPhrases); // check if LoadedPhrases is empty
            Assert.Equal(expected, method); //compare expectations with method returns
        }

        //TODO:
        //zły format pliku
        //brak | w pliku
    }

Tested View Model class:

public interface IMainPageViewModel
    {
        void LoadGroups();
    }
    public class MainPageViewModel : ViewModelBase, IMainPageViewModel
    {
        List<Phrase> oldPhrases = new List<Phrase>(); //verification for PopulateDb method;
        private Func<IPhraseEditViewModel> _phraseEditVmCreator;
        private IMainDataProvider _dataProvider;
        public string FileLocation { get; set; }
        public ObservableCollection<string> Groups { get; set; }
        public List<Phrase> LoadedPhrases { get; set; }
        public bool PhraseEdit { get; set; }
        public IPhraseEditViewModel SelectedPhraseEditViewModel { get; set; }
        public MainPageViewModel(IMainDataProvider dataProvider,
            Func<IPhraseEditViewModel> phraseditVmCreator) //ctor
        {
            _dataProvider = dataProvider;
            _phraseEditVmCreator = phraseditVmCreator;
            Groups = new ObservableCollection<string>();
            LoadedPhrases = new List<Phrase>();
            //commands tests
            AddPhraseCommand = new DelegateCommand(OnNewPhraseExecute);
            LoadFile = new DelegateCommand(OnLoadFileExecute);
        }

        public ICommand AddPhraseCommand { get; private set; }
        public ICommand LoadFile { get; private set; }

        private void OnNewPhraseExecute(object obj)
        {
            SelectedPhraseEditViewModel = CreateAndLoadPhraseEditViewModel(null);
        }

        private IPhraseEditViewModel CreateAndLoadPhraseEditViewModel(int? phraseId)
        {
            //Application.Current.MainPage.Navigation.PushAsync(new PhraseEditPage());
            var phraseEditVm = _phraseEditVmCreator();
            PhraseEdit = true;
            phraseEditVm.LoadPhrase(phraseId);
            return phraseEditVm;
        }
        private async void OnLoadFileExecute(object obj)
        {
            LoadedPhrases.Clear();
            FileLocation = await _dataProvider.PickUpFile();
            LoadedPhrases = LoadFromFile(FileLocation);
            PopulateDb(LoadedPhrases);
            LoadGroups();
        }
        public void LoadGroups() //loads group list from the DB
        {
            Groups.Clear();
            foreach (var group in _dataProvider.GetGroups())
            {
                Groups.Add(group);
            }
        }
        public List<Phrase> LoadFromFile(string filePath)
        {
            if (filePath != "")
            {
                string stream = "";
                LoadedPhrases.Clear();
                stream = _dataProvider.GetStreamFromCSV(filePath);
                Dictionary<string, int> myPhraseMap = new Dictionary<string, int>(); //exception for wrong format
                var sr = new StringReader(stream);
                using (var csv = new CsvReader(sr, true, '|'))
                {
                    int fieldCount = csv.FieldCount;
                    string[] headers = csv.GetFieldHeaders();
                    for (int i = 0; i < fieldCount; i++)
                    {
                        myPhraseMap[headers[i]] = i;
                    }
                    while (csv.ReadNextRecord())
                    {
                        Phrase phrase = new Phrase
                        {
                            Name = csv[myPhraseMap["Name"]],
                            Definition = csv[myPhraseMap["Definition"]],
                            Category = csv[myPhraseMap["Category"]],
                            Group = csv[myPhraseMap["Group"]],
                            Priority = csv[myPhraseMap["Priority"]],
                            Learned = false
                        };
                        LoadedPhrases.Add(phrase);
                    }
                }
            }
            else
            {
                LoadedPhrases.Clear();
            }
            return LoadedPhrases;
        }
        public void PopulateDb(List<Phrase> phrases)
        {
            if (oldPhrases != phrases) //populates only if collection is new
            {
                foreach (var item in phrases)
                {
                    _dataProvider.SavePhrase(item);
                }
                oldPhrases = phrases;
            }
        }
    }

GitHub repository of the project

\$\endgroup\$
2
\$\begingroup\$

MainPageViewModel

You provide an interface with a single method:

public interface IMainPageViewModel
{
    void LoadGroups();
}

Yet, the implementation provides a rich set of public members:

public string FileLocation { get; set; }
public ObservableCollection<string> Groups { get; set; }
public List<Phrase> LoadedPhrases { get; set; }
public bool PhraseEdit { get; set; }
public IPhraseEditViewModel SelectedPhraseEditViewModel { get; set; }
// and so on ..

I'm not a fan of this, because this means there is a clear difference between the public API of the implementation and its interface. Perhaps some of these members are specific to the implementation, but surely ObservableCollection<string> Groups { get; } should be part of the interface. Or IReadOnlyCollection<string> if only readonly mode is allowed. I guess the former would be required for your use cases.

You are mixing naming conventions lowercase oldPhrases vs _lowercase _dataProvider. Stick to a single standard.

Be careful designing mutable properties ObservableCollection<string> Groups { get; set; }. It allows consumers to bypass the interface method LoadGroups() to set Groups. This might be considered a breach of encapsulation. Also, a null reference can easily occur:

public void LoadGroups()
{
    Groups.Clear();  // possible NullReferenceException
    foreach (var group in _dataProvider.GetGroups())
    {
        Groups.Add(group);
    }
}

There is alot to say about DelegateCommand using an async void action like OnLoadFileExecute. I would opt to use a (custom) AsyncDelegateCommand that takes a Task instead, optionally with a nested CancelCommand.

Method LoadFromFile mixes IO, mapping and changing instance state. This might be a bit too much for a single method. Consider splitting this up in classes/methods that each have their own responsibility. Readability could also be improved by inverting the if-statement with an early exit to avoid deep nested statements.

if (string.IsNullOrEmpty(filePath))
{
    LoadedPhrases.Clear(); // assuming the instance can never be null
    Clear();
    return;
}

// remaining method body ..

If filePath is null, you probably get an annoying exception, so prefer string.IsNullOrEmpty over != "".

Use var when the type is obvious to write more compact and clean code: Phrase phrase = new Phrase(.. -> var phrase = new Phrase(...

PopulateDb doesn't seem right. You check on reference equality of the collections. But what if a different stores the same or some of the same items? You would save certain items more than once: _dataProvider.SavePhrase(item); -> if the item is also contained in oldPhrases. You have many unit tests, but in TDD you should also find these edge cases I would expect.

if (oldPhrases != phrases) //populates only if collection is new

MainPageViewModelTests

You have written white-box tests, and you've done it the correct way. The only class you instantiate is the class under test:

_viewModel = new MainPageViewModel(..

While other dependencies get mocked:

_phraseEditViewModelMock = new Mock<IPhraseEditViewModel>();
_mainDataProviderMock = new Mock<IMainDataProvider>();

Allowing for tests against the dependencies without needing to worry about any of their possible implementations:

_phraseEditViewModelMock.Verify(vm => vm.LoadPhrase(null), Times.Once);

However, you did not write any significant number of black-box tests: tests where you check output given some input. By writing more of these tests for edge cases in particular, you would have found a couple of NullReferenceException errors.

| 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.