Testy jednostkowe w legacy -> CakePHP + ActiveRecord

Witam,

chciałbym w tym temacie zadawać pytania odnośnie testów w istniejącym projekcie, w którym działam od dłuższego czasu.
Background: Na pokładzie mamy CakePHP 4 (czyli Active Record), PHP 7.4. Pierwotni twórcy wzięli sobie do serca zasady "Fat controllers" i "Fat models" (choć to drugie znacznie mniej), brak testów, kilka dużych serwisów, często głupie mylące nazwy, brak separacji kodu itd..., testów brak. Generalnie projekt jest napisany bardzo po "cake'owemu", staram się poprawiać różne kwiatki na tyle ile mam wiedzy, nowe funkcjonalności (jak np integracje) staram się pakować do osobnych namespaców poza frameworkiem, zamiast używać powiązanych z frameworkiem modeli/komponentów.

Chciałbym wprowadzić do projektu testy, ale jestem w temacie początkujący i mam wiele pytań więc mam nadzieję, że pomożecie mi nieco w nauce i rozwiewaniu wątpliwości.
ps. proszę sobie darować rady typu "CakePHP? Kto w tym jeszcze pisze... tylko symfony" :)

Do sedna... Pierwszy temat na rozgrzewkę. Mam model FactoryOrder, gdzie zapisuję poszczególne części zamówienia dla różnych fabryk. Poniżej kod:

public function createForOrder(array $orderItems = []): void
    {
        $groupedByFactory = $this->groupByFactory($orderItems);
        foreach ($groupedByFactory as $factoryGroup) {
            $factoryEntity = $this->newEmptyEntity();
            $factoryEntity = $this->patchEntity($factoryEntity, $factoryGroup);
            $this->save($factoryEntity);
        }
    }

    private function groupByFactory(array $orderItems = []): array
    {
        $groups = [];
        $deliveryPart = 1;
        foreach ($orderItems as $item) {
            if (!isset($groups[$item->factory_id])) {
                $groups[$item->cp_provider_id] = [
                    'order_id' => $item->order_id,
                    'factory_id' => $item->factory_id,
                    'factory' => $item->factory_name,
                    'delivery_part' => $deliveryPart,
                ];
                $deliveryPart++;
            }
        }
        return $groups;
    }

Chciałbym przetestować metodę groupByFactory, która powinna zwrócić mi odpowiednią strukturę przed zapisem, jednak jest to metoda prywatna. I teraz kilka pytań:

1. czy powinienem po prostu użyć ReflectionMethod::setAccessible w PHPUnit?
2. może powinienem zmienić private na public (to raczej zły pomysł, bo niby dlaczego taka metoda miałaby być częścią API klasy)
3. a może powinienem wydzielić tę metodę do serwisu, który powinien się zająć obróbką tych danych (wtedy metoda będzie public, więc nie ma problemu) -> to wydaje mi się najbardziej sensowne

Które rozwiązanie będzie najlepsze?

Wydaje mi się że rozwiązanie 3 lub po prostu testowanie metody createForOrder.
Osobiście nie testuję metod prywatnych, prywatne są metodami wewnętrznymi "na potrzeby" metod publicznych.
Testowanie metod publicznych powinno skutkować przetestowaniem metod prywatnych.

Tak na prawdę, to podchodzisz do tego trochę ze złej strony.

1. Chciałbym przetestować metodę groupByFactory - nie chciałbyś przetestować groupByFactory(), bo jasno widać żę to jest szczegół implementacyjny. Na potrzeby testów wyobraź sobie że cały kod z groupByFactory() jest w createForOrder(). I przetestuj createForOrder().
2. Tylko problem tutaj jest taki że Twoja logika groupBy-owania, czy czegoś tam; nie zwraca wyniku tylko robi $this->save(). Więc, jeśli ja miałbym przetestować taki kod to po prostu wywołałbym tą metodę na bazie in-memory, a potem zrobił selecta z bazy i sprawdził czy się zapisało to co miało zapisać - bo w gruncie rzecz to robi Twój model, więc to powinieneś przetestować. (bo rozumiem że $this->save() robi zapis do bazy?).

Odpowiadając na pytania:

1. czy powinienem po prostu użyć ReflectionMethod::setAccessible w PHPUnit? - na pewno nie. To byłoby to samo co zmiana na public, tylko że gorzej bo nie jawnie.
2. może powinienem zmienić private na public (to raczej zły pomysł, bo niby dlaczego taka metoda miałaby być częścią API klasy) - Jeśli chciałbyś wynieść samą tą funkcję do osobnej klasy, która umiałaby operować na kilku modelach, to spoko. Jeśli chciałbyś zmienić na public tylko po to żeby napisać test, to nie.
3. a może powinienem wydzielić tę metodę do serwisu, który powinien się zająć obróbką tych danych (wtedy metoda będzie public, więc nie ma problemu) owszem, mógłbyś wynieść tą funkcje, ale to będzie rozwiązanie raczej słabe. Jasno widać, że dlatego ta metoda jest w modelu bo jest ściśle koncepcyjnie związana z modelem (ma wysoką cohesion z tym modelem), więc rozdzielanie ich przyniesie mały efekt.

Pytanie jest takie, po co piszesz te testy:

• Chcesz po prostu napisać testy żeby były.
• Chcesz napisać testy, żeby pomogły Ci później refaktorować całość, bez obawy że coś zwalisz.

Jeśli chcesz napisać testy po to żeby były, to ja bym napisał test w którym odpalasz metodę, i robisz selecta i robisz asercję na tym co select zwrócił.

Jeśli chcesz napisać testy, żeby pomogły Ci później rafaktorować całość, to ja bym nie testował tego kawałka który wkleiłeś samego w sobie (bo i tak pasowałoby się go pozbyć później). Napisałbym tymczasowy gruby test kontrollera, który testuje: że jak zrobisz request z takim i takie parametrem, to zapisze się taki i taki rekord do bazy. Taki test pozwoli Ci zrefaktorować fat model do jakiejś sensowniejszej architektury, do której potem powinieneś napisać test.

Jeśli teraz napiszesz test tylko pod ten kawałek kodu który wkleiłeś (czyli pod createForOrder()) to utrudnisz sobie późniejszą refaktoryzację go, bo test będzie zbyt ściśle związany z tą strukturą.

Jeśli pytasz o testy, to widać że martwi Cię aktualny stan projektu, na tyle że chciałbyś coś zrobić. Z tego względu nie polecam brać pierwszych lepszych rzeczy, które Ci przyjdą do głowy - jak wydzielanie kodu do serwisu, tylko po to żeby przetestować; bo możesz się zakopać w test-hell niepotrzebnie. Mogę Ci powiedzieć dokładnie jak napisać testy pod tą aplikację, tak żeby to było wszystko zdrowo zrobione, tylko ten kawałek kodu który pokazałeś to jest za mało.

Jeśli chcesz dokłądniejszego opisu jak to należy przetestować dobrze, to musiałbyś wrzucić więcej kodu.