Kod tylko do testów w kodzie produkcyjnym

Natknąłem się ostatnio na taki artykuł Testing Without Mocks: A Pattern Language

Artykuł dość długi, ale cała idea jest przedstawiona w sekcji Example. Z grubsza chodzi o to, żeby implementując infrastrukturę, napisać też implementację, która nie zależy od zewnętrznych narzędzi typu baza danych czy jakieś API. Taki "konfigurowalny" stub, ale to kod produkcyjny. Autor nazywa to Nullable. Potem w testach używamy tego Nullable zamiast mockować bazę, 3rd party API czy system plików.

Klepnąłem sobie prostą apkę z testami w takim stylu i muszę powiedzieć, że jest to całkiem przyjemne, ale strasznie mnie razi w oczy kod "tylko do testów" poplątany z kodem produkcyjnym. Już nie mówię nawet o tych Nullable'ach, ale to podejście wymaga czasem dodatkowych wywołań klas/metod, które służą tylko temu, żeby łatwiej (albo w ogóle) można było napisać asercję lub złapać wynik/efekt uboczny wywołania jakiejś funkcji. Przykład prosto od autora artykułu: implementacja CommandLine ma listener, który jest tam tylko po to, żeby potem w teście móc przechwycić output.

Po drugiej stronie spektrum jest podejście opisane tutaj (wybaczcie za film, nie mogę nigdzie tego znaleźć w formie tekstu). Tutaj z kolei autor poleca następujące podejście:

• zdefiniować zależności modułu (w OOP to będzie interface lub protocol),
• mockować zależności i sprawdzić, czy są dobrze wywoływane, np. jeśli testuję handler HTTP, to testuję, czy na GET /search&q=foobar wywołany zostanie SearchService.getResults("foobar"),
• stubować zależności i sprawdzić, czy handler HTTP jest w stanie obsłużyć wszystkie możliwe zwrotki od swoich zależności, np. stubuję SearchService tak, by getResults zwracało: pustą tablicę, jeden element, kilka elementów, dużo elementów i rzuciło wyjątek,
• moduł uznaję za przetestowany, a testując SearchService trzymam się zasady, że muszę napisać testy, które:
  • wywołują getResults z takimi parametrami, jakie sprawdzałem gdy mockowałem ten serwis w testach handlera HTTP, i osobno takie, które
  • robią setup w taki sposób, by getResults zwróciło mi wyniki, które zastubowałem testując handler HTTP

Po takich testach mam mieć pewność, że http handler poprawnie rozmawia ze swoimi zależnościami i potrafi obsłużyć to, co te zależności zwrócą, oraz, że service przyjmie i obsłuży to, co handler mu przekaże i w odpowiedniej sytuacji zwróci to, czego handler oczekuje.

Plus jest taki, że nie mieszam kodu produkcyjnego z testowym i nie muszę pisać dodatkowego kodu produkcyjnego tylko po to, żeby móc coś (łatwiej) przetestować. Minus jest taki, że jest dużo mocków, a mówią, że mocki są złe :)

Próbowałem po trochu obu podejść i o ile w drugim jest więcej klepania, mam wrażenie, że te testy są czytelniejsze, bardziej zwięzłe i nie testują tego samego 10 razy. Do tego łatwiej mi pisać aplikację "top-down", mogę się skupić na jednym module na raz, zależności zastubować/zamockować i martwić się nimi później. Jednakże używanie tylu mocków i stubów po prostu "doesn't feel right". Do tego nie ma narzędzi do sprawdzenia, czy wszystkie przypadki zakładane w testach handlera HTTP są przetestowane w testach serwisu, więc powiązanie jest dość luźne i wydaje się, że łatwo można popełnić błąd lub o czymś zapomnieć. Czy jest jakiś middleground między jednym a drugim, albo czy preferujecie jedno podejście od drugiego? Co sądzicie o samej idei przeplatania kodu produkcyjnego z testowym? Co sądzicie o pisaniu kodu produkcyjnego, który jest używany tylko w testach?

