[phpunit] Trudne początki.

Hej, sam nie wiem jak nazwać ten temat, niby testy nappisane, konsola na zielono, ale jakoś sam kod mnie nie przekonuje. Wiadomo każdy jakoś zaczynał, ale mam wrażenie, że samo testowanie mi nie wychodzi.
Klasa : https://pastebin.com/kyBngF5x
Testy : https://pastebin.com/UaUrDXi3
Wątpliwości :

• nazwy funkcji - klasa wygenerowana przez phpstorma, ale może zamiast test+NazwaFunkcji lepiej opisać co dana metoda testuje
  np. testFromArray miałoby nazwę testThatYouCanMakePropertiesFromArray
• dużo kodu się powtarza, mam na myśli ten blok, który zawsze robi to samo

$this->assertJson($result);
$this->assertEquals($result, json_encode($this->payload));

czy powinienem przenieść to do oddzielnej funkcji i przekazywać jako argumenty? Czy może zrobić private $apiResponse @before nadpisywać nową instancją klasy i w @before dawać te testy?
W końcu czy public function testFromArray() jest poprawny?

Prawdę mówiąc ta Twoja klasa nic nie robi - na przemian przypisujesz coś do pól klasy i wołasz json_encode / json_decode, nic wartego testowania IMO.

Jeśli chcesz poćwiczyć testowanie, napisz sobie np. klasę do gry w kółko i krzyżyk, i z tym kombinuj ;-)

Patryk27 napisał(a):

Prawdę mówiąc ta Twoja klasa nic nie robi

wiem że nic nie robi, tak jakoś prosto chciałem zacząć.

No to zacznij od tego kółka i krzyżyk ;-)

Powiedzmy, aby klasa miała taki interfejs:

class TicTacToe
{
    
    public function __construct(int $size)
    {
        /* ... */
    }

    public function setO(int $x, int $y): void
    {
        /* ... */
    }

    public function setX(int $x, int $y): void
    {
        /* ... */
    }

    public function getWinner(): ?string // 'O', 'X' lub `null`
    {
        /* ... */
    }

}

Napisz przykładowe testy oraz implementację, w takiej właśnie kolejności (tzn. zacznij od testów).

No dobra zrobiłem (może kod nie jest najpiękniejszy, ale tdd było) https://gitlab.com/jakubdomanski/ttd_tictactoe/tree/master/
No i nadal mam wątpliwości :
-Nazwy - przez to, że najpierw pisałem test a nie generowałem go, zrobiłem nazwy opisowe
-Refaktor kodu : powiedzmy że teraz chcę wyodrębnić funkcjonalności do klas (single responsibility) i znów mam sytuację że testy będą robić to samo, co w takim przypadku?

1. Poczytaj o array_fill (w kontekście metody initializeCheckerboard).

2. Metoda getCheckerboardFieldValue nie sprawdza czy koordynaty nie wychodzą poza tablicę + nazwa jest trochę długa. Biorąc pod uwagę, że mamy krótkie setO, setX - wystarczy równie krótkie get.

3. Nazwa oraz sygnatura isMoveValid sugeruje, że metoda zwróci false w momencie, gdy ruch będzie nieprawidłowy - ona jednak rzuca wyjątek. Co powiesz na coś takiego?

4. 'O' oraz 'X' są odpowiednikami magic numbers - wyrzuć je do stałych.

5. Metoda getWinner jest nieco zawoalowana - może więcej bardziej opisowych komentarzy (z prostymi rysunkami w ASCII itd.) pomogłoby w czytelności?

6. Komentarz XxY w konstruktorze nic nie wnosi IMO.

Jeśli natomiast chodzi o sam test - brawo, jest prawie ideolo :-)

1. testSetCheckboardSize jest zbędny - sprawdzasz w ten sposób konkretną implementację planszy do gry (fakt, że wykorzystuje tablice), a nie o to chodzi: powinieneś sprawdzać zachowanie.

   Ujmując inaczej: załóżmy, że bardzo zależy Ci na pamięci i chcesz przerobić TicTacToe tak, aby wewnątrz działało na stringach, a nie tablicach. No i masz problem, bo musisz teraz przerobić test - a przecież zachowanie klasy się nie zmieniło (setX wywołane dwa razy na tym samym polu nadal rzuci wyjątek etc.), tylko jej wewnętrzna implementacja.

2. IMO testSetO oraz testSetX są zbędne - zauważ, że testSetOMoveNotAllowed już pokrywa ten warunek. Tzn. jeśli by się okazało, że z jakiegoś powodu TicTacToe nie zapisuje informacji o ruchu gracza, wykrzaczy się testSetOMoveNotAllowed, więc tamte dwa początkowe testy są nadmiarowe.

3. "NULL" === $winner && $winner = null; co ta instrukcja robi?

4. Dlaczego w metodzie testGetWinner wykonujesz $this->assertEquals($marker, $ticTacToe->getCheckerboardFieldValue($x, $y));? Poprzednie testy to już pokrywają.

