Code review testów i architektury

0

Hej, stawiam pierwsze kroki w TDD i wykorzystaniu testów na żywym projekcie.

Piszę dla siebie swój pierwszy projekt w Xamarin.Forms . Żeby nie było zbyt łatwo, zdecydowałem się go pisać przy pomocy metodologii TDD i wzorca MVVM (plus inne warstwy).

Ciekaw jestem, czy bardziej doświadczeni koderzy będą mieli jakieś uwagi do dotychczasowych testów i architektury projektów. Aplikacja nie jest ukończona. Nim zabiorę się za kolejne widoki i modele widoków chcę się upewnić, że robię 'to' dobrze, co by uniknąć ewentualnych poprawek.

Testy są pisane przy użyciu xUnit, wykorzystuję też Moq i Autofac.

Klasa testowa pierwszego modelu widoku:
https://github.com/przemyslawbak/Flashcards/blob/master/Flascards.UnitTests/ViewModels/MainPageViewModelTests.cs

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
    }

Testowany model widoku:
https://github.com/przemyslawbak/Flashcards/blob/master/Flashcards/Flashcards/ViewModels/MainPageViewModel.cs

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;
            }
        }
    }

Repo projektu:
https://github.com/przemyslawbak/Flashcards/tree/master/Flashcards/Flashcards

1

Brakuje pustych linii między metodami.

0
somekind napisał(a):

Brakuje pustych linii między metodami.

Ale ja powaznie, nie wiem czy nie pominalem czegos przy testach, albo czy cos moze byc zle separowane lub wstrzykiwane.

0

W metodach też brakuje odstępów. To, co robisz, mija się z celem testów jednostkowych. Do tego, co starasz się uzyskać poleciłbym testowanie całych historyjek w stylu BDD.

0
Gworys napisał(a):

W metodach też brakuje odstępów. To, co robisz, mija się z celem testów jednostkowych. Do tego, co starasz się uzyskać poleciłbym testowanie całych historyjek w stylu BDD.

Myślałem, że jednostki powinny być jak najmniejsze, tak samo same testy.

A z tymi odstępami to poważnie?

0

Unit testy powinieneś stosować do sprawdzenia jakiejś pojedynczej "mechaniki" np. wyrażenie regex, etc. Testowanie obiektów na wyższym poziomie jak API aplikacji lub jeśli chcesz to ViewModel powinny być bardziej testami funkcyjnymi - akceptacyjnymi.

Przykład takiego testu "Logowanie":

        [Scenario]
        [ScenarioCategory(Categories.Security)]
        public void Successful_login()
        {
            Runner.RunScenario(

                Given_the_user_is_about_to_login,
                Given_the_user_entered_valid_login,
                Given_the_user_entered_valid_password,
                When_the_user_clicks_login_button,
                Then_the_login_operation_should_be_successful,
                Then_a_welcome_message_containing_user_name_should_be_returned);
        }
0

Już masz jakiegoś MainDataProvider - dlaczego on nie daje ViewModelowi gotowego IList<Phrase> czy czegoś, tylko daje mu strumień CSV i zmusza go, aby to robił sam? Ładowanie danych z pliku to nie jest funkcja ViewModelu.

0
Ktos napisał(a):

Już masz jakiegoś MainDataProvider - dlaczego on nie daje ViewModelowi gotowego IList<Phrase> czy czegoś, tylko daje mu strumień CSV i zmusza go, aby to robił sam? Ładowanie danych z pliku to nie jest funkcja ViewModelu.

ViewModel się pyta MainDataProvidera o GetStreamFromCSV, metodę tak naprawdę wykonuje DataRepository który implementuje interfejs. Model widoku nic nie pobiera z pliku bezpośrednio. VM otrzymuje string z pliku, który w VM jest przerabiany na kolekcję obiektów.

1 użytkowników online, w tym zalogowanych: 0, gości: 1