Według mnie kod nie powinień być konkretnie pod testy. Z drugiej strony kod powinień być napisany tak generycznie, żeby dało się go używać w różnych kontekstach tj. produkcyjne czy w testach.
Przypadek z listenerem jest IMO zły. Prawidłowo powinno się go przekazać przez konstrutkor (DI) w taki sposób, żeby nie potrzebne były takie pomocniczne metody.

Co do całości: po to masz interfejsy, żeby nie było konieczne mieszanie kodu produkcyjnego z testowym. Kod produkcyjny używa interfejsów w miejscach, gdzie chcesz mieć zmienne zachowanie i tyle. Idea przeplatania jest zła, bo cierpi na tym czytelność i utrzymanie. Jak dany kawałek kodu nie jest używany produkcyjnie to przyszły archeolog twojego kodu ma problem z ogarnięciem po cholerę to tutaj jest

Musiałeś to chyba źle zinterpretować, bo w sekcji "Nullable" nie ma nic o kodzie "tylko pod testy".

Jest tam tylko wstrzyknięta fabryka abstrakcyjna która zwraca albo prawdziwą instancję albo NullObject. Tzn, ten NullObject jest implementacją pod testy, ale to jest część samych testów, a nie kodu produkcyjnego.

Riddle napisał(a):

Musiałeś to chyba źle zinterpretować, bo w sekcji "Nullable" nie ma nic o kodzie "tylko pod testy".

Jest tam tylko wstrzyknięta fabryka abstrakcyjna która zwraca albo prawdziwą instancję albo NullObject. Tzn, ten NullObject jest implementacją pod testy, ale to jest część samych testów, a nie kodu produkcyjnego.

Zobacz na ten przykład https://github.com/jamesshore/testing-without-mocks-example/blob/javascript/src/infrastructure/command_line.js#L27 . Jest wystawiona statyczna metoda createNull, która wygląda jak kod tylko do testów.

slsy napisał(a):

Riddle napisał(a):

Musiałeś to chyba źle zinterpretować, bo w sekcji "Nullable" nie ma nic o kodzie "tylko pod testy".

Jest tam tylko wstrzyknięta fabryka abstrakcyjna która zwraca albo prawdziwą instancję albo NullObject. Tzn, ten NullObject jest implementacją pod testy, ale to jest część samych testów, a nie kodu produkcyjnego.

Zobacz na ten przykład https://github.com/jamesshore/testing-without-mocks-example/blob/javascript/src/infrastructure/command_line.js#L27 . Jest wystawiona statyczna metoda createNull, która wygląda jak kod tylko do testów.

No to zgadzam się, że faktycznie ten kod jest bez sensu.

Nie tak się powinno to zrobić.

Czyli jak dobrze rozumiem: stuby i mocki OK, ale w kodzie testowym, nie produkcyjnym?

Biorąc na tapet ten kawałek kodu konstruktor CommandLine powinien przyjmować jakiś interfejs, który byłby implementowany przez Node'owy process (albo nasz wrapper na process), a w testach... no właśnie. Jak przetestować, czy writeOutput robi to, co powinien? Czy process w testach powinien być zmockowany i sprawdzać, czy jest wywołane stdout.write z opowiednim parametrem? Czy może powinienem wstrzyknąć jakiś processSpy, który mi przechwyci wszystko, co wchodzi do stdout.write i zapisze w zmiennej, do której mam dostęp i mogę sobie napisać assert expectedOutput == processSpy.capturedOutput? W zasadzie to wiele się nie różni od mocka i assertCalledWith. Czy da się jakoś inaczej, najlepiej tak, by jak najmniej (w ogóle) nie wiązać testu z implementacją?

Ja bym to zrobił tak (pseudo-typescript):

//////////// kod produkcyjny
// command_line.ts
interface Process {
  argv(): string[];
  write(text: string);
}
class CommandLine {
  constructor(process: Process) {
     this.process = process;
  }

  args(): string[] {
    return this.process.argv().slice(2);
  }

  writeOutput(text: string) {
    this.process.write(text);
  }
}

// node_process.ts
class NodeProcess implements Process {
  argv(): string[] {
    return process.argv;
  }
  write(text: string) {
    process.stdout.write(text);
  }
}

//////////// main
// write passed args to stdout
let commandLine = CommandLine(NodeProcess());
commandLine.writeOutput(commandLine.args());