Ogólnie: very very good, zwłaszcza jak na początki w testach :-)

Poczytaj o array_fill (w kontekście metody initializeCheckerboard).

Przeczytałem, dzięki. Użyłem. Tylko nie jestem pewien czy aby dobrze taki kod jak stworzyłem się dobrze czyta.

Nazwa oraz sygnatura isMoveValid sugeruje, że metoda zwróci false w momencie, gdy ruch będzie nieprawidłowy - ona jednak rzuca wyjątek.

Dałem wyjątek żeby się pobawić testowaniem wyjątków. Ale tu masz 100% rację.

Metoda getWinner jest nieco zawoalowana - może więcej bardziej opisowych komentarzy (z prostymi rysunkami w ASCII itd.) pomogłoby w czytelności?

Jest, to prawda że zawalona, co do komentarzy to tylko te wymagane zostawiłem co dany kawałek robi. Ale dodam.

Dlaczego w metodzie testGetWinner wykonujesz $this->assertEquals($marker, $ticTacToe->getCheckerboardFieldValue($x, $y));?

Żeby potwierdzić czy pętla ustawiająca wartości pól dla testów nie ma błędów logicznych. Wydaje mi się, że powien zostać dla ewentualnej zmiany kodu testu.

Komentarz XxY w konstruktorze nic nie wnosi IMO.

Dałem go żeby pamiętać która oś to który wymiar tablicy.

testSetCheckboardSize jest zbędny - sprawdzasz w ten sposób konkretną implementację planszy do gry (fakt, że wykorzystuje tablice), a nie o to chodzi: powinieneś sprawdzać zachowanie.

Ok. Rozumiem, że powinna być w takim razie metoda, która zwraca wymiary planszy w klasie TicTacToe, a nie liczyć ilości z planszy. Tak?

"NULL" === $winner && $winner = null; co ta instrukcja robi?

Jeśli zmienna jest stringiem "NULL" ustawia ją na prawdziwego nulla, wiem że trudno się czyta, ale to przyzwyczajenie z którym walczę :D

IMO testSetO oraz testSetX są zbędne - zauważ, że testSetOMoveNotAllowed już pokrywa ten warunek. Tzn. jeśli by się okazało, że z jakiegoś powodu TicTacToe nie zapisuje informacji o ruchu gracza, wykrzaczy się testSetOMoveNotAllowed, więc tamte dwa początkowe testy są nadmiarowe.

Tak tu masz rację. Jestem trochę skonfundowany tym wszystkim, zacząłem sobie od testowania klas i oznaczałem co jest przetestowane przez annotacje @CoVerS. I jak widzisz w tdd miałem taki problem z tym że tego nie zrobiłem, bo nie wiem czy powinienem po fakcie oznaczać wszystko, nawet metody prywatne dla klasy jeśi metoda publiczna z nich korzysta?

Nadal szukam odpowiedzi na to czy jeśli zrobię ekstrakcję kodu do klas to czy je testować? np. tworzę CheckerboardInterface i później implementuje to w klasie Checkerboard i jej używam w TicTacToe (bo teoretycznie jest już to przetestowane)

Żeby potwierdzić czy pętla ustawiająca wartości pól dla testów nie ma błędów logicznych

Potwierdzą Ci to inne testy oraz metoda getWinner() - gdyby się okazało, że setO / setX nie działają prawidłowo, getWinner również nie zwróci poprawnego wyniku.

Dałem go żeby pamiętać która oś to który wymiar tablicy.

Naprawdę potrzebujesz do tego komentarza? ;-)

Rozumiem, że powinna być w takim razie metoda, która zwraca wymiary planszy w klasie TicTacToe, a nie liczyć ilości z planszy. Tak?

Technicznie taka metoda jest zbędna - tworząc obiekt klasy TicTacToe musisz już ten wymiar znać (czytaj: ustala go "coś wyżej", a nie sama klasa TicTacToe), więc nie wydaje mi się, aby był do tego potrzebny jakikolwiek getter.

Jesteś może w stanie opisać jakiś przypadek warty testowania, który opierałby się o to?

ale to przyzwyczajenie z którym walczę

Jeez, faktycznie - napisz po prostu if ;-p

Nadal szukam odpowiedzi na to czy jeśli zrobię ekstrakcję kodu do klas to czy je testować?

Jeśli jesteś w stanie wydzielić odpowiedzialność do innych klas (np. w tym przypadku można by wyrzucić sprawdzanie wygranej do odrębnej klasy), jak najbardziej warto to przetestować.

Pamiętaj tylko o tym, aby testy się nie duplikowały - tzn. jeśli zrobisz sobie jakąś klasę WinnerChecker, to nie powielaj potem testu jednostkowego dla TicTacToe::getWinner (ponieważ ten test nie wniesie nic nowego), chyba że pojawią się jakieś nowe przypadki skrajne.