///////////// testy
// command_line_test.ts
class ProcessDouble implements Process {
  constructor(argv: string[]) {
    this.argv = argv;
    this.capturedOutput: string[] = [];
  }
  argv(): string[] {
    return this.argv;
  }
  write(text: string) {
    this.capturedOutput.push(text);
  }
  
}
describe('CommandLine', function() {
  it('gets args', function() {
    const args = ["foo", "bar"];
    let commandLine = CommandLine(ProcessDouble(["nodejs", "main", ...args]));
    assert.Equal(args, commandLine.args());
  });

  it('writes output', function() {
    const text = "hello";
    let process = ProcessDouble();
    CommandLine(process).write(text);
    assert.Equal(process.capturedOutput, [text]);
  });
})

// node_process_test.ts
// tutaj zrobiłbym test integracyjny, tj. odpaliłbym prawdziwy proces z parametrami i przechwytywałbym stdout

Czekam na krytykę :) tylko proszę bez nit-pickingu. Wiem, że ta klasa nie ma dużego sensu, ale wyobraźmy sobie, że zawiera inne metody, trochę bardziej rozbudowane niż przekazywanie parametrów do warstwy niżej. Rozmawiajmy tylko o podejściu.

Druga opcja to nie pisałbym ProcessDouble tylko stworzył mocka Process i

1. przy założeniu, że mock.argv() zwraca ["a", "b", "c"] napisałbym assert ["c"] == CommandLine(mock).args()
2. const text = "hello"; CommandLine(mock).write(text); assert.CalledWith(mock.write, text)

Zasadniczo nie widzę dużej różnicy między tymi podejściami. Jakie widzicie wady/zalety?

iksde napisał(a):`

Zasadniczo nie widzę dużej różnicy między tymi podejściami. Jakie widzicie wady/zalety?

Wygląda dobrze. Co do różnicy: praktycznie nie ma żadnej, mock (jak nie wołasz żadnego CalledWith i innych rzeczy sprawdzających co i ile razy zostało zawołane) to w gruncie rzeczy tworzenie takiego ProcessDouble tylko w mniej manualny sposób.

Co do assert.CalledWith(mock.write, text) to nie lubię używać takich instrukcji. Lepiej sprawdzać wynik a nie to, czy dana metoda została zawołana. Dużo lepiej napisać kod i testy w taki sposób, że w razie jakiejkolwiek zmiany (capturedOutput jest dobrym przykładem) możesz od razu wychwycić co się stało

slsy napisał(a):

Według mnie kod nie powinień być konkretnie pod testy. Z drugiej strony kod powinień być napisany tak generycznie, żeby dało się go używać w różnych kontekstach tj. produkcyjne czy w testach.
Przypadek z listenerem jest IMO zły. Prawidłowo powinno się go przekazać przez konstrutkor (DI) w taki sposób, żeby nie potrzebne były takie pomocniczne metody.

Co do całości: po to masz interfejsy, żeby nie było konieczne mieszanie kodu produkcyjnego z testowym. Kod produkcyjny używa interfejsów w miejscach, gdzie chcesz mieć zmienne zachowanie i tyle. Idea przeplatania jest zła, bo cierpi na tym czytelność i utrzymanie. Jak dany kawałek kodu nie jest używany produkcyjnie to przyszły archeolog twojego kodu ma problem z ogarnięciem po cholerę to tutaj jest

Też kiedyś tak uważałem, ale to wynikało trochę z niedostatków języków, w których pisałem. Tzn w wielu językach (np Java) podział pomiędzy kodem produkcyjnym a testowym może przebiegać tylko na granicy całych plików - tzn masz pewne pliki tylko testowe w osobnym katalogu (choć nawet mogą być w tym samym pakiecie). O tym czy plik jest włączony do ostatecznego buildu produkcyjnego decyduje system budowania projektu a nie kompilator.

Tymczasem da się znacznie lepiej. Istnieją języki obsługujące kompilację warunkową (C, C++, Rust, D) i tam można sobie dowolny fragment kodu, nawet jedną linijkę oznaczyć jako do testów. Wtedy nie ma pytania "po cholerę jest ten kod", bo widzisz czarno na białym #[cfg(test)]. Oczywiście realnie nikt chyba nie używa tego do przeplatania kodu co linijkę, ale jest to użyteczne np. jak musisz w testach dobrać się do prywatnych bebechów jakiegoś modułu - wtedy możesz to zrobić bez psucia API modułu dodając np. testowy getter czy dodatkowe testowe metody walidujace stan obiektu.
Dzięki temu nie komplikujesz niepotrzebnie kodu produkcyjnego tylko po to aby napisać testy.

W kodzie produkcyjnym nie powinno być nic pod testy. Jeżeli jest, to najprawdopodobniej testujesz implementację a nie funkcjonalność jednostek.

piotrpo napisał(a):

W kodzie produkcyjnym nie powinno być nic pod testy. Jeżeli jest, to najprawdopodobniej testujesz implementację a nie funkcjonalność jednostek.

Nie powinno się testować implementacji, zgadzam się, ale rozważany tutaj przypadek bynajmniej tego nie robi i autor to kilkakrotnie zaznacza, m.in tutaj:

Easy refactoring. Object interactions are considered implementation to be encapsulated, not behavior to be tested. Although the consequences of object interactions are tested, the specific method calls aren’t. This allows structural refactorings to be made without breaking tests.

Readable tests. Tests follow a straightforward “arrange, act, assert” structure. They describe the externally-visible behavior of the unit under test, not its implementation. They can act as documentation for the unit under test.

Pokazuje też kilka wzorców, które mają jeszcze bardziej odizolować testy od implementacji, albo przynajmniej ułatwić refaktoryzację.

piotrpo napisał(a):

W kodzie produkcyjnym nie powinno być nic pod testy. Jeżeli jest, to najprawdopodobniej testujesz implementację a nie funkcjonalność jednostek.

A kto powiedział, że nie wolno testować implementacji i jakie ma doświadczenie w utrzymaniu dużych projektów? ;)

Po pierwsze granica między implementacją a funkcjonalnością jest płynna i zależy mocno od skali. Oprogramowanie ma zwykle strukturę hierarchiczną - funkcjonalność małych jednostek stanowi implementację dużych. Jakby potraktować Twoją radę dosłownie to by należało przyjąć że jedyne dozwolone testy to testy end-to-end. A jak wiadomo testy end-to-end mają szereg wad (choćby nikła wartość diagnostyczna).

Po drugie testy dzielimy na white-box i black-box. Nie jest nigdzie powiedziane jakiej wielkości może być jednostka testowana. W testach white-box testujemy implementację. Jak najbardziej jest sensowne pisać testy prywatnych metod, bo czasem bywa że w takiej prywatnej metodzie masz całkiem skomplikowany algorytm, ale nie chcesz robić tej metody publicznej / chronionej w kodzie produkcyjnym tylko dlatego że testy.

Podobnie czasami zachodzi potrzeba weryfikacji stanu wewnętrznego testowanego systemu. Taki kod weryfikujacy jest oczywiście silnie związany z implementacją i powinien być blisko implementacji (można go traktować jako część implementacji), ale np. z pewnych względów nie może być umieszczony w finalnym produkcie np. ze względu na wydajność albo wprowadzane ryzyko.

Mała uwaga zanim się zapędzimy: Wszystko fajnie tylko, że obie alternatywy (Null implementacja (nie do końca ogarniam czym to się różni od mocka, poza wpierniczaniem w kod) i mocki) to raczej się stosuje jak nie da się po prostu
odpalić testów z bazą, systemem plików czy implementacją API.
Przeważnie się po prostu da.

jarekr000000 napisał(a):

Mała uwaga zanim się zapędzimy: Wszystko fajnie tylko, że obie alternatywy (Null implementacja (nie do końca ogarniam czym to się różni od mocka, poza wpierniczaniem w kod) i mocki) to raczej się stosuje jak nie da się po prostu
odpalić testów z bazą, systemem plików czy implementacją API.
Przeważnie się po prostu da.

Ale czas wykonywania takich testów szybko przekracza te umowne kilka(dziesiąt) sekund, co zaburza flow TDD. Do tego jak zrobię jakiś błąd w SQLu to padnie mi 10 innych testów, których w sumie nie obchodzi, czy coś się poprawnie zapisało.

Null implementacja (nie do końca ogarniam czym to się różni od mocka

Nullable nie sprawdza otrzymanych parametrów. Nullable nawet nie jest test doublem. Lepszą nazwą byłoby "Nullable infrastructure wrapper". To obiekt, który w miejsce swoich zależności ma podane stuby, które nie mają żadnych zależności, są samowystarczalne (to chyba wynika z definicji), zamiast implementacji, które mają (zewnętrzne) zależności, np. bazę danych.

Argument z czasem zupełnie rozumiem, aczkolwiek przykłady moje (akurat cos odpaliłem) 10 sekund na suitę która odpala 2 kontenery bazę i zewnętrzny komponent z API.
Da się z tym żyć.
Wolę to niż testy, które testują mocki czy też te "Nullable".

Argumentu z tym, że ileś testów padnie jak rozwale np. SQL nie rozumiem - to znaczy, nigdy nie miałem większego problemu, żeby znaleźć test, który pokazuje źródło problemu, nawet jak padło tych testów kilkadziesiąt.

Dla mnie, oba te podejścia (mocki i nullables o ile je rozumiem) mają tę samą wspólną wadę. Zastępują (lub wyłączają) jakiś kod, który chcę żeby był przetestowany. Owszem, czasami trzeba sobie radzić, ale jeżeli zastąpię np. repozytorium mockiem, testuję kontroler, to mogę jedynie przetestować, czy kontroler wywoła określone metody repozytorium. Z jednej strony, mało mnie to interesuje, bo chcę, żeby jakieś encje zostały utrwalone, a mam kompletnie w pompie, czy stanie się to za pomocą save(User), czy saveAll(List<User>), z drugiej strony istotna część funkcjonalności mojej aplikacji, czyli jej zdolność do utrwalenia i odzyskania obiektu nie zostanie przetestowana. W moim przekonaniu lepiej mieć testy, które działają chwilę dłużej, ale dają odpowiedź na pytanie "czy działa", niż takie, które działają błyskawicznie i tej odpowiedzi nie są w stanie zwrócić.

W przypadku tego co rozumiem jako nullables, pojawiają się co najmniej 2 dodatkowe wątpliwości:

• implementacja w kodzie produkcyjnym feature switch "pod testy"
• konieczność upewnienia się, że w kodzie produkcyjnym na produkcji (LOL) ten switch będzie miał odpowiednią wartość

iksde napisał(a):

Nullable nie sprawdza otrzymanych parametrów. Nullable nawet nie jest test doublem. Lepszą nazwą byłoby "Nullable infrastructure wrapper". To obiekt, który w miejsce swoich zależności ma podane stuby, które nie mają żadnych zależności, są samowystarczalne (to chyba wynika z definicji), zamiast implementacji, które mają (zewnętrzne) zależności, np. bazę danych.

Czy to nie jest po prostu to samo co Null Object Pattern?

somekind napisał(a):

iksde napisał(a):

Nullable nie sprawdza otrzymanych parametrów. Nullable nawet nie jest test doublem. Lepszą nazwą byłoby "Nullable infrastructure wrapper". To obiekt, który w miejsce swoich zależności ma podane stuby, które nie mają żadnych zależności, są samowystarczalne (to chyba wynika z definicji), zamiast implementacji, które mają (zewnętrzne) zależności, np. bazę danych.

Czy to nie jest po prostu to samo co Null Object Pattern?

Nie. Autor gdzieś pisał/mówił, że niefortunnie wybrał nazwę "Nullable" i precyzyjniejsze byłoby określenie "Nullable Infrastructure Pattern".

Null Object nic nie robi. Nullable Infrastructure Pattern to tworzenie obiektu stubując jego zależności.

Od autora (źródło):

One of the key ideas of the Nullable Infrastructure is that all the semantics of the infrastructure class remain exactly the same other than disabling external communication. So any logic inside the infrastructure code still runs. This is important because we don't have broad integration tests.
That's why the null code is inside the infrastructure, instead of using the classic Null Object pattern. It's also a major difference between it and classic test doubles. And perhaps your simulators?