Uprzątnąłem kod. Jeśli ma ktoś ochotę i czas skomentować jego "czystość" i czytelność to byłbym wdzięczny :)
git: https://gitlab.com/jakubdomanski/ttd_tictactoe/tree/master/

1. https://en.wikipedia.org/wiki/Checkerboard - zwróć uwagę na wielkość liter w tym słowie.

2. Komentarz Get checkerboard field value. w przypadku metody o nazwie getCheckerboardFieldValue jest masłem maślanym - równie dobrze mogłoby by go nie być, bo przecież nie wnosi nic nowego.

3. Tak samo komentarz Initialize checkerboard. do metody o nazwie initializeCheckerboard czy komentarz O marker do pola o nazwie O w klasie Marker - naprawdę: nie warto komentować wszystkiego jak leci, bo to tylko dodaje wizualnego szumu i wbrew pozorom utrudnia analizę kodu, ponieważ zamiast spojrzenia na samą metodę, analizuje się jeszcze odruchowo nic niewnoszący komentarz.

4. Dlaczego zdecydowałeś się zatrzymać przydługawe getCheckerboardFieldValue, lecz już krótkie setO oraz setX?

5. Metoda initializeCheckerboard jest zbędna.

6. Co powiesz np. o takim komentarzu? IMO jest znacznie lepszy niż suche determine winner vertical (które tak swoją drogą nie jest nawet prawidłowym angielskim zdaniem).

7. Dlaczego klasę Checkboard jako jedyną wydzieliłeś do podkatalogu? Jaką rolę pełni ten podkatalog?

8. W jakim celu utworzyłeś BoardInterface? Przecież nigdy nie pojawi się żadna druga implementacja.

9. Wyjątek BoardIsNotASquareException nie zostanie nigdy rzucony - całym sednem istnienia konstruktora, który przyjmuje tylko jeden wymiar planszy, jest to, że zawsze plansza będzie kwadratem. Prove me wrong.

10. Co się stanie gdy wywołam new Checkerboard(-5)? (tj. z ujemnym parametrem lub będącym zerem)

11. Poprzez getter getCheckerboard klasa TicTacToe ujawnia na zewnątrz swoją implementację (fakt, że wykorzystuje pod spodem właśnie klasę Checkerboard) - fachowo nazywa się to leaking abstraction. Wyobraź sobie, że chciałbyś teraz przerobić tę klasę tak, aby nie wykorzystywała już Checkerboard (np. tak, jak było przedtem) - no i jest problem, bo się okazuje, że mogłeś uzależnić wszystkie klasy wykorzystujące TicTacToe dodatkowo o klasę Checkerboard i robi się problem z pozbyciem się jej. A tak wcale być nie musi.

Odnośnie testów:

1. testSetCheckerboardSize jest zbędny - po to mamy konstruktor, aby złej planszy nie dało się w ogóle utworzyć. Tzn. jeśli np. podam złe wymiary w konstruktorze, powinien on już wtedy rzucić wyjątkiem, a nie radośnie udawać, że wszystko gra po to, by potem wyrzucić błąd podczas próby pobrania rozmiaru planszy.

2. W jakim celu masz tego break'a?

IMO za bardzo zapędzasz się w tworzenie kodu, który ma wyglądać uber-poważnie i być Java-like-bizness-style, podczas gdy nie jest wcale sztuką naklepać tysiące linijek kodu tylko do gry w kółko i krzyżyk, tylko właśnie na odwrót: less is more :-)

naprawdę: nie warto komentować wszystkiego jak leci

Wiem,za radą dodałem code sniffera a on daje warning w stormie, to dopisałem żeby się nie czepiał.

Dlaczego zdecydowałeś się zatrzymać przydługawe

Bo jestem podatny na sugestje :D Jak ktoś mi powie nazwę to trudno mi się od tego uwolnić.

W jakim celu masz tego break'a?

debuggowym, już nie mam :D

W jakim celu utworzyłeś BoardInterface

Aby można było utworzyć inną planszę niż kwadrat (choć to się gryzie z testem testSetCheckerboardSize).

jeśli np. podam złe wymiary w konstruktorze, powinien on już wtedy rzucić wyjątkiem

to tak oczywiste że aż smutno mi że tak nie zrobiłem :(

całym sednem istnienia konstruktora, który przyjmuje tylko jeden wymiar planszy, jest to, że zawsze plansza będzie kwadratem. Prove me wrong.

Co jeśli przyjmę, że y=x-1? Powinienem podawać dwa wymiary do konstruktora?

Co powiesz np. o takim komentarzu?

IMO bez róźnicy, jedynie zmiana na zdanie do mnie przemawia. Dlatego do ASCII-ARTU się wcześniej nie odnosiłem :D Ale dodam.

Aby można było utworzyć inną planszę niż kwadrat

https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it

Co jeśli przyjmę, że y=x-1? Powinienem podawać dwa wymiary do konstruktora?

Pierwotna wersja aplikacji zakładała kwadratową planszę.